[llvm-dev] Delete Phabricator metadata tags before committing

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

[llvm-dev] Delete Phabricator metadata tags before committing

Jonas Paulsson via llvm-dev
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
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Jonas Paulsson via llvm-dev
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 <[hidden email]> 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
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Jonas Paulsson via llvm-dev
In reply to this post by Jonas Paulsson via llvm-dev


On Fri, Dec 27, 2019 at 12:48 PM Fangrui Song via llvm-dev <[hidden email]> 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
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Jonas Paulsson via llvm-dev
Maybe there's a way to make an on-save filter that correctly detects that the file being saved is a commit message (bonus if it can detect that it's a commit message for the llvm repo)?

On Fri, Dec 27, 2019 at 10:30 PM Mehdi AMINI via llvm-dev <[hidden email]> wrote:


On Fri, Dec 27, 2019 at 12:48 PM Fangrui Song via llvm-dev <[hidden email]> 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
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Jonas Paulsson via llvm-dev
In reply to this post by Jonas Paulsson via llvm-dev
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 <[hidden email]> 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 <[hidden email]> 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
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Jonas Paulsson via llvm-dev

+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 <[hidden email]> On Behalf Of James Henderson via llvm-dev
Sent: Thursday, January 2, 2020 9:57 AM
To: David Blaikie <[hidden email]>
Cc: llvm-dev <[hidden email]>
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 <[hidden email]> 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 <[hidden email]> 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
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev


_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Jonas Paulsson via llvm-dev
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 <[hidden email]> On Behalf Of James Henderson
>via llvm-dev
>Sent: Thursday, January 2, 2020 9:57 AM
>To: David Blaikie <[hidden email]>
>Cc: llvm-dev <[hidden email]>
>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 <
>[hidden email]> 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 <
>    [hidden email]> 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 -; }
_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Jonas Paulsson via llvm-dev
In reply to this post by Jonas Paulsson via llvm-dev
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 <[hidden email]> 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 <[hidden email]> On Behalf Of James Henderson via llvm-dev
Sent: Thursday, January 2, 2020 9:57 AM
To: David Blaikie <[hidden email]>
Cc: llvm-dev <[hidden email]>
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 <[hidden email]> 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 <[hidden email]> 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
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Jonas Paulsson via llvm-dev
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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> On Behalf Of James Henderson via llvm-dev
Sent: Thursday, January 2, 2020 9:57 AM
To: David Blaikie <[hidden email]>
Cc: llvm-dev <[hidden email]>
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 <[hidden email]> 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 <[hidden email]> 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
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Jonas Paulsson via llvm-dev
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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> On Behalf Of James Henderson via llvm-dev
Sent: Thursday, January 2, 2020 9:57 AM
To: David Blaikie <[hidden email]>
Cc: llvm-dev <[hidden email]>
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 <[hidden email]> 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 <[hidden email]> 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
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Jonas Paulsson via llvm-dev

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 <[hidden email]>
Sent: Monday, January 6, 2020 4:52 AM
To: David Blaikie <[hidden email]>
Cc: Mehdi AMINI <[hidden email]>; Joseph Tremoulet <[hidden email]>; llvm-dev <[hidden email]>
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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> On Behalf Of James Henderson via llvm-dev
Sent: Thursday, January 2, 2020 9:57 AM
To: David Blaikie <[hidden email]>
Cc: llvm-dev <[hidden email]>
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 <[hidden email]> 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 <[hidden email]> 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
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev


_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

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

Jonas Paulsson via llvm-dev
If somebody wants to make sure they don't push commits with extra tags, it's possible to set up a local pre-push hook that would complain.

In /path/to/llvm-project/.git/hooks/pre-push
paste the following

#!/bin/sh

# Disallow pushing commits that contain in their description the Phabricator
# tags that are unwelcome in the upstream LLVM repository.
#
# This hook is called with the following parameters:
#
# $1 -- Name of the remote to which the push is being done
# $2 -- URL to which the push is being done
#
# If pushing without using a named remote those arguments will be equal.
#
# Information about the commits which are being pushed is supplied as lines to
# the standard input in the form:
#
#   <local ref> <local sha1> <remote ref> <remote sha1>
#

# This SHA will be used if the commit is deleted on either side.
z40=0000000000000000000000000000000000000000

while read local_ref local_sha remote_ref remote_sha; do
    if [ "$local_sha" != "$z40" ]; then
        if [ "$remote_sha" = $z40 ]; then
            # New branch, examine all commits
            range="$local_sha"
        else
            # Update to existing branch, examine new commits
            range="$remote_sha..$local_sha"
        fi

        # Check for Phabricator tags and stop if found.
        commit=`git rev-list -n 1 \
          --grep '^Subscribers:' \
          --grep '^Tags:' \
          --grep '^Reviewers:' \
          "$range"`
        if [ -n "$commit" ]; then
            echo >&2 "Found Phabricator tags in $commit, push aborted."
            echo >&2 "Please remove the lines starting with Subscribers, Tags,"
            echo >&2 "Reviewers from the commit messages before pushing."
            exit 1
        fi
    fi
done

exit 0

and don't forget to chmod +x on the file.

This will report the hash of the commit about to be pushed if it contains "Subscribers", "Tags" or "Reviewers" and abort the push.

On Mon, Jan 6, 2020 at 4:18 PM Joseph Tremoulet via llvm-dev <[hidden email]> wrote:

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 <[hidden email]>
Sent: Monday, January 6, 2020 4:52 AM
To: David Blaikie <[hidden email]>
Cc: Mehdi AMINI <[hidden email]>; Joseph Tremoulet <[hidden email]>; llvm-dev <[hidden email]>
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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> On Behalf Of James Henderson via llvm-dev
Sent: Thursday, January 2, 2020 9:57 AM
To: David Blaikie <[hidden email]>
Cc: llvm-dev <[hidden email]>
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 <[hidden email]> 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 <[hidden email]> 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
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev


--
Alex

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev