Page MenuHomeFreeBSD

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

Authored by emaste on Feb 15 2021, 8:49 PM.

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

Lint
Lint Skipped
Unit
Unit Tests Skipped

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.