[llvm-dev] Delete Phabricator metadata tags before committing

[Fangrui Song via 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.
_______________________________________________
LLVM Developers mailing list
llvm...@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

[David Blaikie via llvm-dev]

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).

[Mehdi AMINI via llvm-dev]

On Fri, Dec 27, 2019 at 12:48 PM Fangrui Song via llvm-dev <llvm...@lists.llvm.org> wrote:

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?

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.

-- 

Mehd

[David Blaikie via llvm-dev]

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)?

[James Henderson via llvm-dev]

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.

[Joseph Tremoulet via llvm-dev]

+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

[Fangrui Song via llvm-dev]

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 -; }

[Mehdi AMINI via llvm-dev]

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.

-- 

Mehdi

[David Blaikie via llvm-dev]

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.

[James Henderson via llvm-dev]

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?

[Joseph Tremoulet via llvm-dev]

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”.

[Alex Zinenko via llvm-dev]

If somebody wants to make sure they don't push commits with extra tags, it's possible to set up a local pre-push hook that would complain.

In /path/to/llvm-project/.git/hooks/pre-push

paste the following

#!/bin/sh

# Disallow pushing commits that contain in their description the Phabricator
# tags that are unwelcome in the upstream LLVM repository.
#
# This hook is called with the following parameters:
#
# $1 -- Name of the remote to which the push is being done
# $2 -- URL to which the push is being done
#
# If pushing without using a named remote those arguments will be equal.
#
# Information about the commits which are being pushed is supplied as lines to
# the standard input in the form:
#
#   <local ref> <local sha1> <remote ref> <remote sha1>
#

# This SHA will be used if the commit is deleted on either side.
z40=0000000000000000000000000000000000000000

while read local_ref local_sha remote_ref remote_sha; do
    if [ "$local_sha" != "$z40" ]; then
        if [ "$remote_sha" = $z40 ]; then
            # New branch, examine all commits
            range="$local_sha"
        else
            # Update to existing branch, examine new commits
            range="$remote_sha..$local_sha"
        fi

        # Check for Phabricator tags and stop if found.
        commit=`git rev-list -n 1 \
          --grep '^Subscribers:' \
          --grep '^Tags:' \
          --grep '^Reviewers:' \
          "$range"`
        if [ -n "$commit" ]; then
            echo >&2 "Found Phabricator tags in $commit, push aborted."
            echo >&2 "Please remove the lines starting with Subscribers, Tags,"
            echo >&2 "Reviewers from the commit messages before pushing."
            exit 1
        fi
    fi
done

exit 0

and don't forget to chmod +x on the file.

This will report the hash of the commit about to be pushed if it contains "Subscribers", "Tags" or "Reviewers" and abort the push.

--

Alex

[Jon Roelofs via llvm-dev]

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

[Michael Kruse via llvm-dev]

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

Michael

Am Do., 9. Apr. 2020 um 11:16 Uhr schrieb Jon Roelofs via llvm-dev
<llvm...@lists.llvm.org>:

[Jon Roelofs via llvm-dev]

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:

https://github.com/phacility/arcanist/blob/33b9728b5f65fd747b7a15c0e25436c63e82f592/src/workflow/ArcanistDiffWorkflow.php#L757

 

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

[Hubert Tong via llvm-dev]

On Thu, Apr 9, 2020 at 12:29 PM Michael Kruse via llvm-dev <llvm...@lists.llvm.org> 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, 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.

[Johannes Doerfert via llvm-dev]

On 4/9/20 11:47 AM, Hubert Tong via llvm-dev wrote:
> On Thu, Apr 9, 2020 at 12:29 PM Michael Kruse via llvm-dev <
> llvm...@lists.llvm.org> 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, 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):

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

[Mehdi AMINI via llvm-dev]

On Thu, Apr 9, 2020 at 9:45 AM Jon Roelofs <jroe...@gmail.com> wrote:

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:

https://github.com/phacility/arcanist/blob/33b9728b5f65fd747b7a15c0e25436c63e82f592/src/workflow/ArcanistDiffWorkflow.php#L757

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:".

-- 

Mehdi

[Fāng-ruì Sòng via llvm-dev]

On Mon, Jul 27, 2020 at 10:26 PM Mehdi AMINI via llvm-dev
<llvm...@lists.llvm.org> wrote:
>
>
>
> On Thu, Apr 9, 2020 at 9:45 AM Jon Roelofs <jroe...@gmail.com> wrote:
>>
>>
>>
>> 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:
>>
>> https://github.com/phacility/arcanist/blob/33b9728b5f65fd747b7a15c0e25436c63e82f592/src/workflow/ArcanistDiffWorkflow.php#L757
>
>
>
> 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:".
>
> --
> Mehdi

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 Foad via llvm-dev]

I have just noticed this happening. Thank you, thank you, thank you!

Jay.

[Hiroshi Yamauchi via llvm-dev]

+1, I like this, too.