python / core-workflow

Issue tracker for CPython's workflow
https://mail.python.org/mailman/listinfo/core-workflow
Apache License 2.0
95 stars 59 forks source link

Original PR number in backport PR/commit title is noisy and slightly confusing. #346

Open ericsnowcurrently opened 5 years ago

ericsnowcurrently commented 5 years ago

When I click on the "Squash and Merge" button for a generated backport PR, I end up with a commit title that has 2 different PR numbers in it: the original and the new one. I'd expect only the new PR number and that the original be in the commit body instead. I expect that would mean changing the title and body that get generated for the backport PR.

Below is a concrete example from a PR I just backported.

Actual

PR:

[3.8] bpo-36487: Make C-API docs clear about what the main interpreter is. (gh-12666)

(cherry picked from commit 854d0a4)

Co-authored-by: Joannah Nanjekye <33177550+nanjekyejoannah@users.noreply.github.com>

Commit (from "Squash and Merge"):

bpo-36487: Make C-API docs clear about what the main interpreter is. (gh-12666) (gh-15080)

(cherry picked from commit 854d0a4)

Co-authored-by: Joannah Nanjekye <33177550+nanjekyejoannah@users.noreply.github.com>

Expected

PR:

[3.8] bpo-36487: Make C-API docs clear about what the main interpreter is.

(cherry picked from commit 854d0a4) (gh-12666)

Co-authored-by: Joannah Nanjekye <33177550+nanjekyejoannah@users.noreply.github.com>

Commit (from "Squash and Merge"):

bpo-36487: Make C-API docs clear about what the main interpreter is. (gh-15080)

(cherry picked from commit 854d0a4) (gh-12666)

Co-authored-by: Joannah Nanjekye <33177550+nanjekyejoannah@users.noreply.github.com>

Note that I moved the original PR number down into the body (right after the commit ref).

Mariatta commented 5 years ago

The second PR number is autogenerated by GitHub. Before clicking Squash and Merge, we have the opportunity to remove the second PR number.

In devguide there is a recommendation to remove the backport PR, but maybe not clear enough 😞 : https://devguide.python.org/gitbootcamp/#backporting-merged-changes

When formatting the commit message for a backport commit: leave the original one as is and delete the number of the backport pull request.

Was there a reason not to automerge that PR? miss-islington automatically removed the second GH- when automerging.

Mariatta commented 5 years ago

Closing as won'tfix. This is already handled correctly by automerge. If anything, perhaps Devguide can be improved.

Mariatta commented 5 years ago

I've created https://github.com/python/devguide/issues/518

ericsnowcurrently commented 5 years ago

Thanks @Mariatta. What do you mean by "the second PR number". miss-islington creates the PR with only one PR number in the title (the original PR). The only case currently where there are two PR numbers is in the commit title for the backport PR. I would hope the first one could be removed (moved to the body), not the second one. That first one could be taken care of when miss-islington created the backport PR.

As to why I didn't auto-merge, I was in a merge-it-myself mood today. :)

BTW, thanks for the bots. They make such a huge difference!

Mariatta commented 5 years ago

Sorry! I re-read the issue again, and I've misunderstood it the first time.

The only case currently where there are two PR numbers is in the commit title for the backport PR. I would hope the first one could be removed (moved to the body), not the second one.

There was discussion in the past that people prefer keeping the original PR number, instead of the backport PR number.

I would hope the first one could be removed (moved to the body), not the second one. That first one could be taken care of when miss-islington created the backport PR.

We can look into this, once we're in agreement that we prefer the backport PR number instead of the backport PR number

Mariatta commented 5 years ago

This is one of such mention about keeping the "original PR number" instead of the "Backport PR number": https://github.com/python/bedevere/issues/14#issuecomment-399357644 I think there was prior discussion about that but I couldn't locate it easily now.

From technical point of view, changing the commit message to the suggested format of:

[3.8] bpo-36487: Make C-API docs clear about what the main interpreter is.

(cherry picked from commit 854d0a4) (gh-12666)

Will require modifying bedevere, cherry-picker.py, and miss-islington. cherry-picker.py is the one that construct the PR commit title. Bedevere looks for the original PR number in the title, so it can remove the "needs backport to " label. miss-islington also looks for the original PR number in the title, to notify the original PR author about status checks.

It can be done, just needs more coordination :) So perhaps this is more suitable under core-workflow repo. I'll move it there.

methane commented 5 years ago

FYI, the devguide recommends removing new (backporting) PR number and keeping original PR number.

https://devguide.python.org/gitbootcamp/#backporting-merged-changes

On Mon, Aug 5, 2019 at 4:09 AM Eric Snow notifications@github.com wrote:

When I click on the "Squash and Merge" button for a generated backport PR, I end up with a commit title that has 2 different PR numbers in it: the original and the new one. I'd expect only the new PR number and that the original be in the commit body instead. I expect that would mean changing the title and body that get generated for the backport PR.

Below is a concrete example from a PR I just backported.

Actual

PR:

[3.8] bpo-36487: Make C-API docs clear about what the main interpreter is. (gh-12666)

(cherry picked from commit 854d0a4)

Co-authored-by: Joannah Nanjekye 33177550+nanjekyejoannah@users.noreply.github.com

Commit (from "Squash and Merge"):

bpo-36487: Make C-API docs clear about what the main interpreter is. (gh-12666) (gh-15080)

(cherry picked from commit 854d0a4)

Co-authored-by: Joannah Nanjekye 33177550+nanjekyejoannah@users.noreply.github.com

Expected

PR:

[3.8] bpo-36487: Make C-API docs clear about what the main interpreter is.

(cherry picked from commit 854d0a4) (gh-12666)

Co-authored-by: Joannah Nanjekye 33177550+nanjekyejoannah@users.noreply.github.com

Commit (from "Squash and Merge"):

bpo-36487: Make C-API docs clear about what the main interpreter is. (gh-15080)

(cherry picked from commit 854d0a4) (gh-12666)

Co-authored-by: Joannah Nanjekye 33177550+nanjekyejoannah@users.noreply.github.com

Note that I moved the original PR number down into the body (right after the commit ref).

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

-- Inada Naoki songofacandy@gmail.com

ericsnowcurrently commented 5 years ago

Thanks for the update, @Mariatta (and @methane). If this has already been discussed/decided then I say don't worry about it. Maybe no one considered moving the original PR number to the body? It's more likely this boils down to personal preference, which hardly seems like a great reason here to make any changes. :)

Regardless, I don't feel comfortable asking anyone to do a bunch of work if it doesn't objectively improve things. I may take a look if I have a few minutes. @Mariatta, you said there would have to be changes to bedevere. What would have to change there?

Mariatta commented 5 years ago

In backport.py we try to locate the original PR number: https://github.com/python/bedevere/blob/e80b3bc6f1bc1137fa642caf29aa4bda63d4854c/bedevere/backport.py#L59

We need to make sure that it will still work if the original PR number is in the bottom (after the cherry-picked from ... instead of in the title. Probably add tests to cover this case.

brettcannon commented 5 years ago

So should we close this issue? Or do you want to leave this open for your test case idea, @Mariatta ?