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

Mon Jan 6 01:51:47 PST 2020

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?

On Sat, 4 Jan 2020 at 19:12, David Blaikie <dblaikie at gmail.com> wrote:

> 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.
>
> On Sat, Jan 4, 2020 at 7:45 AM Mehdi AMINI <joker.eph at gmail.com> wrote:
>
>> 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/20200106/2013a2f0/attachment.html>