[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
Differential Revision: https://reviews.llvm.org/Dxxxxx
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? It'd be nice if some pre-receive hooks can be set up to enforce deleting some really unnecessary metadata tags.
_______________________________________________
LLVM Developers mailing list
llvm...@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
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
Differential Revision: https://reviews.llvm.org/Dxxxxx
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?
It'd be nice if some pre-receive hooks can be set up to enforce deleting some really unnecessary metadata 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.
-Joseph
I kept `Reviewed By:` before and hestitated whether I should continue
this practice. Nice to see more rationale:)
I am now using a variant of Mehdi's shell function to keep Reviewed By: and Differential Revision:
function arcfilter() { git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} /Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:/' | git commit --amend -F -; }
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”.
* 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
Michael
Am Do., 9. Apr. 2020 um 11:16 Uhr schrieb Jon Roelofs via llvm-dev
<llvm...@lists.llvm.org>:
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, 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)
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):
arcfilter () { arc amend; git log -1 --pretty=%B | awk
'/Reviewers:|Subscribers:/{p=1} /Reviewed By:|Differential
Revision:/{p=0} !p && !/^Summary:/' | git commit --amend -F - }
and for now it also breaks single line commit messages (not subjects).
Cheers,
Johannes
On Thu, Apr 9, 2020 at 10:29 AM Michael Kruse <llv...@meinersbur.de> wrote: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:
Awesome!
>
>>
>>
>>
>>>
>>> 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
>>
>>
>> From what I can tell, upstream doesn't seem /that/ interested in it being any more than a one-off thing:
>>
>> https://secure.phabricator.com/Q268
>> https://secure.phabricator.com/T12276
>> https://secure.phabricator.com/T11864
>>
>> Jon
>>
>>>
>>> Michael
>>>
>>> Am Do., 9. Apr. 2020 um 11:16 Uhr schrieb Jon Roelofs via llvm-dev
>>> <llvm...@lists.llvm.org>:
>>> >
>>> > 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
>
> _______________________________________________
> LLVM Developers mailing list
> llvm...@lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
--
宋方睿
Jay.