RFE automerge enhancements

The “automerge” label could generate better commit messages.

Example of commit messages generated by automerge:

commit c5b242f87f31286ad38991bc3868cf4cfbf2b681
Author: Ashwin Ramaswami <[aramaswamis@gmail.com](mailto:aramaswamis@gmail.com)>
Commit: Miss Islington (bot) <[31488909+miss-islington@users.noreply.github.com](mailto:31488909%2Bmiss-islington@users.noreply.github.com)>
Date:   Sat Aug 31 10:25:35 2019 -0500

bpo-37764: Fix infinite loop when parsing unstructured email
headers. (GH-15239)

Fixes a case in which email._header_value_parser.get_unstructured
hangs the system for some invalid headers. This covers the cases in
which the header contains either:
- a case without trailing whitespace
- an invalid encoded word

https://bugs.python.org/issue37764

This fix should also be backported to 3.7 and 3.8

https://bugs.python.org/issue37764
  • very long lines: usually, commit messages are wrapped to use 80 columns maximum
  • 3 empty lines then 2 empty lines, why? 1 empty line would be enough :slight_smile:
  • https://bugs.python.org/issue37764” link is redundant with “bpo-37764:”
  • https://bugs.python.org/issue37764” link is written twice (this issue might be specific to this specific PR/change)

The bot doesn’t log who approved the PR: “Commit: Miss Islington
(bot) <31488909+miss-islington@users.noreply.github.com>” is not
helpful. I would prefer to have directly the information in Git, rather than depending on GitHub (we might migrate to yet another forge in 10 years).

Victor

4 Likes

If I remember correctly the last time I looked this up I couldn’t find a way to specify the committee in git itself. Best you can get is a line in the commit message.

Oh, I’m perfectly fine with having it in the commit message. The minimum would be to include the GitHub handle. The best would be to have a format like “Full Name ”.

Just to be clear, is it fair to say that your concern here is with the commit messages created by the new feature of automerging initial commits (to master) rather than the more familiar feature of automerging backports, where Miss Islington uses the commit message from the original, manually-committed and -edited merge as a starting point?

I saw issues of commit messages with pull requests proposed on the master branch and merged using the “automerge” label added manually.

I understood that the pull request title and description are used to build the commit message: but the function which does that could be enhancement. Moreover, I’m not sure that users of the automerge label understood that properly.

I see now that the changes have already been made to add the “Automerge Triggerred by: @handle” in the commit messages, like this one.

It would be great if we didn’t use the “@” since Github tends to consider it as a mention, even in commit messages, even in forks and tends to send out notification emails.

For example, this one of the emails I received about a commit merged:

bpo-32793: Fix a duplicate debug message in smtplib (GH-15341)

_get_socket() already prints a debug message for the host and port.

https://bugs.python.org/issue32793

Automerge-Triggered-By: @maxking

-- 
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
https://github.com/PatrikKopkan/cpython/commit/c35c387bccf9b8447c9051f5d332c4770ccd08cb

I hope GitHub won’t send out notifications if we don’t use the “@”.

1 Like

I think this is a feature, not a bug. The bpo-37764 is good in the title, where space is limited, but spelling out the full link is very useful for:

  • new people who tend to be overwhelmed by abbreviations.
  • automatic linkifying (even if I install an extension, my terminal or GitHub don’t know about bpo)
  • future archaeologists, if bpo is abandoned and the abbreviation forgotten
1 Like

It won’t, so it’s a question on whether the automatic linking in the commit log when viewed on GitHub and the notifications are a good or bad thing.

I wouldn’t call it a bad thing, but I don’t think the minimal benefit outweighs the spam from “mention” emails.

Oh, nice! It would be even better with the full name :wink:

1 Like

It would solve my notification problem, but if the original intent is to lookup who merged it, handles are unique and easier to search than Full Names in Github.

I was thinking that we can write both. For example:

Automerge-Triggered-By: Abhilash Raj (@maxking)

I’m unable to recall the GitHub handle of all core devs, and I’m sure that people who are external to Python have even more troubles to recognize GitHub handles.

Some core devs use different nickanmes on the bug tracker, GitHub and IRC.

1 Like

To help with fixing this concern, I was working on Add github usernames to the experts index · Issue #507 · python/devguide · GitHub. It would be useful to have the full listing on the devguide, but I figured that starting with the experts (who are the most likely to be needed in PRs) would be significantly easier. Currently awaiting review on the first PR in that process, which adds a JSON file to the devguide Add JSON for storing expert data by aeros · Pull Request #517 · python/devguide · GitHub (as suggested by Mariatta and Zach Ware).

In the meantime, I usually refer to the committers list on bpo. But there’s not an easy way to link that to the experts index on the devguide, which was the main purpose of the JSON. Also, since we’re planning on moving away from bpo, that should probably not be the only location where GitHub handles are linked to the names of core developers.

But, for the purpose of the automerge enhancement, it’s probably better to just use the full names. The issue with looking up GitHub handles is not limited to this alone though, particularly for newer contributors or for those external to the organization.

I’m concerned that a contributor might hijack the full name of someone else on purpose, or more simply than two people can have the same full name. Recently, the rockstar Ben Harper contributed to Python!!!
https://bugs.python.org/issue36356

For the author of a commit, we have the email address which is unique. So I suggest to store the full name + GitHub handle.

1 Like

Ah, good point. Name collision is probably the more likely of the two. I honestly hadn’t considered that there are enough contributors for full name collisions to be an issue, but I suppose that’s a good thing. (:

Hey, you never know. :wink:

Are your thoughts similar on storing the expert data? I was thinking that the expert index in the devguide should use a platform-independent identifier, in case we move away from GitHub at some point in the distant future or need the expert info for other platforms. Since Python has already outlived several other VC platforms, it wouldn’t surprise me if we outlived GitHub as well.

An email address would be a better unique identifier, my only concern is that some of the experts may not want to publicly list their email address (this could be fixed by assigning all of them an @python.org email). The full name collision is a bit less of an issue for the experts index than it would be automerge, but it never hurts to be more scalable.

IMO we don’t necessarily need to do this since experts index is platform dependent when it comes to requesting review/tagging someone. Today, we use it for nosying experts on BPO and hence it uses bpo usernames, I can’t do that with email addresses.

When we move the issues to Github, perhaps we can just use the Github username. If the index is converted to a json/toml format, it can easily be changed by automation in future, when needed, from either the voter repo + some manul work for non-core-devs in the experts index.

That’s exactly what I was aiming to do with Add JSON for storing expert data by aeros · Pull Request #517 · python/devguide · GitHub. On the related issue page, @zware linked the script that was used by bpo to grab experts for the nosy list. If that PR is merged, I would be able to adjust the script to pull from the json instead of the experts index page. That would allow us to add GitHub usernames (in addition to the bpo usernames) to the table on experts.rst without having to worry about conflicts.

Can that be the GH handle without the @? For the example above:

Automerge-Triggered-By: Abhilash Raj (maxking at GitHub)

That way GH won’t send notifications. It also won’t linkify it, but I think that’s not a big loss.
Also, this way will be much more useful if we ever move away from GitHub.

3 Likes