Mehdi AMINI via llvm-dev
2020-Jan-04 15:44 UTC
[llvm-dev] [EXTERNAL] Re: Delete Phabricator metadata tags before committing
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>
David Blaikie via llvm-dev
2020-Jan-04 19:12 UTC
[llvm-dev] [EXTERNAL] Re: Delete Phabricator metadata tags before committing
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/20200104/7dc66f14/attachment.html>
James Henderson via llvm-dev
2020-Jan-06 09:51 UTC
[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> 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>