Page MenuHomeFreeBSD

tools/git: Add a script which can process fixup tags
Needs ReviewPublic

Authored by markj on Feb 14 2025, 8:09 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 10, 9:23 PM
Unknown Object (File)
Mon, Mar 10, 5:20 AM
Unknown Object (File)
Sat, Mar 8, 5:57 AM
Unknown Object (File)
Fri, Mar 7, 1:38 AM
Unknown Object (File)
Thu, Mar 6, 8:42 PM
Unknown Object (File)
Tue, Mar 4, 1:22 AM
Unknown Object (File)
Thu, Feb 27, 5:16 PM
Unknown Object (File)
Feb 19 2025, 12:35 AM
Subscribers

Details

Summary

When MFCing a set of commits, I want to know whether there are any fixup
commits that reference a commit that I'm MFCing. This script provides a
way to do that. In particular, given one or more revisions as
arguments, it finds the closure of commits which fix up the original
revisions. All it does is print them to stdout, so one can run
something like

$ git cherry-pick -x $(mfc-helper 9a4131629bb3083ddc02a32950e4eb4806a07710~..3fa552149885766b009d95d20bdf651786fac7b7)

and be confident that I picked up any fixups. Of course, this assumes
that developers are tagging their commits properly, but a growing number
are doing this. In the future we could alternately annotate such
information with git notes.

It's also useful to know whether there are any fixup commits missing
from a stable branch. When run in a stable branch checkout without any
arguments, this script finds all cherry-picked commits that have
un-cherry-picked fixups in an upstream branch (anon/main by default).
Right now there's a number of such commits on both stable/13 and 14.
Some are false positives (e.g., because developers squashed multiple
MFCs into a single commit for some reason), and some are unimportant,
but there are some real missed fixups.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 62501
Build 59385: arc lint + arc unit

Event Timeline

markj requested review of this revision.Feb 14 2025, 8:09 PM
markj created this revision.

Handle -l when looking for dangling commits.

I'm interested in comments on the UI as well as the implementation. Really this should probably be two scripts, but they both need to share some code, and I'm not sure how best to package this, so for a POC it's easier to just keep everything in one file.

This is something very useful, I'll play with it a few more days. Thank you!

tools/tools/git/mfc-helper
2

I have also been instructed in the past to lint with:

black --line-length 79 tools/tools/git/mfc-helper.py
29

Should a requirements.txt file containing gitpython (devel/py-gitpython) or an explanation that this package is required?

48

This dictionary should also get pruned over time, once the malformed/typoed/incorrect tag is already in the oldest currently supported stable branch?

181

Some are false positives (e.g., because developers squashed multiple MFCs into a single commit for some reason

I am guilty of this, it is just:
commit A <some old commit>
commit B <Fix commit A, but breaks the build>
commit C <Fix commit B>
When MFCing, I do not want to just cherry-pick a commit that I know is wrong. In this case I would squash B and C, and discard C's message (but leave the cherry picked from commit C part) because I want to atomically commit the Fix for A.
I could see lastline becoming the last -n lines matching the RE. But I also imagine you prefer a 1:1 mapping, so I'll stop squashing cherry-picks.

kevans added inline comments.
tools/tools/git/mfc-helper
181

I also have a number of 1:N for the same reason; I don't think we'd ever established a policy that 1:1 was required, as long as things were annotated accurately.

tools/tools/git/mfc-helper
29

My expectation was that this script would eventually be included in the freebsd-git-devtools package, which would be updated to carry a dependency on py-gitpython, so it wouldn't matter too much.

I guess requirements.txt would help if one wants to use pip instead? I'm pretty unfamiliar with python packaging so am not sure what's best here. I could certainly move this script into its own dir and add a requirements.txt.

48

Yeah, probably. It's fairly cheap to maintain though.

181

Hmm, ok. It's reasonable to MFC that way I suppose. I think I can update this script to cope.

In terms of UI, I just wasn't sure if you wanted to this be the start of a git mfc script. I can imagine something that is like git mfc <hash> [hash ...] and then maybe a git mfc --fixes <hash> [hash ...] that is a wrapper around doing a bunch of git cherry-pick -x commands. I could even see a use case for the 1:N merges people referenced earlier of git mfc --fixes --squash <hash> that would cherry-pick -x all the changes and then pop up an editor to let you squash the commit logs down similar to if you had cherry-picked them all individually and then did a git rebase where all the fixes commits were squashes.

tools/tools/git/mfc-helper
24

freebsd/main would be more consistent with how we document developer setups

181

I would be tempted to say that you can look in the last set of lines after a blank line, but even then, it's permissible for a cherry-pick to have its own trailers if someone requested review of a MFC then you end up with:

original title

original body

Reviewed by: foo

(cherry picked from commit XYZ)

Reviewed by: bar

so I think you just want to return a list of all matching commits

One other possible use for this functionality might be FreeBSD downstreams: if you're tracking main and are somewhat behind, you might want to enumerate all unmerged commits that fix up a merged commit, and merge those. That way, if you're preparing to cut a release, you get more confidence that you're not releasing with known bugs. Of course, this is far from foolproof, but it's better than nothing.

It's a bit hard to build that into this script since different downstreams might have different ways of merging FreeBSD commits in.

In D49013#1118074, @jhb wrote:

In terms of UI, I just wasn't sure if you wanted to this be the start of a git mfc script. I can imagine something that is like git mfc <hash> [hash ...] and then maybe a git mfc --fixes <hash> [hash ...] that is a wrapper around doing a bunch of git cherry-pick -x commands. I could even see a use case for the 1:N merges people referenced earlier of git mfc --fixes --squash <hash> that would cherry-pick -x all the changes and then pop up an editor to let you squash the commit logs down similar to if you had cherry-picked them all individually and then did a git rebase where all the fixes commits were squashes.

Yeah, I was thinking of having git mfc <rev> be a script which effectively cherry-picks the output of mfc-helper <rev> into the current branch.

I don't feel too strongly about squashing vs. preserving 1-1 commits with the main branch. git cherry-pick -x means that users won't get a chance to edit the log, which makes it less likely they'll accidentally strip important metadata. I also just find it easier that way.

In any case, the script now looks for all strings of the form "(cherry-picked from ..." and not just the last line.

tools/tools/git/mfc-helper
29

If it is going to be included as a package, there should be no need.
The idea of a comment could be useful for those developing on non-FreeBSD systems. But it is not a big deal.

On a somewhat related issue, I started thinking about the possibility of adding an --audit option to the cherry-picked (MFCd) commits. That is, to compare the diffs (original commit vs, cherry-picked commit), but it is not so straightforward sometimes, it would be easier with a one-to-one commit history.
(DISCLAIMER: Not that I doubt or have reasons to believe that some tampering has been done while cherry-picking, it was just out of curiosity, and for compliance reasons).

On a somewhat related issue, I started thinking about the possibility of adding an --audit option to the cherry-picked (MFCd) commits. That is, to compare the diffs (original commit vs, cherry-picked commit), but it is not so straightforward sometimes, it would be easier with a one-to-one commit history.
(DISCLAIMER: Not that I doubt or have reasons to believe that some tampering has been done while cherry-picking, it was just out of curiosity, and for compliance reasons).

You can use git cherry to find all the commits on a stable/N branch that aren't exact duplicates of a commit in main for auditing purposes.