[IPython-dev] Pull request workflow...

[Fernando Perez]

I would like to add, from today's experience and thinking:

On Sun, Oct 10, 2010 at 2:01 PM, Fernando Perez <fperez.net at gmail.com> wrote:
> For reviewers:

- When possible, rebase the branch you're about to merge into trunk
before applying the merge.  If the rebase works, it will make the
feature branch appear in all displays of the log that are
topologically sorted as a contiguous set of commits, which makes it
much nicer to look at and inspect later, as related changes are
grouped together.

Consider for example when I merged Min's cursor fixes, a tiny
one-commit branch, where I did *not* rebase:

* |   b286d0e Merge branch 'cursor' of git://github.com/minrk/ipython into trunk
|\ \
| * | 6659636 fixed some cursor selection behavior
* | | f0980fb Add Ctrl-+/- to increase/decrease font size.
* | | 8ea3b50 Acknowledge T. Kluyver's work in credits.
| |/
|/|
* | 78f7387 Minor robustness/speed improvements to process handling

The threads of multiple branches there make the graph harder to read
than is necessary (with a gui tool the lines are easier to see, but
the problem remains).

Contrast that with today, when I merged Thomas' 2to3 preparation
branch.  He had done multiple merges *from* trunk, that made the graph
horrible.  But rather than bouncing it back to him, I tried simply
calling rebase onto trunk, and in 10 seconds git had it ready to merge
in a nice, clean and easy to read set of grouped commits:

*   572d3d7 Merge branch 'takowl-ipy3-preparation' into trunk
|\
| * 468a14f Rename misleading use of raw_input so it's not
automatically converted to input by 2to3.
| * 91a11d5 Revert "Option for testing local copy, rather than global.
Testing needs more work." (Doesn't work; will use virtualenv instead)
| * b0a6ac5 Option for testing local copy, rather than global. Testing
needs more work.
| * be0324d Update md5 calls.
| * 10494fd Change to pass tests in IPython.extensions
| * edb052c Replacing some .items() calls with .iteritems() for
cleaner conversion with 2to3.
| * d426657 Tidy up dictionary-style methods of Config (2to3 converts
'has_key' calls to use the 'in' operator, which will be that of the
parent dict unless we define it).
| * b2dc4fa Update use of rjust
| * 3fd347a Ignore .bak files
| * 50876ac Ignore .py~ files
| * 760c432 Further updating.
| * 663027f Cleaning up old code to simplify 2to3 conversion.
|/
* 263d8f1 Complete fix for __del__ errors with .reset(), add unit test.

This is the kind of graph that is a pleasure to read, and makes it
easy to inspect how a feature evolved.  Note that all commits retain
their original timestamps, it's just the topological sort that
changes.

Obviously there will be cases where a rebase may fail, and at that
point there's a judgment call to be made, between accepting a messy
DAG and asking the original author to rebase, resolve conflicts and
resubmit.  Common sense should prevail: we want a clean history, but
not at the cost of making it a pain to contribute to ipython, so a
good contribution should be accepted even if it causes a messy history
sometimes.

It's just that via good practices, we can reduce to a minimum the need
for such messes, and we'll have in the long run a much more
comprehensible project evolution.

Regards,

f