Jon Roelofs via llvm-dev
2020-Apr-09 16:15 UTC
[llvm-dev] Delete Phabricator metadata tags before committing
Can we fix this in reviews.llvm.org's fork of phab? https://github.com/phacility/phabricator/blob/cac3dc4983c3671ba4ec841aac8efac10744a80c/src/applications/differential/conduit/DifferentialGetCommitMessageConduitAPIMethod.php Seems straightforward(-ish) to drop the relevant fields there, that way `arc land` automatically DTRT. Jon On Fri, Dec 27, 2019 at 11:30 PM Mehdi AMINI via llvm-dev < llvm-dev at lists.llvm.org> wrote:> > > 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? > > > That's be great! > I suspect we can provide a bash function to add to one's .bashrc to make > it trivial: > > function arcfilter() { git log -1 --pretty=%B | awk '/Reviewers: /{p=1; > sub(/Reviewers: .*Differential Revision: /, "")}; /Differential Revision: > /{p=0;}; !p' | git commit --amend -F - ; } > > Just running `arcfilter` before pushing will filter the commit it out > automatically! > > >> It'd be nice if some pre-receive hooks can be set up to enforce deleting >> some really unnecessary metadata tags. > > > Unfortunately you can't add pre-receive hook on GitHub as far as I know. > > -- > Mehd > > _______________________________________________ > 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/20200409/f19dafb6/attachment.html>
Michael Kruse via llvm-dev
2020-Apr-09 16:28 UTC
[llvm-dev] Delete Phabricator metadata tags before committing
I was always assuming that the suggested commit is assembled in the PHP code run by arcanist command run locally. If indeed the arc command requests the commit message from the server, we could do some additional things: * Remove "Summary:" in front of message * Line break after 72 columns * Convert summary markdown formatting to plain texts (e.g. remove backticks; I don't know any git client that renders as markdown) * Add/check existence of [component] tag Alternatively, we could make an upstream feature request Michael Am Do., 9. Apr. 2020 um 11:16 Uhr schrieb Jon Roelofs via llvm-dev <llvm-dev at lists.llvm.org>:> > Can we fix this in reviews.llvm.org's fork of phab? > > https://github.com/phacility/phabricator/blob/cac3dc4983c3671ba4ec841aac8efac10744a80c/src/applications/differential/conduit/DifferentialGetCommitMessageConduitAPIMethod.php > > Seems straightforward(-ish) to drop the relevant fields there, that way `arc land` automatically DTRT. > > Jon > > On Fri, Dec 27, 2019 at 11:30 PM Mehdi AMINI via llvm-dev <llvm-dev at lists.llvm.org> wrote: >> >> >> >> 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? >> >> >> That's be great! >> I suspect we can provide a bash function to add to one's .bashrc to make it trivial: >> >> function arcfilter() { git log -1 --pretty=%B | awk '/Reviewers: /{p=1; sub(/Reviewers: .*Differential Revision: /, "")}; /Differential Revision: /{p=0;}; !p' | git commit --amend -F - ; } >> >> Just running `arcfilter` before pushing will filter the commit it out automatically! >> >>> >>> It'd be nice if some pre-receive hooks can be set up to enforce deleting some really unnecessary metadata tags. >> >> >> Unfortunately you can't add pre-receive hook on GitHub as far as I know. >> >> -- >> Mehd >> >> _______________________________________________ >> 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
Jon Roelofs via llvm-dev
2020-Apr-09 16:45 UTC
[llvm-dev] Delete Phabricator metadata tags before committing
On Thu, Apr 9, 2020 at 10:29 AM Michael Kruse <llvmdev at meinersbur.de> wrote:> I was always assuming that the suggested commit is assembled in the > PHP code run by arcanist command run locally. If indeed the arc > command requests the commit message from the server,I assumed so too until I went digging for it. Seems the client-side stuff only deals with the structured data, and calls out to the server to construct the raw text when needed: https://github.com/phacility/arcanist/blob/33b9728b5f65fd747b7a15c0e25436c63e82f592/src/workflow/ArcanistDiffWorkflow.php#L757> we could do some > additional things: > > * Remove "Summary:" in front of message > * Line break after 72 columns > * Convert summary markdown formatting to plain texts (e.g. remove > backticks; I don't know any git client that renders as markdown) > * Add/check existence of [component] tag > > Alternatively, we could make an upstream feature request >>From what I can tell, upstream doesn't seem /that/ interested in it beingany more than a one-off thing: https://secure.phabricator.com/Q268 https://secure.phabricator.com/T12276 https://secure.phabricator.com/T11864 Jon> Michael > > Am Do., 9. Apr. 2020 um 11:16 Uhr schrieb Jon Roelofs via llvm-dev > <llvm-dev at lists.llvm.org>: > > > > Can we fix this in reviews.llvm.org's fork of phab? > > > > > https://github.com/phacility/phabricator/blob/cac3dc4983c3671ba4ec841aac8efac10744a80c/src/applications/differential/conduit/DifferentialGetCommitMessageConduitAPIMethod.php > > > > Seems straightforward(-ish) to drop the relevant fields there, that way > `arc land` automatically DTRT. > > > > Jon >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200409/23b8e721/attachment.html>
Hubert Tong via llvm-dev
2020-Apr-09 16:47 UTC
[llvm-dev] Delete Phabricator metadata tags before committing
On Thu, Apr 9, 2020 at 12:29 PM Michael Kruse via llvm-dev < llvm-dev at lists.llvm.org> wrote:> I was always assuming that the suggested commit is assembled in the > PHP code run by arcanist command run locally. If indeed the arc > command requests the commit message from the server, we could do some > additional things: > > * Remove "Summary:" in front of message > * Line break after 72 columns > * Convert summary markdown formatting to plain texts (e.g. remove > backticks; I don't know any git client that renders as markdown) >-1 on removing backticks. The `lowercase` in the middle of a sentence can be confusing without the backticks.> * Add/check existence of [component] tag > > Alternatively, we could make an upstream feature request > > Michael > > Am Do., 9. Apr. 2020 um 11:16 Uhr schrieb Jon Roelofs via llvm-dev > <llvm-dev at lists.llvm.org>: > > > > Can we fix this in reviews.llvm.org's fork of phab? > > > > > https://github.com/phacility/phabricator/blob/cac3dc4983c3671ba4ec841aac8efac10744a80c/src/applications/differential/conduit/DifferentialGetCommitMessageConduitAPIMethod.php > > > > Seems straightforward(-ish) to drop the relevant fields there, that way > `arc land` automatically DTRT. > > > > Jon > > > > On Fri, Dec 27, 2019 at 11:30 PM Mehdi AMINI via llvm-dev < > llvm-dev at lists.llvm.org> wrote: > >> > >> > >> > >> 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? > >> > >> > >> That's be great! > >> I suspect we can provide a bash function to add to one's .bashrc to > make it trivial: > >> > >> function arcfilter() { git log -1 --pretty=%B | awk '/Reviewers: > /{p=1; sub(/Reviewers: .*Differential Revision: /, "")}; /Differential > Revision: /{p=0;}; !p' | git commit --amend -F - ; } > >> > >> Just running `arcfilter` before pushing will filter the commit it out > automatically! > >> > >>> > >>> It'd be nice if some pre-receive hooks can be set up to enforce > deleting some really unnecessary metadata tags. > >> > >> > >> Unfortunately you can't add pre-receive hook on GitHub as far as I know. > >> > >> -- > >> Mehd > >> > >> _______________________________________________ > >> 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 > _______________________________________________ > 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/20200409/465bbc0c/attachment.html>