Many git commits in the monorepo look like the following:
[Tag0][Tag1] Title line
Summary:
Lorem ipsum dolor sit amet, consectetur adipiscing elit. Quisque mauris neque, porta nec tristique at, sagittis vel nisi. Fusce pharetra nunc et mauris consequat venenatis.
Reviewers: username0, username1
Reviewed By: username0
Subscribers: username2, username3, llvm-commits
These Phabricator metadata lines (`Reviewers:`, `Reviewed By:`, etc) are created automatically when the author uploads the patch via `arc diff 'HEAD^'`.
The summary and metadata can be copied from Phabricator to the local commit via `arc amend`.
Among the metadata tags, `Differential Revision:` is the most useful one. It associates a commit with a Phabricator differential (Dxxxxx) and allows the differential to be closed automatically when the commit is pushed to llvm-project master.
The other tags (Subscribers:, Reviewers:, Reviewed By:, Tags:, and the `Summary:` line) are not useful. They just clutter up the commit message. Shall we mention on LLVM Developer Policy — LLVM 16.0.0git documentation that developers are recommended to delete metadata tags before committing? It'd be nice if some pre-receive hooks can be set up to enforce deleting some really unnecessary metadata tags.
I don’t think they’re sufficiently problematic to worry about adding more steps people need to be aware of/follow to submit.
Maybe more advisory “you can remove the unneeded tags” might not hurt (but still adds to the length of new contributor documentation, etc, which isn’t free) - though “Reviewed By” is useful (in addition to “Differential Revision”) if it accurately reflects who reviewed/signed off on the change (makes it easy when I’m reading the mailing list to see who’s already looked a patch over, etc).
Many git commits in the monorepo look like the following:
[Tag0][Tag1] Title line
Summary:
Lorem ipsum dolor sit amet, consectetur adipiscing elit. Quisque mauris neque, porta nec tristique at, sagittis vel nisi. Fusce pharetra nunc et mauris consequat venenatis.
These Phabricator metadata lines (Reviewers:, Reviewed By:, etc) are created automatically when the author uploads the patch via arc diff 'HEAD^'.
The summary and metadata can be copied from Phabricator to the local commit via arc amend.
Among the metadata tags, Differential Revision: is the most useful one. It associates a commit with a Phabricator differential (Dxxxxx) and allows the differential to be closed automatically when the commit is pushed to llvm-project master.
The other tags (Subscribers:, Reviewers:, Reviewed By:, Tags:, and the Summary: line) are not useful. They just clutter up the commit message. Shall we mention on https://llvm.org/docs/DeveloperPolicy.html that developers are recommended to delete metadata tags before committing?
That’s be great!
I suspect we can provide a bash function to add to one’s .bashrc to make it trivial:
Maybe there’s a way to make an on-save filter that correctly detects that the file being saved is a commit message (bonus if it can detect that it’s a commit message for the llvm repo)?
I also find the “Reviewed by” tag useful (as well as the review link), for the same reasons. In fact, I don’t even use arcanist to push commits, so I do it all by hand, and only include the “Reviewed by” and “Differential Revision” tags.
+1 to keeping “Reviewed by”. Whenever I’m fixing a bug in unfamiliar code, I look at both author and “reviewed by” from the history of that code to help pick reviewers for my change.
The limitation with the “Reviewed by” line is that if you just use arc it indicates who was added as reviewer on the revision, but not who approved it. Because of this, I am wary of relying on this line for anything.
If you want to know who reviewed a change, better click on the Differential Revision link and go to the source of truth.
Yeah, I tend to prune it down myself - and if the list has only one name on it, it’s usually a pretty good indication they actually reviewed it. If it has many names, good chance it was just everyone someone put on the list and then I go check it manually.
I’m sure I’ve seen many commits with both “Reviewed by:” and “Reviewers:” tags, which look to have been done with arc (though I can’t be sure). How were those generated?
My own commits come through that way, with everyone I nominated listed in “reviewers” and then just those that replied in “reviewed by”. I generate the messages using “arc amend”.
I was always assuming that the suggested commit is assembled in the
PHP code run by arcanist command run locally. If indeed the arc
command requests the commit message from the server, we could do some
additional things:
* Remove "Summary:" in front of message
* Line break after 72 columns
* Convert summary markdown formatting to plain texts (e.g. remove
backticks; I don't know any git client that renders as markdown)
* Add/check existence of [component] tag
Alternatively, we could make an upstream feature request
I was always assuming that the suggested commit is assembled in the
PHP code run by arcanist command run locally. If indeed the arc
command requests the commit message from the server,
I assumed so too until I went digging for it. Seems the client-side stuff only deals with the structured data, and calls out to the server to construct the raw text when needed:
I was always assuming that the suggested commit is assembled in the
PHP code run by arcanist command run locally. If indeed the arc
command requests the commit message from the server, we could do some
additional things:
Remove “Summary:” in front of message
Line break after 72 columns
Convert summary markdown formatting to plain texts (e.g. remove
backticks; I don’t know any git client that renders as markdown)
-1 on removing backticks. The lowercase in the middle of a sentence can be confusing without the backticks.
>> I was always assuming that the suggested commit is assembled in the
>> PHP code run by arcanist command run locally. If indeed the arc
>> command requests the commit message from the server, we could do some
>> additional things:
>>
>> * Remove "Summary:" in front of message
>> * Line break after 72 columns
>> * Convert summary markdown formatting to plain texts (e.g. remove
>> backticks; I don't know any git client that renders as markdown)
>>
> -1 on removing backticks. The `lowercase` in the middle of a sentence can
> be confusing without the backticks.
TBH, I don't need markdown to be rendered to make sense to me. If I see
backticks I know that code or commands are meant here, which can
disambiguate common words from variables nicely. I also don't see how
`backticks`, *asterisks*, or __underlines__ hurt the text flow. Maybe it
is just me, since I'm used to read/wrie markdown in vim, but I find they
highlight words just as well in plain text.
I do think we could automate the process in general though. I regularly
forget to run my `arcfilter` command (adopted from Fangrui Song I think):
and for now it also breaks single line commit messages (not subjects).
Cheers,
Johannes
>> * Add/check existence of [component] tag
>>
>> Alternatively, we could make an upstream feature request
>>
>> Michael
>>
>>>
>>> Can we fix this in reviews.llvm.org's fork of phab?
>>>
>> https://github.com/phacility/phabricator/blob/cac3dc4983c3671ba4ec841aac8efac10744a80c/src/applications/differential/conduit/DifferentialGetCommitMessageConduitAPIMethod.php
>>>
>>> Seems straightforward(-ish) to drop the relevant fields there, that way
>> `arc land` automatically DTRT.
>>>
>>> Jon
>>>
>>>>
>>>>>
>>>>> Many git commits in the monorepo look like the following:
>>>>>
>>>>> [Tag0][Tag1] Title line
>>>>>
>>>>> Summary:
>>>>> Lorem ipsum dolor sit amet, consectetur adipiscing elit. Quisque
>> mauris neque, porta nec tristique at, sagittis vel nisi. Fusce pharetra
>> nunc et mauris consequat venenatis.
>>>>>
>>>>> Reviewers: username0, username1
>>>>>
>>>>> Reviewed By: username0
>>>>>
>>>>> Subscribers: username2, username3, llvm-commits
>>>>>
>>>>> Tags: LLVM Project
>>>>>
>>>>> Differential Revision: Login
>>>>>
>>>>> These Phabricator metadata lines (`Reviewers:`, `Reviewed By:`, etc)
>> are created automatically when the author uploads the patch via `arc diff
>> 'HEAD^'`.
>>>>> The summary and metadata can be copied from Phabricator to the local
>> commit via `arc amend`.
>>>>>
>>>>> Among the metadata tags, `Differential Revision:` is the most useful
>> one. It associates a commit with a Phabricator differential (Dxxxxx) and
>> allows the differential to be closed automatically when the commit is
>> pushed to llvm-project master.
>>>>>
>>>>> The other tags (Subscribers:, Reviewers:, Reviewed By:, Tags:, and the
>> `Summary:` line) are not useful. They just clutter up the commit message.
>> Shall we mention on LLVM Developer Policy — LLVM 18.0.0git documentation that
>> developers are recommended to delete metadata tags before committing?
>>>>
>>>> That's be great!
>>>> I suspect we can provide a bash function to add to one's .bashrc to
>> make it trivial:
>>>>
>>>> function arcfilter() { git log -1 --pretty=%B | awk '/Reviewers:
>> /{p=1; sub(/Reviewers: .*Differential Revision: /, "")}; /Differential
>> Revision: /{p=0;}; !p' | git commit --amend -F - ; }
>>>>
>>>> Just running `arcfilter` before pushing will filter the commit it out
>> automatically!
>>>>
>>>>>
>>>>> It'd be nice if some pre-receive hooks can be set up to enforce
>> deleting some really unnecessary metadata tags.
>>>>
>>>> Unfortunately you can't add pre-receive hook on GitHub as far as I know.
FYI: I patched our Phab instance and now arc land will omit “Subscribers”, “Reviewers”, and “Tags”.
It’ll also print the description without prefixing with “Summary:”.
I was always assuming that the suggested commit is assembled in the
PHP code run by arcanist command run locally. If indeed the arc
command requests the commit message from the server,
I assumed so too until I went digging for it. Seems the client-side stuff only deals with the structured data, and calls out to the server to construct the raw text when needed:
FYI: I patched our Phab instance and now `arc land` will omit "Subscribers", "Reviewers", and "Tags".
It'll also print the description without prefixing with "Summary:".