git: Use dashes rather than spaces in trailers

[emaste]

Summary

In the broader development community commit message trailers typically
use dashes.  Follow suit for better integration with other tooling.

Phabricator requires a space in "Differential Revision" so leave it
as is for now.

Sponsored by:   The FreeBSD Foundation

[imp]

Now all we need is commit hooks...

[emaste]

steps need to include (at least)

• srcmgr/core signoff
• send heads-up to appropriate lists
• update commit hooks, MFC reminder, MFC tracker to accept both forms
• update template (this change) and the committer's guide https://docs.freebsd.org/en/articles/committers-guide/#_include_appropriate_metadata_in_a_footer

[lwhsu]

For PR: as it's not changed so I think there is no affect for the hooks for notifying Bugzilla and mailing lists (dev-commits-*). But for MFC-after I suggest let's check if https://github.com/gonzoua/mfctracker and https://github.com/sobomax/mfc_notifications can handle it.

[lwhsu]

Oh, "Approved-by" needs pre-commit hook change.

[emaste]

@kevans has a pull request open to add - support to the MFC notification service https://github.com/sobomax/mfc_notifications/pull/3

[emaste]

mfctracker already matches dashes re.match('^\s*mfc(-|\s+)after\s*:', line, flags=re.IGNORECASE)

https://github.com/gonzoua/mfctracker/blob/77800ee5ab17e4efefb12bfbd547bff1ed94c6a1/mfctracker/management/commands/importcommits.py#L181

[kevans]

In D40025#911513, @emaste wrote:

mfctracker already matches dashes re.match('^\s*mfc(-|\s+)after\s*:', line, flags=re.IGNORECASE)

https://github.com/gonzoua/mfctracker/blob/77800ee5ab17e4efefb12bfbd547bff1ed94c6a1/mfctracker/management/commands/importcommits.py#L181

yeah, I did both of those at the same time... a bit unusual that @sobomax never picked that one up, actually

[lwhsu]

In D40025#911505, @lwhsu wrote:

Oh, "Approved-by" needs pre-commit hook change.

This has been updated and deployed.

[jhb]

While I prefer the tab alignment, the style in the broader community that uses dashes is to not do the tabs either, and in particular that would be compatible with using git commit --trailer

[imp]

In D40025#911589, @jhb wrote:

While I prefer the tab alignment, the style in the broader community that uses dashes is to not do the tabs either, and in particular that would be compatible with using git commit --trailer

I concur... having written tools that tried to align things, it's a bit of a pain to do once, but adds an ongoing burden for each new tool created. The --trailer functionality makes it so much easier to do since it gets a lot of edge cases right for free that are likewise an ongoing tax.

[dvl]

I've checked the FreshPorts code. For the CVS commits, these were parsed. Interesting... because:

