James Henderson via llvm-dev
2020-Jan-02 14:57 UTC
[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. >> _______________________________________________ >> 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>
Joseph Tremoulet via llvm-dev
2020-Jan-02 15:44 UTC
[llvm-dev] [EXTERNAL] Re: Delete Phabricator metadata tags before committing
+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>
Fangrui Song via llvm-dev
2020-Jan-02 19:11 UTC
[llvm-dev] [EXTERNAL] Re: Delete Phabricator metadata tags before committing
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 -; }
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>