LWN: Comments on "Change IDs for kernel patches"

Git needs a information preserving rebase

wtarreau — Tue, 17 Sep 2019 09:57:08 +0000

I just had the same idea when reading the discussion above. We could have "git rebase", "git am", "git commit --amend", "git cherry-pick" automatically append a tag "original-id:" with the ID of the original patch. In case of fixup/squash, it may very well merge all of them. This way we do keep good traceability in patch series with exactly *zero* effort nor risk of mistake on the developer's side. In fact the developer would only make an effort to remove the tag!

This would also cover part of what is already being done with "cherry-pick -x" that's used a lot for backports. And very often the information provided there would be useful because it would reference a commit ID which is available in another public tree, and even if that tree is not publicly available it would at least serve as a common tracking key.

Git needs a information preserving rebase

zblaxell — Mon, 16 Sep 2019 16:01:23 +0000

Indeed, the whole point of git rebases is that they are *not*
information-preserving. Rebase is a tool to discard irrelevant cruft:
typos, failed experiments, rejected patches, accidentally added binaries,
commits where you forgot to add "-s", and so forth.

If someone comes up with a way to import and archive patch review
discussions (perhaps in a separate branch or tree) and reference them
in git commits in a way that plays nicely with 'git push' and 'git gc',
then rebase should preserve _that_.

Git needs a information preserving rebase

mathstuf — Mon, 16 Sep 2019 13:51:48 +0000

Maybe only if those commits are actually available somewhere? References to commits which only existed locally before rebases seems like pure noise to me. Also, what do you do for split/joined commits? Do the magic fixup! or squash! prefixes suppress this? What about manual fixup or squash?

(From a heavy git-rebase user :) )

Git needs a information preserving rebase

jrincayc — Mon, 16 Sep 2019 12:00:30 +0000

Side note: Git needs to by default preserve information during a rebase. As in just as each commit records the parents, patches produced by rebase should record the git id of the patches that were used for the rebase. This would allow automated tools to more easily figure out where a patch came from.

History of the history

marcH — Thu, 05 Sep 2019 09:33:20 +0000

> Any git project where feature branches are revised and shared over time could benefit from having links from one version of a commit to a previous one.

The tool laser-focused on the "history of the history" is https://github.com/git-series/git-series

Unlike email, all tools specialized for code review implement some form of linking between revisions in their database. While such linking is one of the most basic code review features, there are surprisingly big differences between the approaches.

Gerrit (obviously) supports this well for individual patches but doesn't treat series as a "first-class" citizens, patches in a series are just "related":
https://gerrit-review.googlesource.com/Documentation/user... Trying for instance to see how a series has been re-ordered is close to impossible with that user interface. I just fetch the two revisions and compare with git locally.

On the other hand Gerrit's ability to filter out rebase noise has become as impressive as git series': https://bugs.chromium.org/p/gerrit/issues/detail?id=217#c42

Github is probably the worse at this. Series are first-class citizens but the recommended workflow is to never rewrite anything and always add the equivalent of fixup! commits until the review is finished - except there's no way to mark which commit is a fixup for what. Force pushes are possible but break a number of features. More details and references at https://github.com/zephyrproject-rtos/zephyr/pull/14444

Gitlab follows github's model but seems to cope with force pushes much better.

Change IDs for kernel patches

marcH — Thu, 05 Sep 2019 08:54:56 +0000

> I find this an odd comment

It's not just odd, it's plain wrong.

Change-Id are being re-used across Gerrit instances and across branches and it works great.

Change IDs for kernel patches

neilbrown — Tue, 03 Sep 2019 23:49:39 +0000

> This is not a new practice; it appears to have first been used for this patch applied by H. Peter Anvin in 2011.

It is predated by the URL: tag, first used (that I can find) by Dominik Brodowski in 2005.

https://git.kernel.org/pub/scm/linux/kernel/git/history/h...

Cover letter

johill — Tue, 03 Sep 2019 07:44:35 +0000

Some maintainers will create a side-branch and merge it for a bigger series of patches, and put the cover letter into the merge commit message, which seems like a good way to capture this. Perhaps it should be done more often.

Change IDs for kernel patches

Fowl — Tue, 03 Sep 2019 07:14:35 +0000

> You don't really need it: it's highly likely that this particular commit would only exist on a developer's machine and would never be posted.

And this confusion is probably the flaw in the idea - once people read 'commit hash' they'll be assume it's a commit they can find. ;)

Change IDs for kernel patches

khim — Tue, 03 Sep 2019 06:50:44 +0000

> Couldn't a first/arbitrary comit hash become the 'change ID'?

You don't really need it: it's highly likely that this particular commit would only exist on a developer's machine and would never be posted.

Change-Id: used by Gerrit is not really created by Gerrit - that's just a random string which git presubmit hook adds to the description if it's not there.

This is done that way precisely because Android developers don't have a central Gerrit server: there are bazillion of them - each vendor have one, Google have half-dozen and so on.

Change-Id just allows you to do a search, nothing more, nothing less.

Change IDs for kernel patches

Fowl — Tue, 03 Sep 2019 04:25:42 +0000

> Given the ID is effectively a globally unique string

Indeed - I'm not sure why an actual GUID wouldn't work, they're shorter and don't require mailing anything. I guess they're a bit Microsoft-y.

