[python-committers] Should I merge a PR that I approved if it was written by a different core developer?

[Ezio Melotti]

On Wed, Sep 20, 2017 at 11:27 PM, Victor Stinner <victor.stinner at gmail.com>
wrote:

> Hi,
>
> Before the GitHub era, in the old "Mercurial era", the unwritten rule
> was to not merge a patch written by a developer who has the commit
> bit, to not "steal" his/her work. The old workflow (patches attached
> to the bug tracker) didn't allow to easily keep the author. You had to
> find the author email and full name and specify it manually.
>
> Moreover, there was a written rule of using the name of the developer
> who actually pushed the commit, so the commiters took the
> responsability of any regression (reminder the old era with no
> pre-commit CI? ;-)).
>
> In the new Git era, the author and committer *can* be two different
> people. Examples with "git log --pretty=full":
>
> commit 9abee722d448c1c00c7d4e11ce242ec7b13e5c49
> Author: Victor Stinner <victor.stinner at gmail.com>
> Commit: GitHub <noreply at github.com>
>
> commit 8f51bb436f8adfd139cad046b91cd462c7f27f6c (tag: v3.7.0a1)
> Author: Ned Deily <nad at python.org>
> Commit: Ned Deily <nad at python.org>
>
> commit 9b47af65375fab9318e88ccb061394a36c8c6c33
> Author: svelankar <17737361+svelankar at users.noreply.github.com>
> Commit: Raymond Hettinger <rhettinger at users.noreply.github.com>
>
> My question is: is someone opposed that a core developer clicks on the
> [Merge] button for a PR proposed by a different core developer?
>
>
Personally I would prefer if other core devs just left a positive review
without merging, for a few reasons:

1) before merging I want to double check that everything is fine, and
possibly tweak a few minor things before merging.  Some authors also push
WIP or working but unpolished PRs to get some early feedback before
following up with more iterations, and if they don't specify it clearly you
might end up merging too early.
2) even if you are not "stealing their work", you deprive them of the
closure they get by finally merging a PR they have been working on for some
time.
3) some tools (e.g. buildbot) might only look at the committer field, and
this might end up notifying the wrong person or skewing some statistics.
4) I don't see a reason to do that in the first place: I expect the author
to quickly merge a PR once all the checks pass (especially since they can
now do it from their smart watch while lying on the beach), and even if the
author takes a few days to do so, it usually doesn't really matter.


> IMHO having a committer different than the author is valuable since
> the responsability is now shared by two developers instead of single
> one. It's similar to the "Signed-Off" tags used by the Linux kernel,
> but the list is limited to a single Signed-Off :-) Well, the committer
> is usually seen as the most reponsible, but now we can complain to the
> author as well *if needed* :-D
>
>
IME if the author is not a core dev, the responsibility falls on the
committer; if the author is a core dev, the responsibility falls on the
author/comitter (i.e. the core-dev).  If core devs merge their own patches,
the responsible will always be the committer.

If you merge other core devs' patches, you are likely to get blamed when
something breaks even if the code wasn't yours, and if the author runs away
and you want someone else to blame, you can always find all the reviews by
looking at the PR referenced by the changet :)

Best Regards,
Ezio Melotti


> Victor
> _______________________________________________
> python-committers mailing list
> python-committers at python.org
> https://mail.python.org/mailman/listinfo/python-committers
> Code of Conduct: https://www.python.org/psf/codeofconduct/
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/pipermail/python-committers/attachments/20170921/ca7dfefd/attachment.html>