[IPython-dev] Pull request workflow...
Tue Oct 12 14:12:32 CDT 2010
On Tue, Oct 12, 2010 at 8:59 AM, Brian Granger <email@example.com> wrote:
> Definitely, but I do agree with Hans that this is really a problem
> with the viewer, not the DAG itself. But, I definitely agree with you
> that the rebased version is much cleaner.
Yes, I agree, and Mark's mention of gitk shows that there are even
viewers who do a better job in this regard (I had looked with gitg and
qgit, but hadn't opened gitk in a few weeks and didn't realize it was
a bit smarter in its sorting).
>> These branches aren't meant for third-parties to follow, since they
>> are being proposed for merging into trunk, so I don't see the rebasing
>> as an issue fort third-parties. In fact, even without rebasing,
>> following these branches is never a good idea since people are likely
>> to regularly prune their repo from obsolete branches (I know I do, and
>> I've seen others do it as well). So I think for these types of
>> branches, the argument of possible headaches for downstream users of
>> the branches isn't very compelling.
> I don't think the cost of rebasing is something that users/third
> parties pay, but rather a cost that we, as developers pay. Consider
> the following:
> 1. I work in branch foo, rebase it on top of master and then post on
> github as a pull request.
> 2. People comment on the work and I have to make additional commits to
> address the comments.
> 3. If we always try to rebase, I have to create a *new* foo2 branch
> that has my recent commits rebased and post that to guithub. But
> because it is a new branch, I have to submit a new pull request and
> the discussion has to continue in a manner that is disconnected from
> the original foo pull request.
> 4. This creation of new branches has to be repeated for each
> comment/edit/rebase cycle.
> This is a significant cost for developers, and I simply don't think it
> is worth the effort. Not to mention that creating/deleting lots of
> branches is error prone.
> This is not to say I don't think that sometimes rebasing is a great
> idea. It definitely is. But, I think we want to continue to use
> non-rebased merges as a part of our regular workflow. I should say
> that if rebasing didn't have this extra cost for developers, I would
> be totally fine with it being the norm in most cases.
Good points, and I think we're finding a good balance. I'm more than
happy to go with what seems to be the consensus on this one, thanks a
lot for entertaining this discussion (by the way, this thread is
proving very useful for numpy, where more or less the same discussion
started a day later). It's a good thing to go carefully through this
once, and we'll work smoothly for a long time.
As soon as I can find a spare minute, I'll try to summarize this and
other small points about our worflow in our docs, so that we can refer
to it more easily in the long run.
> For simple pull requests where there are no additional commits, I
> think rebasing is just fine. It is mostly in the more complex
> review/commit/rebase cycles.
Do we think balance is the following?
- From the proposer's side, rebase *before* you make your first pull
request. After that, don't worry too much about any more rebasings.
That will give a reasonably clean starting point for the review, as
well as ensuring that the branch applies cleanly. For developers
(like Min points out with his example) who are very comfortable with
rebasing in-review they can do so (like he did), but we shouldn't
*ask* that they do.
- And no rebasing from the committer's side like I originally
proposed, except in cases where significant criss-cross merges need to
be cleaned up.
And we could make the idea of an initial rebase 100% optional, only to
be done by those who feel comfortable with git. I know the word
'rebase' scares many new to git, and we don't want to put artificial
barriers to contribution. So it may be best to say that these are our
suggestions for more advanced developers proposing complex merges, but
that we will still accept any non-rebased pull request, as long as it
doesn't merge *from trunk*.
How does this sound for our final take?
More information about the IPython-dev