llvm dev - Jan 2020 - [EXTERNAL] Re: Delete Phabricator metadata tags before committing

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>

+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

From: llvm-dev <llvm-dev-bounces at lists.llvm.org> On Behalf Of James
Henderson via llvm-dev
Sent: Thursday, January 2, 2020 9:57 AM
To: David Blaikie <dblaikie at gmail.com>
Cc: llvm-dev <llvm-dev at lists.llvm.org>
Subject: [EXTERNAL] Re: [llvm-dev] Delete Phabricator metadata tags before
committing

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<mailto: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<mailto: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<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Freviews.llvm.org%2FDxxxxx&data=02%7C01%7Candya%40microsoft.com%7Cfa22dca30ad64a56b98508d78f940ec0%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637135738379643383&sdata=Lv4bK2YjVOoLGvjhQ9dIA8o%2FZUzZKajpigs5J13Aaqk%3D&reserved=0>

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<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fllvm.org%2Fdocs%2FDeveloperPolicy.html&data=02%7C01%7Candya%40microsoft.com%7Cfa22dca30ad64a56b98508d78f940ec0%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637135738379653343&sdata=zmn0lZLOQnbXm80S2tuHysCIOg9wwLp%2FI7D7csRkhm0%3D&reserved=0>
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<mailto:llvm-dev at lists.llvm.org>
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.llvm.org%2Fcgi-bin%2Fmailman%2Flistinfo%2Fllvm-dev&data=02%7C01%7Candya%40microsoft.com%7Cfa22dca30ad64a56b98508d78f940ec0%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637135738379653343&sdata=93vOsnMq%2BdoDlnqp1ToMUy30H9cWCzd4bM2VUIj5CR0%3D&reserved=0>
_______________________________________________
LLVM Developers mailing list
llvm-dev at lists.llvm.org<mailto:llvm-dev at lists.llvm.org>
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.llvm.org%2Fcgi-bin%2Fmailman%2Flistinfo%2Fllvm-dev&data=02%7C01%7Candya%40microsoft.com%7Cfa22dca30ad64a56b98508d78f940ec0%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637135738379653343&sdata=93vOsnMq%2BdoDlnqp1ToMUy30H9cWCzd4bM2VUIj5CR0%3D&reserved=0>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200102/a4ea7d5a/attachment-0001.html>

On 2020-01-02, Joseph Tremoulet via llvm-dev wrote:
>+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
>
>From: llvm-dev <llvm-dev-bounces at lists.llvm.org> On Behalf Of James
Henderson
>via llvm-dev
>Sent: Thursday, January 2, 2020 9:57 AM
>To: David Blaikie <dblaikie at gmail.com>
>Cc: llvm-dev <llvm-dev at lists.llvm.org>
>Subject: [EXTERNAL] Re: [llvm-dev] Delete Phabricator metadata tags before
>committing
>
>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.
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 -; }

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

On Thu, Jan 2, 2020 at 10:44 AM Joseph Tremoulet via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> +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
>
>
>
> *From:* llvm-dev <llvm-dev-bounces at lists.llvm.org> *On Behalf Of
*James
> Henderson via llvm-dev
> *Sent:* Thursday, January 2, 2020 9:57 AM
> *To:* David Blaikie <dblaikie at gmail.com>
> *Cc:* llvm-dev <llvm-dev at lists.llvm.org>
> *Subject:* [EXTERNAL] Re: [llvm-dev] Delete Phabricator metadata tags
> before committing
>
>
>
> 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
>
<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Freviews.llvm.org%2FDxxxxx&data=02%7C01%7Candya%40microsoft.com%7Cfa22dca30ad64a56b98508d78f940ec0%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637135738379643383&sdata=Lv4bK2YjVOoLGvjhQ9dIA8o%2FZUzZKajpigs5J13Aaqk%3D&reserved=0>
>
> 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
>
<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fllvm.org%2Fdocs%2FDeveloperPolicy.html&data=02%7C01%7Candya%40microsoft.com%7Cfa22dca30ad64a56b98508d78f940ec0%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637135738379653343&sdata=zmn0lZLOQnbXm80S2tuHysCIOg9wwLp%2FI7D7csRkhm0%3D&reserved=0>
> 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
>
<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.llvm.org%2Fcgi-bin%2Fmailman%2Flistinfo%2Fllvm-dev&data=02%7C01%7Candya%40microsoft.com%7Cfa22dca30ad64a56b98508d78f940ec0%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637135738379653343&sdata=93vOsnMq%2BdoDlnqp1ToMUy30H9cWCzd4bM2VUIj5CR0%3D&reserved=0>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.llvm.org%2Fcgi-bin%2Fmailman%2Flistinfo%2Fllvm-dev&data=02%7C01%7Candya%40microsoft.com%7Cfa22dca30ad64a56b98508d78f940ec0%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637135738379653343&sdata=93vOsnMq%2BdoDlnqp1ToMUy30H9cWCzd4bM2VUIj5CR0%3D&reserved=0>
>
> _______________________________________________
> 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/20200104/a218141e/attachment.html>