llvm dev - Dec 2019 - Delete Phabricator metadata tags before committing

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.

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

On Fri, Dec 27, 2019 at 12:48 PM Fangrui Song via llvm-dev <
llvm-dev at 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?
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-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191227/8bbff6e2/attachment.html>

On Fri, Dec 27, 2019 at 12:48 PM Fangrui Song via llvm-dev <
llvm-dev at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191227/72d31a0c/attachment.html>

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

On Fri, Dec 27, 2019 at 10:30 PM Mehdi AMINI via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
>
>
> On Fri, Dec 27, 2019 at 12:48 PM Fangrui Song via llvm-dev <
> llvm-dev at 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
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191228/e00da4ae/attachment.html>

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.

On Fri, 27 Dec 2019 at 20:55, David Blaikie via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> 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).
>
> On Fri, Dec 27, 2019 at 12:48 PM Fangrui Song via llvm-dev <
> llvm-dev at 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?
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-dev at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200102/17d79e1d/attachment.html>

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

On Fri, Dec 27, 2019 at 11:30 PM Mehdi AMINI via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
>
>
> On Fri, Dec 27, 2019 at 12:48 PM Fangrui Song via llvm-dev <
> llvm-dev at 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
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200409/f19dafb6/attachment.html>