[0:58 dev-ingress01 dan ~/scripts] % egrep -r 'Reported by|Reviewed by|Tested by|Approved by|Obtained from|MFC after|Sponsored by|Pull Request' ~/scripts ~/modules
/usr/home/dan/modules/process_cvs_mail.pm:		if ($line =~ /^  Obtained from:/i) {
/usr/home/dan/modules/process_cvs_mail.pm:		if ($line =~ /^  Approved by:/i) {
/usr/home/dan/modules/process_cvs_mail.pm:		if ($line =~ /^  Reviewed by:/i) {
/usr/home/dan/modules/process_svn_mail.pm:		if ($line =~ /^  Obtained from:/i) {
/usr/home/dan/modules/process_svn_mail.pm:		if ($line =~ /^  Approved by:/i) {
/usr/home/dan/modules/process_svn_mail.pm:		if ($line =~ /^  Reviewed by:/i) {

... I can't find any proof that it did anything with it. :) But I've stopped looking.

[imp]

Also my comments about commi hooks would be to deny the old forms on main

[jrm]

tools/tools/git/hooks/prepare-commit-msg

59

Do we want to match the format in the comment?

For the other fields with a dash, we use Uppercase-lowercase, so should we use Pull-request:? Did we keep the space in Differential Revision: intentionally instead of using Differential-revision:?

[emaste]

tools/tools/git/hooks/prepare-commit-msg

59

Yes this should be Pull-Request (or pull-request). Also Differential-Revision but we'll need to update Phabricator to search for that as well.

[imp]

tools/tools/git/hooks/prepare-commit-msg

59

Do we have docs for this one?

[emaste]

Make case consistent and add - in all uses of Pull-request

[emaste]

Also my comments about commi hooks would be to deny the old forms on main

Agreed

[sobomax]

In D40025#911506, @emaste wrote:

@kevans has a pull request open to add - support to the MFC notification service https://github.com/sobomax/mfc_notifications/pull/3

Thanks, merged in.

-Max

[sobomax]

In D40025#911514, @kevans wrote:

In D40025#911513, @emaste wrote:

mfctracker already matches dashes re.match('^\s*mfc(-|\s+)after\s*:', line, flags=re.IGNORECASE)

https://github.com/gonzoua/mfctracker/blob/77800ee5ab17e4efefb12bfbd547bff1ed94c6a1/mfctracker/management/commands/importcommits.py#L181

yeah, I did both of those at the same time... a bit unusual that @sobomax never picked that one up, actually

Sorry got bit busy with work here lately...

[grahamperrin]

Where horizontal tabs are used, is this essential? Use of simple spaces, in lieu of tabs, will allow neatness (no raggedness).

tools/tools/git/hooks/prepare-commit-msg

47

51

Without the additional horizontal tab for this line: when all lines are used, there's raggedness as shown below.

PR:             <If and which Problem Report is related.>
Reported-by:    <If someone else reported the issue.>
Reviewed-by:    <If someone else reviewed your modification.>
Tested-by:      <If someone else tested the change.>
Approved-by:    <If you needed approval for this commit.>
Obtained-from:  <If the change is from a third party.>    
Fixes:  <Short hash and title line of commit fixed by this change>
MFC-after:      <N [day[s]|week[s]|month[s]].  Request a reminder email>
Relnotes:       <Set to 'yes' for mention in release notes.>
Security:       <Vulnerability reference (one per line) or description.>
Sponsored-by:   <If the change was sponsored by an organization.>
Pull-request:   <https://github.com/freebsd/<repo>/pull/###>
Differential Revision:  <https://reviews.freebsd.org/D###>

For consistency, add a full stop.

52

For consistency, a full stop.

53

56–59

Lowercase r for request, so consider lowercase r for revision.

Observe the 72-column maximum for commentary. Not strictly necessary, but it can help.

60–61

The phrase someone else is used three times above, we can use the same phrase here.

[emaste]

tools/tools/git/hooks/prepare-commit-msg

56–59

Differential Revision: is what Phabricator wants to see. We can see about updating Phabricator to be more permissive.

[grahamperrin]

tools/tools/git/hooks/prepare-commit-msg

56–59

Thanks.

For what it's worth, I have written Differential revision: (lowercase r) for as long as I can remember.

As far as I can tell, lowercase r is good for (at least) closure purposes. https://reviews.freebsd.org/D39480#899087, for example.

[grahamperrin]

In addition to the trailers above:

• is Closes: effective, for pull requests?

[imp]

In D40025#934480, @grahamperrin wrote:

In addition to the trailers above:

• is Closes: effective, for pull requests?

Not in my experiments. Try it on the doc repo, but on a pull request I created on a fork, it didn't automatically close...

[grahamperrin]

In D40025#934481, @imp wrote:

… Try it on the doc repo, …

Success:

Closed #226 via 1037414.pdf14 KBDownload

https://codeberg.org/FreeBSD/freebsd-doc/commit/10374142a5202ea4a28cdabba82407ba87dbf042

I imagine Pull-request: being superfluous to Closes:, however people might be disconcerted by an absence of Pull-request: (or Pull Request: or Pull request:) during a transition period.

[imp]

But does it have the full url? We commit pull requests from many places and with out that 245 is meaningless. Also, many pull requests have several parts that aren't merged before landing (because they shouldn't be). I'm skeptical

[grahamperrin]

In D40025#934585, @imp wrote:

But does it have the full url? …

Maybe crossed paths with the 16:56 (?) edition of my previous comment, where I added a Codeberg view of the commit to show the full URL ☑

@salvadore in partial reply to your email (thanks), the successful test related to my curiosity at https://reviews.freebsd.org/D40025#934480 above.

---

Also for what it's worth, I have not encountered any failure with use of:

…

[emaste]

In D40025#934659, @grahamperrin wrote:

Also for what it's worth, I have not encountered any failure with use of:

Differential-revision:

• hyphenated
• lowercase r

Is the claim that Phabricator will detect & close a review with Differential-revision:? Do you have a link to an example of such a commit?

[grahamperrin]

In D40025#942450, @emaste wrote:

Is the claim that Phabricator will detect & close a review with Differential-revision:? …

My mistake.

As far as I can tell:

• Differential revision: (lowercase r) results in automated closure
• Differential-revision: (hyphenated) does not.

https://reviews.freebsd.org/D41283#940020 is an example of a mention, but I guess that the commit hash was recognised.

[grahamperrin]

Knowing that Closes: is effective for pull requests in e.g. the github.com/freebsd/freebsd-doc area (see, for example, https://codeberg.org/FreeBSD/freebsd-doc/commit/10374142a5202ea4a28cdabba82407ba87dbf042) …

… can Closes: be equally effective – the automation – where the requirement is specifically to not use a freebsd area for a FreeBSD-specific PR?