[Numpy-discussion] commit rights for Nathaniel

Charles R Harris charlesr.harris@gmail....
Tue Jun 5 18:15:53 CDT 2012


On Tue, Jun 5, 2012 at 4:59 PM, Fernando Perez <fperez.net@gmail.com> wrote:

> 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@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).
>
>
There are other advantages to pulling down the patch. Fixups can be merged
together, commit comments enhanced, whitespace removed, style cleanups can
be added, tests can be run, and the PR is automatically rebased. I still
like fast forward for single commit merges, for larger merges I specify
no-ff so that things come in as a well defined chunk.

Chuck
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.scipy.org/pipermail/numpy-discussion/attachments/20120605/040ac0fd/attachment.html 


More information about the NumPy-Discussion mailing list