Reconsider GH-NNNN vs #NNNN in commit log

We decided using GH-NNNN in commit log since we moved to Github.
But there are still some #NNNN. It’s difficult to use GH-NNNN consistently. (see here)

Additionally, GH-NNNN makes commit log page on Github fork ugly.
See here

  • #12518” becomes “python#12518”.
  • “GH-12531” becomes “pythonGH-12531”.

How do you think about stop using GH-NNNN in commit log and just use #NNNN instead?

We need to be able to know which numbers reference github items after we move to the next version control system/service.

CPython has already lasted longer than Mercurial, SVN, and at least two others older than I am. It’s fair to expect that it’ll outlast git and Github too.

6 Likes

When GH-xxx is used, GitHub UI renders it directly as a link which is convenient.

Example: https://github.com/python/cpython/commit/8cbffc4d96d1da0fbc38da6f34f2da30c5ffd601 you can click on “GH-14504” and “GH-14515”. Sadly, GitHub is unable to recognize “bpo-37467”.

Our Roundup instances recognizes bpo-xxx and GH-xxx, but recognizes #xxx as a bpo number:
https://bugs.python.org/issue36853#msg351036

New changeset ebe709dc1d7c1f9f07dc7d77e53674d2500b223e by Jason R. Coombs (Anthony Sottile) in branch '3.7':
bpo-36853: Fix suspicious.py to actually print the unused rules (#13579) (#15653)
https://github.com/python/cpython/commit/ebe709dc1d7c1f9f07dc7d77e53674d2500b223e

#13579” and “#15653” link to the bug tracker, not to GitHub.

It’s more a practical issues.

Maybe migrating bugs to GitHub will solve some of these problems, but I also expect it will not magically solve all identifiers problems :slight_smile:

I expect that “prefix-identifier” reduces the risk of confusion.

A few years ago, Python bugs were hosted on SourceForge. Hopefully, the migration to Roundup succeeded to keep existing identifier. It’s quite common that I have to dig into very old bugs with bug identifier near 1 000 000: likely old SourceForge identifiers.

Aaaaaaah, migrations :slight_smile:

Note: I will not comment sha1 which may be Mercurial or Git commit identifier…

1 Like

The “automerge” label on GitHub replaces #xxx with GH-xxx in the PR title for you, but I dislike automerge because it produces bad commit messages :frowning:

Only if you don’t edit the opening comment first, which is usually forgotten :slight_smile:

3 Likes

I created RFE automerge enhancements to explain my concerns about automerge.

Could a bot change the title of the PR from (#NNNNN) to (GH-NNNNN)?

The title does not contain #NNNNN. It is automatically added to the commit message and you can edit it before confirming the merge. But many core developers forget to do this if they do not merge PRs regularly.

After merge, only by modifying history and force pushing the new head.

Since we always squash merge, this wouldn’t be horrible, as not even the PR owner has the commit immediately after merge. But the risk of getting conflicting histories and messing up people’s repos certainly goes up.

Perhaps the next time we change version control system we can fix up the commits with #NNNN based on dates? It’s not the worst thing ever if a few slip through.

We could use a user-script or browser add-on to do this automatically for us. With probably <100 people merging PRs, getting most of them to use it shouldn’t be impossible. I’d certainly use it to avoid making this error occasionally.

Personally, I’d also make it change the color of the merge button, so that I’d notice if it wasn’t active. Then we’d also have something to bikeshed! :wink:

Are we not able to use the git commit hooks to make the s/#/GH- change?

not when you commit/merge a PR using GitHub web ui,

As previously mentioned, the PR number (#NNNN) is only added by GitHub when you’re about to merge the PR using the web ui. If you don’t merge PR through GitHub web ui (e.g. if you write your own script to merge using GitHub API) then you can bypass this problem.

Merging a PR using GitHub web UI is actually two steps:

  1. You click the “Squash & merge” button. (it doesn’t actually commit/merge yet)
    After this you see the commit message text box, with (#NNNN) added to the end.
  2. You click “Confirm squash & merge”. (now this PR is merged using the commit message in the text box)

If people are eager to merge a PR, it is easy to speed through the process and just click “Confirm Squash & merge” without first changing #NNNN to GH-, or without first editing the commit message that appears in the textbox.

I personally would recommend using the automerge by miss-islington, which was meant to solve the problem of forgetting to change #NNN to GH-
And regarding “bad commit squash message”, that is also solved by editing the PR title and body, which is something you would need to do anyway if you were to merge a PR yourself.

1 Like

For this reason I have started to use the auto merge label. Maybe we could require that?

Automerge requires someone else to review the PR. And while I agree that’s an ideal thing to require, those of us who maintain the more esoteric aspects of the repo (such as installers, tools or build scripts) could be seriously impeded by that. The night watch team (Victor and Pablo) would also not be able to apply quick fixes.

Personally I’m fine with the current setup. I haven’t missed updating the reference in quite a while now (thanks to the reminder message training me well), and I prefer editing the message at that late stage rather than getting whatever mix of commit messages happen to be included. And the automerge option is available for anyone’s PR besides your own, which should be most normal PRs.

3 Likes

Automerge uses the PR’s description so that it may be used for the extended commit message. It works, but I usually avoid using automerge to avoid having a bunch of not-important-enough text in the commit message, without clobbering the PR description.

1 Like

I have some complains about automerge: RFE automerge enhancements

I only use the web UI to merge a PR. I’m trying to always modify the title to replace #xxx with GH-xxx. I’m painful, but it’s also common that I modify the commit message, or even sometimes rewrite it fully when it’s really irrelevant.

One issue with the web UI is that if someone merges another PR while you’re merging a PR, the merge fails. GitHub shows a “Retry” button: if you click, the PR is merged with the old unmodified PR title… (with #xxx instead of GH-xxx for example). I reload the page instead to workaround this issue.

2 Likes

One of problems with auyomerge which nobody mentioned earlier is that it loses the information about the commiter name. I often read email notifications about commits and the name of the core developer who pressed the Merge button is an important part of information to me.

It can also fool the tools which determines the authorship of the code to suggest reviewers.

1 Like

It’s at the end of the commit message, e.g. [3.7] bpo-38316: Fix co_stacksize documentation (GH-16983). (GH-17660) · python/cpython@917419f · GitHub shows Victor added the label.

1 Like

This is related to a somewhat recent change in our workflow, where a core dev’s PR doesn’t automatically become "awaiting merge".

If it is important for miss-islington to automerge coredev’s PR even if it is not "awaiting merge" then I’m sure we can figure out a way to facilitate this.