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

Mon Jan 6 07:18:27 PST 2020

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

From: James Henderson <jh7370.2008 at my.bristol.ac.uk>
Sent: Monday, January 6, 2020 4:52 AM
To: David Blaikie <dblaikie at gmail.com>
Cc: Mehdi AMINI <joker.eph at gmail.com>; Joseph Tremoulet <jotrem at microsoft.com>; llvm-dev <llvm-dev at lists.llvm.org>
Subject: Re: [llvm-dev] [EXTERNAL] Re: Delete Phabricator metadata tags before committing

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<mailto: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<mailto: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<mailto: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<mailto: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<mailto:dblaikie at gmail.com>>
Cc: llvm-dev <llvm-dev at lists.llvm.org<mailto: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%7Cjotrem%40microsoft.com%7C5cbabf2f837b4d6ca1cb08d7928e1285%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637139011210477226&sdata=ytxz8oxg8MTVu5BhcMLtuAlXq71xVH%2BbDiefGurG70c%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%7Cjotrem%40microsoft.com%7C5cbabf2f837b4d6ca1cb08d7928e1285%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637139011210487186&sdata=imFMd8CPhPcKasrQvedqtvPa7NI2kn5CjkZ8ilg8kEU%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%7Cjotrem%40microsoft.com%7C5cbabf2f837b4d6ca1cb08d7928e1285%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637139011210497143&sdata=DXTMUQPlQsIomb6huyOgqhaktFqvdAS3AmckRaF7j7o%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%7Cjotrem%40microsoft.com%7C5cbabf2f837b4d6ca1cb08d7928e1285%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637139011210497143&sdata=DXTMUQPlQsIomb6huyOgqhaktFqvdAS3AmckRaF7j7o%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%7Cjotrem%40microsoft.com%7C5cbabf2f837b4d6ca1cb08d7928e1285%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637139011210497143&sdata=DXTMUQPlQsIomb6huyOgqhaktFqvdAS3AmckRaF7j7o%3D&reserved=0>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200106/43b913d0/attachment-0001.html>