Couldn't a first/arbitrary comit hash become the 'change ID'? Even if that exact commit never ends up in a tree it's a shortish, unique string that doesn't require any new tooling to generate.

Cover letter

meuh — Mon, 02 Sep 2019 21:13:26 +0000

I've used Link: tag inside commit messages to link those to the cover letter, as I found there's quite often the need to look at the patchset to understand a patch. The cover letter, and the patch series give more context, even in the presence of a "good" commit message.

Those links wouldn't be needed if the cover letter was included as commit message for a merge, but, first, merge commit almost only happen as a result of a pull request, second, it's usual for a patchset to go through different trees.

Also, I've made my best to provide a changelog for each patch series, with links to previous iterations. Having the Link: to the cover letter allows access to this changelog.

Anyway, one could wonder when Git would be able to record the patchset history, so that this history is not recorded in mail box, but in proper version control.

Change IDs for kernel patches

mathstuf — Mon, 02 Sep 2019 17:28:46 +0000

Hmm. I haven't been told to stop doing it yet (like I said, my submissions are rare). I feel like such scripts could easily be modified to only look at reroll count subject prefixes or only find patches under a given cover letter. *shrug* it's a thought. I don't maintain any kernel bits, but it's probably how I'd run it if I were to do so.

Change IDs for kernel patches

pbonzini — Sun, 01 Sep 2019 20:32:05 +0000

Most maintainers actually dislike that, because they (or their scripts) are used to having one thread per submission---so if you want to apply temporarily v(n-1) it would be harder to extract the appropriate patches out of a thread that includes multiple versions. Others' scripts might have different issues.

Change IDs for kernel patches

glasserc — Sun, 01 Sep 2019 15:08:54 +0000

This discussion is interesting, but I wish it didn't focus on the specific use case of code review for the Linux kernel. Any git project where feature branches are revised and shared over time could benefit from having links from one version of a commit to a previous one. I've often wished Git had something like Mercurial's "obsolesence markers" (from the "changeset evolution" extension). From my (non-kernel) perspective, the question of how these links are serialized into emails is not very important, but having them enables a bunch of other features.

Change IDs for kernel patches

jgg — Fri, 30 Aug 2019 15:26:48 +0000

I think my general complaint is that the built in git tooling for sending to a mailing list does not do nearly enough porcelain stuff to make the patch submission flow easy. I'm always sad when using git send email.

But there is some much potential here! Surely someone must have written nice wrapper scripts..

Change IDs for kernel patches

rgb — Fri, 30 Aug 2019 13:31:34 +0000

"Greg Kroah-Hartman described it as overly complex, though, and suggested simply posting patches as a reply to previous versions, but others pointed out that not all mailers would make that entirely easy to do."

I agree with Greg. That's what reply is there for, to provide context. Also reply is such a basic functionality I'm really curious what kind of mail program does not support reply?

I doubt that anyone can come up with an alternative to reply, that is as easy to use and adopt by most/all developers.

Change IDs for kernel patches

mathstuf — Fri, 30 Aug 2019 13:02:55 +0000

Hmm. How hard would it be to ask that subsequent submissions use `--in-reply-to=` the previous patchset (usually its cover letter)? That's what I do on my (admittedly rare) submissions. It keeps the patchsets together in mail viewers and provides context for the previous submission(s).

Change IDs for kernel patches

johill — Fri, 30 Aug 2019 12:01:25 +0000

And we use that, with some commit log mangling, to create the Link: in most cases.

Change IDs for kernel patches

wiktor — Fri, 30 Aug 2019 10:58:37 +0000

> The idea that Torvalds liked the best — and which seems to have the widest support across the community — is to use the unique ID that is already associated with a patch posting, which is the message ID created by the poster's email client

For the record `git am` already has `--message-id` option that can be used to insert back-reference to the patch message's ID: https://git-scm.com/docs/git-am#Documentation/git-am.txt-...

Change IDs for kernel patches

kleptog — Fri, 30 Aug 2019 09:21:44 +0000

> Linus Torvalds was quick to reject the idea of putting a bare change ID into patch changelogs, citing the same reasoning that has kept those IDs out thus far: they are really only useful to whoever put that ID into the changelog in the first place.

I find this an odd comment. Given the ID is effectively a globally unique string, simply throwing the Change-ID into $SEARCHENGINE will immediately return any and all mail conversations, bug trackers, etc referring to this patch. This seems much more useful that some email message ID, which is not easily searchable because it contains so many word-breaking characters and significant constant parts.

In any case, even in Gerrit you don't really use those Change-IDs to search for patches, each change has an actual URL too, but that doesn't appear in the commit while it's under discussion. The Change-ID is mostly useful after the fact to locate patches that are related.

Change IDs for kernel patches

diconico07 — Fri, 30 Aug 2019 06:44:49 +0000

For me it completely relates to what Konstantin Ryabitsev said in his blog post:
https://people.kernel.org/monsieuricon/patches-carved-int...

Change IDs for kernel patches

dianders — Fri, 30 Aug 2019 00:08:57 +0000

Nice summary! A further link for those interested in one solution that might be an OK compromise (if anyone adopts it):

https://patchwork.ozlabs.org/patch/1154833/

-Doug