Should I merge a PR that I approved if it was written by a different core developer?
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@gmail.com Commit: GitHub noreply@github.com
commit 8f51bb436f8adfd139cad046b91cd462c7f27f6c (tag: v3.7.0a1) Author: Ned Deily nad@python.org Commit: Ned Deily nad@python.org
commit 9b47af65375fab9318e88ccb061394a36c8c6c33 Author: svelankar 17737361+svelankar@users.noreply.github.com Commit: Raymond Hettinger rhettinger@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?
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
Victor
On Wed, Sep 20, 2017 at 2:27 PM, Victor Stinner victor.stinner@gmail.com wrote:
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@gmail.com Commit: GitHub noreply@github.com
commit 8f51bb436f8adfd139cad046b91cd462c7f27f6c (tag: v3.7.0a1) Author: Ned Deily nad@python.org Commit: Ned Deily nad@python.org
commit 9b47af65375fab9318e88ccb061394a36c8c6c33 Author: svelankar 17737361+svelankar@users.noreply.github.com Commit: Raymond Hettinger rhettinger@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?
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
I think it's a good idea in many cases, but not required. E.g. you may be OK with the diff but still ask the author to clean up some small nits, and then they can merge their own diff. Or you may be OK with the diff but want to wait for some other reviewer's OK.
The good news is that it's no longer wrong, since the author is preserved regardless of who merges.
-- --Guido van Rossum (python.org/~guido)
I think it's a good idea in many cases, but not required.
I'm not sure that I understood correctly, what is a good idea? To merge the PR if I consider that it's now good enough to be merged?
E.g. you may be OK with the diff but still ask the author to clean up some small nits, and then they can merge their own diff.
Oh, my question was specific to a PR at the "LGTM." stage, after I even approved the PR.
I wrote my email after approving https://github.com/python/cpython/pull/3678 which is written by Antoine Pitrou. I asked a question, he replied, I like his answer. The patch is small and makes sense. So LGTM :-)
Or you may be OK with the diff but want to wait for some other reviewer's OK.
Yeah, it's not uncommon that I prefer to get a second review. Usually, I explicitly say it in a comment.
The good news is that it's no longer wrong, since the author is preserved regardless of who merges.
Yep, that's my point :-)
Victor
On Wed, Sep 20, 2017 at 2:49 PM, Victor Stinner victor.stinner@gmail.com wrote:
I think it's a good idea in many cases, but not required.
I'm not sure that I understood correctly, what is a good idea? To merge the PR if I consider that it's now good enough to be merged?
Sorry, yes.
with the diff but still ask the author to clean up some small nits, and
E.g. you may be OK then
they can merge their own diff.
Oh, my question was specific to a PR at the "LGTM." stage, after I even approved the PR.
I wrote my email after approving https://github.com/python/cpython/pull/3678 which is written by Antoine Pitrou. I asked a question, he replied, I like his answer. The patch is small and makes sense. So LGTM :-)
Or you may be OK with the diff but want to wait for some other reviewer's OK.
Yeah, it's not uncommon that I prefer to get a second review. Usually, I explicitly say it in a comment.
The good news is that it's no longer wrong, since the author is preserved regardless of who merges.
Yep, that's my point :-)
Seems we're in violent agreement! :-)
-- --Guido van Rossum (python.org/~guido)
On Thu, Sep 21, 2017 at 12:27 AM, Victor Stinner victor.stinner@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.
I was quite happy with that unwritten rule because it also gave me the opportunity to watch buildbots on my own time. If you merge my PR in the middle of the night in my time zone, I can't check buildbots in the next 15-20 hours since I don't always have time for CPython at work.
Also, I personally don't dump all of the items in my TODO list in every PR I opened so it would be better to let the author of the PR do the merging (or at least ask them if it's ready to be merged)
In the new Git era, the author and committer *can* be two different people. Examples with "git log --pretty=full":
Unfortunately, that doesn't work in practice because Buildbot doesn't care about the 'committer' field and you don't get any notification on IRC if you merged someone else's PR.
--Berker
On Wed, Sep 20, 2017 at 11:27 PM, Victor Stinner victor.stinner@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@gmail.com Commit: GitHub noreply@github.com
commit 8f51bb436f8adfd139cad046b91cd462c7f27f6c (tag: v3.7.0a1) Author: Ned Deily nad@python.org Commit: Ned Deily nad@python.org
commit 9b47af65375fab9318e88ccb061394a36c8c6c33 Author: svelankar 17737361+svelankar@users.noreply.github.com Commit: Raymond Hettinger rhettinger@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:
- 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.
- 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.
- 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.
- 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@python.org https://mail.python.org/mailman/listinfo/python-committers Code of Conduct: https://www.python.org/psf/codeofconduct/
On 21 September 2017 at 07:27, Victor Stinner victor.stinner@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.
I'll typically still just leave an "Approve" review rather than merging directly, unless it's a PR for an issue I filed, or I'm working on a separate change that would end up conflicting with their PR if I left theirs open.
That way if they think of an alternative approach that they'd prefer to use, they can still just amend their existing PR and ask for an additional review, rather than having to submit a new PR.
Cheers, Nick.
-- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
On Sep 20, 2017, at 20:54, Nick Coghlan ncoghlan@gmail.com wrote:
On 21 September 2017 at 07:27, Victor Stinner victor.stinner@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.
I'll typically still just leave an "Approve" review rather than merging directly, unless it's a PR for an issue I filed, or I'm working on a separate change that would end up conflicting with their PR if I left theirs open.
In general, I like the unwritten rule. There’s something very satisfying about finally hitting the merge button, and as Nick points out, you may want to do some final tweaks. I’ll usually just leave the Approve review too, and let the submitter/committer do the final merge.
That said, there are times when it’s useful to do the merge for them. If it was my PR, I might leave a note in the PR asking for a merge if I know I won’t get to it soon (maybe I’m going on vacation), or if as Nick says, it needs to get merged soon so as to avoid conflicts or blocking other people’s progress. I’d still like it to mostly come from the original submitter.
Cheers, -Barry
It's funny. At Dropbox, engineers *have* to merge their own work (we call it "landing"). When I first got to use GitHub (for asyncio, IIRC) I could land other people's patches once I approved of them. That was very satisfying, and felt like the "better" workflow, and we adopted this for mypy. Cases where it's better to wait for the author also exist, but it's always clear from the author's comments. In general merging sooner means fewer conflicts with other work, and we like that.
But I'll refrain from merging CPython patches by other core devs since it seems the consensus that usually the author (if a core dev) wants to merge their own work on their own time.
-- --Guido van Rossum (python.org/~guido)
середа, 20 вересня 2017 р. 23:27:49 EEST Victor Stinner написано:
My question is: is someone opposed that a core developer clicks on the [Merge] button for a PR proposed by a different core developer?
I'm opposed. The author can be more acquainted with the writing code than reviewers. The author sees it in wider context and knows its history. I continue to think about my patches even after creating a PR and several times rethinked it after approving by other core developers, that lead to rewriting or even rejecting a PR.
In additional, I think there is less probability that the author forgot to edit the commit message before clicking on the [Merge] button. ;-)
Ok, no problem, let's say that a core dev should not merge a PR written by another core dev.
In fact, I was already following this rule. I hesitated many times to click on Merge, but I wanted first to open a discussion. Here we are :-)
Obvious, the good practice is to put as many approval as possible, by different developers :-)
Victor
participants (7)
-
Barry Warsaw -
Berker Peksağ -
Ezio Melotti -
Guido van Rossum -
Nick Coghlan -
Serhiy Storchaka -
Victor Stinner