Hi Michelle, I've added some comments below. - Justin
On 2011-03-08, at 2:55 PM, Michelle D'Souza wrote: > 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. This is interesting. In git branches are a bit different than in svn. In git, if you delete a branch it will actually get garbage collected, but until then you can still do some digging to find it again. I'm wondering if this commented code is just still visible because the garbage collection hasn't run yet. Apparently github does garbage collection on push, so it shouldn't be too hard to test that. > > >> 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 agree that we should probably be moving away from the patch/review process, but I think there may be cases where we will need it. Not sure what those are off the top of my head though. I think your point about wanting it to be clear about changes that occur between the time of commit and time of merge, is a good argument for keeping with merge over rebase. It's true that with rebasing you still get the dates, but it becomes less straightforward to find out what happened in the interim, as your changes become scattered. I think the timestamp is pretty much the only way we can tell the order of changes reliably in git, since the commit hash isn't useful for this. > >> 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: But I think if we don't want to be doing the patch/review, that we won't want to squash commits and alter commit messages. Maybe the contributor should be handling this type of thing. As with any of this I'm not so set to not accept any other view points though :) > > 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
