Hi Justin, Thanks for the response - my comments are inline below.
Michelle On 2011-03-08, at 1:18 PM, Justin Obara wrote: > Hi Michelle, > > Nice experimentation. It's pretty detailed. Here are some comments and > questions. > > In regards to losing code review comments, what would happen if Golam were to > delete his branch/fork, would we still see the code review comments? The commit and the code review comments will still be there but the commit won't be attached to the upstream repo in any way. I did a little experiment on my fork and created a branch, did a commit, commented on the commit and then deleted the branch. Here's the commit: https://github.com/michelled/infusion/commit/5dd89c6c4abdc4366f05cf2fa07b672a4b957c2c One way of retaining this information is by adding a comment to the appropriate JIRA with a link to the commit where code review was done. > By using rebase, the commits are pulled out of chronological order. You can > see this in your examples where Golam's commit on Feb 23 is on top of my > commit on Mar 6. This might make it confusing when looking through the > history. I guess that depends on how we interpret the history. I took a look at jquery ui and noticed that their history shows out of chronological order. It seems that on the network graph the commit will show on the day it was merged in not the day it was originally committed which will show in the log. For example commit 74b7c3f684f5d83effcee6d5099c6fffb020fdf9 was committed on February 17th but shows on their network graph on March 7th. > Another option would be to just go back to the patch/review process. > Good information about creating and applying patches > http://ariejan.net/2009/10/26/how-to-create-and-apply-a-patch-with-git/ > githubs help shows how to use git am with pull requests > http://help.github.com/pull-requests/ > I feel that we would be losing some of the benefit of moving to git if we go back to the patch review process. When I do patch review, I end up writing the commit log instead of the author. Also, the date that the author originally committed the work might be interesting. For example, if a framework feature is implemented between the time of commit and the time of merge it would be clear why it wasn't used in the original commit. > I think my preference would still be to use merge. You can do "git log > --no-merges" to view the logs and not see the merge commits. This seems like > the best of both worlds since you'll see only the history you want, and it > will remain in order, although you do have to add another flag. Ah, good to know. That will be useful regardless of what we decide here because we will still have the merges of the working branches into the upstream master. > > If we go with the patch/review process, I think we should use "git am" as you > can do "git am -signoff" to signoff on the patch when you commit it. This > will add another line to the comment. > This ends up with something very similar to the rebase strategy but I don't get the opportunity to squash commits or alter commit messages. Here's what it looks like: commit e41abefcdcbf486b48285af9b8efa22902b7b4f7 Author: Golam <golam@golam-desktop.(none)> Date: Mon Mar 7 16:13:25 2011 -0600 FLUID-4044: refactored some more of the codes for inline-edit unit tests. commit 13fe817fcf528722e4832d79213da3e048ab8071 Author: Golam <golam@golam-desktop.(none)> Date: Thu Mar 3 12:43:15 2011 -0600 FLUID-4044: cleaned the inline-edit unit test codes with jslint and refactored some more of the codes. commit f9a4b9bee8496090bf0eeba5309303303a466bcf Author: Golam <golam@golam-desktop.(none)> Date: Wed Mar 2 10:48:11 2011 -0600 FLUID-4044: cleaned the inline-edit unit test code with jslint and refactored some of the code. commit 939b2e16f62dee97643ce10eb6856650a6663042 Author: Golam <golam@golam-desktop.(none)> Date: Tue Mar 1 13:31:13 2011 -0600 FLUID-4044: cleaned the inline-edit unit test code as per code reviewer recomendation. commit 21fce8c144f087fbfdc475b2a2ba2d18dada8a0e Author: Golam <golam@golam-desktop.(none)> Date: Fri Feb 25 13:47:43 2011 -0600 FLUID-4044: cleaned the inline-edit unit test code as per Jslint recomendation. commit e8ffac52f93c7eb0c1114488b29a5403fd6b3e46 Author: Golam <golam@golam-desktop.(none)> Date: Wed Feb 23 10:23:25 2011 -0600 FLUID-4044: Conversion of the manual test cases to automated test cases. commit d800c49b532c6535b323e0df9d04540a4b390b76 Author: Justin Obara <[email protected]> Date: Sun Mar 6 17:33:32 2011 -0500 FLUID-4127: cleaning up the globals comments I've updated the globals comment at the top of all of our js files, making them consistent. They all have a comment above commit 97841c6c3b03cbbc8bccc3192f799756bd266e5a Merge: a580593 4151fcc Author: Michelle D'Souza <[email protected]> Date: Fri Mar 4 15:33:52 2011 -0500 Merge remote branch 'colinbdclark/FLUID-4131' Notice that the commit hashes don't match the commit hashes in Golam's branch. > Thanks > Justin >
_______________________________________________________ fluid-work mailing list - [email protected] To unsubscribe, change settings or access archives, see http://fluidproject.org/mailman/listinfo/fluid-work
