[Numpy-discussion] commit rights for Nathaniel

[Fernando Perez]

A couple of notes from the IPython workflow in case it's of use to you guys:

On Tue, Jun 5, 2012 at 10:52 AM, Charles R Harris
<charlesr.harris at gmail.com> wrote:
>
> For the commits themselves, the github button doesn't do fast forward or
> whitespace cleanup, so I have the following alias in .git/config
>
> getpatch = !sh -c 'git co -b pull-$1 master &&\
>            curl https://github.com/numpy/nump/pull/$1.patch|\
>            git am -3 --whitespace=strip' -
>
> which opens a new branch pull-nnn and is useful for the bigger commits so
> they can be tested and then merged with master before pushing. The
> non-trivial commits should be tested with at least Python 2.4, 2.7, and 3.2.
> I also suggest running the one-file build for changes in core since most
> developers do the separate file thing and sometimes fail to catch single
> file build problems.

1) We've settled on using the green button rather than something like
the above, because we decided that having the no-ff was actually a
*good* thing (and yes, this reverses my initial opinion on the
matter).  The reasoning that convinced me was that the merge commit in
itself is signal, not noise:

- it indicates who did the final reviewing and merging (which doesn't
happen in a ff merge b/c there's no separate merge commit)

- it serves as a good place to cleanly summarize the PR itself, which
could possibly contain many commits.  It's the job and responsibility
of the person doing the merge to understand the PR enough to explain
it succinctly, so that one can read just that message and get a
realistic idea of what the say 100 commits that went in were meant to
do.  These merge commits are the right thing to read when building
release notes, instead of having to slog through the individual
commits.

- this way, the DAG's topology immediately shows what went in with
review and what was committed without review (hopefully only
small/trivial/emergency fixes).

- even if the PR has a single commit, it's still OK to do this, as it
marks the reviewer (and credits the reviewer as well, which is actual
work).

For all these reasons, I'm very happy that we reversed our policy and
now *only* use the green button to merge, and *never* do a FF merge.
We only commit directly to master in the case of absolutely trivial
typo fixes or emergency 'my god master is borked' scenarios.

2) I'd encourage you to steal/improve our  'test_pr / post_pr_test'
as well as git-mrb tools:

https://github.com/ipython/ipython/blob/master/tools/test_pr.py
https://github.com/ipython/ipython/blob/master/tools/post_pr_test.py
https://github.com/ipython/ipython/blob/master/tools/git-mrb

In particular test_pr is a *huge* help.  We now almost never merge
something that doesn't have a test_pr report.  Here's an example where
test_pr revealed initially problems, later fixed:

https://github.com/ipython/ipython/pull/1847

Once the fix was confirmed, it was easy to merge.  It routinely
catches python3 errors we put in because most of the core devs don't
use python3 regularly.  But now I'm not worried about it anymore, as I
know the problems will be caught before merging (I used to feel guilty
for constantly breaking py3 and having poor Thomas Kluyver have to
clean up my messes).

Cheers,

f