Phab adds highly misleading "Reviewers" and fairly useless "Subscribers" to commit messages

[Quuxplusone]

Bugzilla Link	PR28300
Status	NEW
Importance	P normal
Reported by	Justin Bogner (llvm-bugs@justinbogner.com)
Reported on	2016-06-24 23:36:21 -0700
Last modified on	2016-09-02 12:57:27 -0700
Version	unspecified
Hardware	PC All
CC	ahmed@bougacha.org, anemet@apple.com, klimek@google.com, llvm-bugs@lists.llvm.org, sanjoy@playingwithpointers.com, silvasean@google.com
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

Commits of reviews that are on phabricator often list "Reviewers" and "Subscribers", but both fields are just a random set of people who were CC'd and may never have actually looked. This is noise and misleading information that makes our commit history far more confusing.

[Quuxplusone]

My suggestion is to not use phab to submit code. It's up to folks in general, though.

[Quuxplusone]

If we're going to allow people to use phab for this kind of thing, it needs to work. I'd be fine with banning it, but I'm pretty sure people disagree with me there.

[Quuxplusone]

I have the opposite sentiment from Justin --- I think the reviewers/subscribers are useful to include as a quick mirroring of the info in phabricator. I think that "who was CC'd on the review" is actually handy to have in the commit message and wish we had that for non-phab commits.

[Quuxplusone]

I strongly disagree with Sean here. The list of people who happened to be CC'd is not useful, and implies that they read and tacitly approved of the patch despite that they may not have actually seen it yet. I'm only okay with someone including my name in the message when they commit if I read the patch before they committed.

If you aren't absolutely certain I've actually read your patch before committing, please do not include my name in the commit message.

[Quuxplusone]

I think the Reviewers and Subscribers fields are definitely noise (since, as Justin said, they don't reflect who actually reviewed the change). I personally just ignore them, but I can see how it can mislead not familiar with Phabricator.

This looks like a problem any phabricator-using project would face -- perhaps there is a simple fix out there?

Ideally, a arc amend should change the commit message to:

Reviewed by: < people who commented on the review > LGTM'ed by: people who lgtmed the change Subscribers: (removed, I think this field is not useful)

[Quuxplusone]

+1 on getting this fixed. I don't land via arc but use arc amend to get the magic line into the commit log to get the review close automatically. As Sanjoy I have learned to ignore the Reviewers/Subscriber lines.