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

[ericsnowcurrently]

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]

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]

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

[Mariatta]

I've created python/devguide#518

[ericsnowcurrently]

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]

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]

This is one of such mention about keeping the "original PR number" instead of the "Backport PR number": python/bedevere#14 (comment)
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]

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 ***@***.***> 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 ***@***.***> 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 ***@***.***> 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 ***@***.***> 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 ***@***.***> 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]

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]

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]

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