1299071 - Autoland shouldn't use r=me when landing something $user is not the author of

Status: RESOLVED FIXED

(Conduit Graveyard :: Transplant, defect)

Description

[Mike Hommey [:glandium]]

Last week, I landed bug 1291356, which was in a weird situation:
- there was one patch attached (as an actual bugzilla attachment) by bholley, authored by heycam, r? me.
- there was an additional patch from myself, on mozreview, r+ed by chmanchester.

So, since I wanted to land this, what I did was:
- import bholley's attachment to my local branch, retaining heycam's authorship, and r?glandium.
- push that and my r+ed patch to mozreview.
- r+ed heycam's patch on mozreview.
- Autolanded the whole.

Autoland put a r=me on heycam's patch, based on the fact that the person pushing to mozreview was me, but I wasn't the patch author, so this should have been a r=glandium.

Comment 1

[:glob ✱]

https://reviewboard.mozilla.org/r/74530/diff/1#index_header is the review request in question.

while the author is indeed heycam on reviewboard-hg (https://reviewboard-hg.mozilla.org/gecko/raw-rev/641fe5dd1c31), reviewboard looks at the submitter, which is glandium.

so we have https://github.com/mozilla/version-control-tools/blob/master/pylib/mozreview/mozreview/resources/commit_rewrite.py#L68 which ignores reviews made by the submitter, and https://github.com/mozilla/version-control-tools/blob/master/pylib/mozreview/mozreview/resources/commit_rewrite.py#L77 which appends r=me.

it's probably better to stop using r=me completely, and change that to always append the child request submitter's name instead.

Comment 2

[Gregory Szorc [:gps]]

I agree with doing away with r=me. It is a silly convention, likely invented to work around server side repo hooks.

Also, there are security implications to determining authorship based on commit metadata, as the author field can be spoofed. The only things we trust are the pushlog and the MozReview submitter, since those are authenticated.

Comment 3

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #2)
> I agree with doing away with r=me. It is a silly convention, likely invented
> to work around server side repo hooks.
> 
> Also, there are security implications to determining authorship based on
> commit metadata, as the author field can be spoofed.

The proposal in comment 0 was that r=me only apply when the person hitting r+ matches the author in the commit, and r=user otherwise. There is no security implication from that.

That said, comment 1 suggest to remove r=me entirely, that's fine-ish by me. I think there's value in having r=me (identifying patches that, in fact, haven't had a formal review by someone else)... when it's not wrong.

Comment 5

[:glob ✱]

Attachment: mozreview: Use instead of when rewriting commit descriptions (bug 1299071)

When the submitter is the same as the reviewer, use "r=username" instead of
"r=me".

Review commit: https://reviewboard.mozilla.org/r/79976/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/79976/

Comment 6

[:glob ✱]

Attachment: mozreview: Add a test for submitter==reviewer rewriting (bug 1299071)

Review commit: https://reviewboard.mozilla.org/r/79978/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/79978/

Comment 7

[Steven MacLeod [:smacleod]]

Comment on attachment 8793172 [details]
mozreview: Use  instead of when rewriting commit descriptions (bug 1299071)

https://reviewboard.mozilla.org/r/79976/#review83068

Comment 8

[Steven MacLeod [:smacleod]]

Comment on attachment 8793173 [details]
mozreview: Add a test for submitter==reviewer rewriting (bug 1299071)

https://reviewboard.mozilla.org/r/79978/#review83070

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/0b2233b1251c
mozreview: Use r=smacleod instead of when rewriting commit descriptions 
https://hg.mozilla.org/hgcustom/version-control-tools/rev/827379b102d6
mozreview: Add a test for submitter==reviewer rewriting r=smacleod

Comment 10

[:glob ✱]

(In reply to Pulsebot from comment #9)
> Pushed by [e-mail]:
> https://hg.mozilla.org/hgcustom/version-control-tools/rev/0b2233b1251c
> mozreview: Use r=smacleod instead of when rewriting commit descriptions 

oops - autoland butchered this commit description and i didn't catch it on the confirmation dialog :(