Page MenuHomeFreeBSD

prepare-commit-msg: add "Fixes:"
ClosedPublic

Authored by emaste on Feb 15 2021, 8:49 PM.
Tags
None
Referenced Files
F81998479: D28693.id84051.diff
Wed, Apr 24, 8:24 AM
F81954435: D28693.diff
Tue, Apr 23, 5:06 PM
Unknown Object (File)
Sat, Apr 20, 4:16 PM
Unknown Object (File)
Mar 20 2024, 4:55 AM
Unknown Object (File)
Mar 2 2024, 7:32 AM
Unknown Object (File)
Jan 17 2024, 12:56 AM
Unknown Object (File)
Jan 7 2024, 9:03 AM
Unknown Object (File)
Jan 6 2024, 6:36 PM
Subscribers

Details

Summary

A number of projects use "Fixes: <hash>" to identify a commit that is fixed by a given change. Adopt that convention.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

emaste created this revision.

This should be considered 'X-MFC-With' that some folks have used.

In D28693#642200, @imp wrote:

This should be considered 'X-MFC-With' that some folks have used.

I don't quite follow what you mean by "considered" here

It seems to me that X-MFC-WITH is also used for follow-up commits, not specifically for fixes.

For examples, see efd9d47, 39b7445, 5a3b950, and quite a lot of other examples from here.

A Fixes: field that's supplied by the commit hook also ensures that there's a consistent place to look for fixes, rather than in the description.

royger added a subscriber: royger.

Other projects I work with use the Fixes tag with the following format:

Fixes: <16 digit start hash> ('commit subject')

Which at least give you the subject line of the offending commit (not that it's much work to go and find it with git show). In either form I think it's a useful tag.

I usually do commits without MFC, so using X-MFC-With won't make sense there since I didn't plan to MFC the offending commit in the first place.

This revision is now accepted and ready to land.Feb 16 2021, 8:45 AM

Suggest short hash and title line as suggested by @royger

This revision now requires review to proceed.Feb 16 2021, 10:46 PM

We have both X-MFC-With and MFC with in the tree (the latter is slightly more popular); one nice property of Fixes is that it conveys useful information even in the absence of an intent to MFC, e.g. if a downstream project wants to cherry-pick the fix.

@mjg I see you've used Fixes: <shorthash> (<title line>) (with parens), is common elsewhere?

That's what they used to do in Linux, I don't know about other projects.

In general when referring to a commit they don't just paste hashes but also provide the one-liner summary which I consider to be drastically more readable.

Yes already added "title line" on @royger's suggestion, agreed that makes it much more useful.

The parentheses just seem to be making use of two of the 72 columns which we have until we need to add a newline.
Do we need them?

The parentheses just seem to be making use of two of the 72 columns which we have until we need to add a newline.
Do we need them?

Other projects I work with that use the Fixes tag don't apply line length rules to it, or to tags in general. I don't think we want to split the title for length reasons.

I don't have a strong opinion regarding the parentheses.

Other projects I work with that use the Fixes tag don't apply line length rules to it, or to tags in general. I don't think we want to split the title for length reasons.

I don't have a strong opinion regarding the parentheses.

Oh, it's very possible I misunderstood and might've thought the 72 column limit to commit headers applied to the whole thing, given that I have my editor configured to insert linebreaks at 72 columns. :)

Don't mind me folks, and don't let my comments hold this up. ;)

Other projects I work with that use the Fixes tag don't apply line length rules to it, or to tags in general. I don't think we want to split the title for length reasons.

Yeah, we would not want to split the title e.g. something like

Fixes:          ab4fad4be144 ("Add ifdef TCPHPTS around build_ack_entry 
                               and do_bpf_and_csum to avoid")

Scripts would probably be fine with this (as we'd likely just use a regex along the lines of fixes:[[:space:]]([0-9a-f]+) but we might as well avoid troubles with line splits.

I think it'd be fine to truncate

Fixes:          ab4fad4be144 ("Add ifdef TCPHPTS around build_ack_...")

or go beyond 72 cols

Fixes:          ab4fad4be144 ("Add ifdef TCPHPTS around build_ack_entry and do_bpf_and_csum to avoid")
This revision was not accepted when it landed; it landed in state Needs Review.Feb 22 2021, 3:32 PM
This revision was automatically updated to reflect the committed changes.