On Thu, Jan 5, 2012 at 4:50 AM, Sylvain Lebresne <sylv...@datastax.com> wrote: > I agree that having merge commits each time a new "patch" is committed > is a pain and adds no useful information imo (to be clear I'm not > talking of actual merge (like say cassandra-1.0 -> trunk)), so +1 on > using git pull --rebase to avoid it.
Personally, the merge commits and extra(neous) history don't bother me. I favor rebasing a private topic branch only for purposes of creating a concise result for review. > Now, I'm a little slow so I'd like to make sure I understand how we expect > this to work. Currently the life of a ticket goes more or less like that: > - author attaches patches: 0001-uberidea.patch and 0002-unit-test.patch. > - reviewer makes some remarks, maybe some that require fairly intensive > changes. > - author attaches new version: 0001-uberidea-v2.patch and > 0002-unit-test-v2.patch. > - reviewer makes a few last small remarks. > - author attaches patch 0003-fixes-v3.patch. Note that it could also > attach a v3 of 0001 and 0002 instead, but let's say that those two are > big and the last remarks are small, so attaching a 0003 makes it > easier for the reviewer to quickly check how his last remark has been > addressed. > - reviewer +1 the patch. > - committer applies the patches on his local svn/git main branch and > push upstream. > > The way it could translate with git: > - author says "patches are available at github.com/author/666". > - reviewer makes remarks. > - author pushes some more commit on branch 666 (but he does not rebase > because it's a public branch). > - reviewer makes last remarks. > - author pushes one more commit (again, no rebasing). > - reviewer +1 the patch. > - Now github.com/author/666 has many commits but we want to commit > only one upstream because those commits are the back-and-forth of > review. So committer would be in charge of pulling 666 locally, squash > the commits, then rebase his local main branch against the result (or > cherry-pick the now unique commit) and push upstream? This discourages collaboration because anyone that might fork github.com/author/666 is sitting on a powder keg. Imagine the case where Alice pushes a topic branch for a new feature, and Bob forks that because his feature depends on it and he wants to test, or doesn't want to wait for it to be committed. Or perhaps Bob found a bug in Alice's code, or is taking her feature to the next stage. It's possible that this isn't even the only branch Bob has merged onto his. Let's hope Bob is on top of things. > Does that sound like what we all had in mind? > > It seem to me it puts a little bit more work on the committer (he has > to squash commits), but I can live with that. Also, if committer != > reviewer, there is the slight issue of how the committer make sure > that he commits what has been reviewer (i.e, that author hasn't made > some last minute, post-review change). But I suppose we can either say > "don't do that" and trust people, or ask reviewer to comment with > something like "+1 on 666:<sha1 of last commit>". At best it's yak shaving. At worst it's going to result in some very frustrated contributors. This is one of the major reasons why rebase is so contentious, and it's exactly why you hear so many people saying "don't rebase branches that have been published". -- Eric Evans Acunu | http://www.acunu.com | @acunu
