Respect existing trailers (including co-author lines) when backporting

[Jackenmen]

Fixes python/miss-islington#439

[Jackenmen]

A small issue with this is that for a single commit PR, GitHub UI shows the trailers that are part of that single commit and it appends the trailers on its own too which causes them to be duplicated:

This doesn't break the actual functionality so that I don't think it's a big issue, and CPython probably relies on miss-islington's auto-merge so it might not even be an issue there anyway.

Either way, figured this quirk with GH UI is worth mentioning.

[ezio-melotti]

(One of the comments from my previous review got lost.)

It would be helpful to add a docstring and a couple more comments to this function to explain what it does, since it's not immediately obvious.
AFAIK and by looking at the tests, it seems it retrieves the original commit message and:

• adds a prefix at the beginning with the branch the commit is being backported to
• adds a line in the middle mentioning the original commit id
• adds the co-authors (and possibly other metadata?) at the end

I also wonder if there's a more straightforward way to do achieve this.
Can you use git interpret-trailers --only-trailers to extract the metadata and then do something like this?

f"""{commit_prefix}{original_commit_message}
(cherry picked from commit {self.commit_sha1})

{metadata}"""

[Jackenmen]

Welp, some value sure was lost by me not adding more comments in there in the first place... I added some now to explain the way this is supposed to work.

I also wonder if there's a more straightforward way to do achieve this. Can you use git interpret-trailers --only-trailers to extract the metadata and then do something like this?

f"""{commit_prefix}{original_commit_message}
(cherry picked from commit {self.commit_sha1})

{metadata}"""

I can look into an alternative way of doing this. The important things are:

• the trailers cannot break after the addition of (cherry picked from commit ...)
• adding Co-authored-by should not cause duplicates
• the order of the trailers probably shouldn't be affected more than needed by the changes we make (currently we only insert a new trailer at the beginning of existing trailers and don't touch them further)

One of the problems is that git interpret-trailers does not have a flag that does the opposite of --only-trailers so getting the "original commit message" may not be as easy as it seems. A naive way of doing it would be to cut the end of the full message by the length of the output of git interpret-trailers but I wouldn't be too surprised if this wouldn't work that well in practice (as I said at the beginning, some value sure was lost by me not writing some more comments when I wrote this PR so I'll have to look into that).

[ezio-melotti]

adding Co-authored-by should not cause duplicates

If we can isolate all the trailers (and --only-trailers seems to do that) you could pass them through a set() to remove duplicates. Even if the order is lost I'm not sure it matters. If you want to preserve it, it's pretty easy to write a function that removes duplicates without losing the order.

getting the "original commit message" may not be as easy as it seems.

Recently I was looking into manually adding co-authors, and apparently you had to leave two empty lines between the message and the trailers, so an msg = full_commit_msg.rsplit('\n\n', 1)[0] might work (assuming this is enforced). Otherwise we can cut the end like you said or remove individual trailer lines from full_commit_msg.splitlines().

[Jackenmen]

If we can isolate all the trailers (and --only-trailers seems to do that) you could pass them through a set() to remove duplicates. Even if the order is lost I'm not sure it matters. If you want to preserve it, it's pretty easy to write a function that removes duplicates without losing the order.

cherry-picker is Python 3.7+ so this could just rely on the insertion order of dictionaries, possibly with the help of dict.fromkeys().

Recently I was looking into manually adding co-authors, and apparently you had to leave two empty lines between the message and the trailers, so an msg = full_commit_msg.rsplit('\n\n', 1)[0] might work (assuming this is enforced).

The problem with this is that we would need to write a proper detection of trailers or we risk the trailers no longer being trailers. A single split is not enough as we plan on adding a trailer and that means we need to make sure that we add a trailer with other trailers. For example, if this were the original commit message:

Commit message

some text

Then we can't just simply add it to the last part:

Commit message

some text
Co-authored-by: Author Name <[email protected]>

On the other hand, we also have to keep all trailers together so if this were the original commit message:

Commit message

Co-authored-by: Co-Author Name <[email protected]>

Then this would not be acceptable:

Commit message

Co-authored-by: Co-Author Name <[email protected]>

Co-authored-by: Author Name <[email protected]>

For reference, here's the logic that git uses: https://github.com/git/git/blob/afa70145a25e81faa685dc0b465e52b45d2444bd/trailer.c#L818-L915

Otherwise we can cut the end like you said

Based on these two:
https://github.com/git/git/blob/afa70145a25e81faa685dc0b465e52b45d2444bd/trailer.c#L1108-L1123
https://github.com/git/git/blob/afa70145a25e81faa685dc0b465e52b45d2444bd/trailer.c#L982-L990
I think there's no alteration done to the printed trailers which would make this doable but I'll have to read into it just a bit more to be sure. I didn't want to delay my response further since I might not be able to get to it at all today so I have not yet carefully examined these two linked fragments of code.

or remove individual trailer lines from full_commit_msg.splitlines().

I think this falls into same category as rsplit but I'm not sure I fully understand what you mean by this.

[ezio-melotti]

I'm not sure I fully understand what you mean by this.

Given this:

$ cat msg.txt 
gh-12345: fix some broken stuff

Some stuff was broken and now
it's fixed.

Co-authored-by: Co-Author Name <[email protected]>
Co-authored-by: Author Name <[email protected]>

$ cat msg.txt | git interpret-trailers --only-trailers 
Co-authored-by: Co-Author Name <[email protected]>
Co-authored-by: Author Name <[email protected]>

You could do something like this:

>>> # get these from self.get_commit_message(self.commit_sha1).splitlines()
>>> with open('msg.txt') as f:
...   lines = [line.strip() for line in f]  # remove trailing whitespace/newlines
... 
>>> # get these from `git interpret-trailers --only-trailers`
>>> trailers = ['Co-authored-by: Co-Author Name <[email protected]>',
...             'Co-authored-by: Author Name <[email protected]>']
>>> # remove trailers from the full message
>>> for trailer in trailers:
...   lines.remove(trailer)
... 
>>> msg = '\n'.join(lines).strip()  # this is the msg without trailers
>>> trailers.append('Co-authored-by: Another Author')
>>> metadata = '\n'.join(trailers)  # these are the updated trailers
>>> print(f"""[3.10] {msg}
... (cherry-picked from somewhere)
... 
... {metadata}""")  # this is the final message
[3.10] gh-12345: fix some broken stuff

Some stuff was broken and now
it's fixed.
(cherry-picked from somewhere)

Co-authored-by: Co-Author Name <[email protected]>
Co-authored-by: Author Name <[email protected]>
Co-authored-by: Another Author

That is, if you take the full commit and remove the trailer lines you obtained with git interpret-trailers --only-trailers you'll be left with the message only. Once you have the message and the trailers you can update the trailers and recombine everything in a new msg. Do you think this will work?

[zware]

Kind of a tangent, but maybe we should make it a trailer? Cherry-Picked-From: {self.commit_sha1}?

[Mariatta]

The message cherry picked from commit ... is the default message generated by git when we use git cherry-pick -X. So I think it should be kept that way.

[Mariatta]

Or this can perhaps be addressed in a separate PR :)