Open Bug 1538151 Opened 6 years ago Updated 3 years ago

Could not land a patch from a volunteer after having rebased it

Categories

(Conduit :: Phabricator, defect, P2)

defect

Tracking

(Not tracked)

REOPENED

People

(Reporter: padenot, Unassigned)

References

Details

(Keywords: conduit-triaged, conduit-upstream)

Attachments

(2 files)

I have this patch written by a volunteer, that I'd like to land, but before I need to rebase it. I've pulled the patch locally, and did the rebase, and so I new have a revision with the volunteer's name on it, and I'm the reviewer for this revison, that now applies fine. I now want to update the revision on phab to then land it using lando, but I can't:

~/src/trees/mozilla-unified::[5839d96]$ hg log -f
changeset:   520287:5839d96348d4
tag:         tip
parent:      520284:b2f1edb41241
user:        Valentin Millet <valentin.millet39@gmail.com>
date:        Thu Mar 21 15:31:25 2019 +0100
summary:     Bug 1477205 - Remove the exception when a node is created after closing the AudioContext. r=padenot
~/src/trees/mozilla-unified::[5839d96]$ hg phabsend -r .
abort: Conduit Error (ERR-CONDUIT-CORE): Validation errors:
  - The author of a revision can not be a reviewer.

This makes little sense, since the author of the revision is Valentin, and I'm just the reviewer and the person that pushed an updated version.

Removing r=padenot from the message allow is enough to side-step this bug, it was then added back by Lando, as expected.

It think it would be good to have this scenario working well, rebasing a patch from a volunteer to land it is common enough.

:zalun, could you take a look and see if moz-phab also has this problem?

Flags: needinfo?(pzalewa)

The error is the same when using moz-phab, technically it comes from calling Arcanist.

EXCEPTION: (ConduitClientException) ERR-CONDUIT-CORE: Validation errors:
  - The author of a revision can not be a reviewer. at [<phutil>/src/conduit/ConduitFuture.php:62]

I believe the same situation would happen when called API directly.

The revision in the error message is Phabricator revision, not Mercurial one.
Author of the "commit" might be any string in the right format. It can be not represented in the Phabricator at all.

Flags: needinfo?(pzalewa)
Keywords: conduit-triaged
Priority: -- → P2

This actually appears to be an upstream Phabricator bug. It looks like Phabricator is considering the currently authenticated user uploading the diff as the "author", when it should be using the real author of the revision. Phabricator itself needs to change to pull the author from the proper spot.

Until this happens the workaround is to make sure you're not attempting to set yourself as a reviewer using moz-phab (so removing r=<username> from the message as padenot indicated).

Blocks: 1514805

(Note: this bug is completely unrelated to Bug 1557726. Bugzilla receives the correct information from Phabricator, it's just using data with semantics the user wouldn't expect. The issue in this bug is an application bug in Phabricator itself preventing diff upload. Yes, they both involve revision ownership, but this bug has nothing to do with commit authorship or misleading users)

Following up on this bug. Is this currently on the roadmap?

Attached image Update revision messages (deleted) β€”

I've used the following steps and ended up with the desired result:

Revision https://phabricator-dev.allizom.org/D1340 was created by mars and no reviewer was set.

$ moz-phab patch D1340
Patching revision: D1340
Checked out 101726fb9ac6
Bookmark set to D1340
D1340 applied

$ hg log --graph
@  changeset:   8:5e371180e9cd
|  bookmark:    D1340
|  tag:         tip
|  parent:      1:101726fb9ac6
|  user:        Māris Fogels <mars@mozilla.com>
|  date:        Wed Sep 18 17:54:01 2019 +0200
|  summary:     Bug 1394742 - Add TEST2
|
| o  changeset:   7:e50a49d2ffe1
|/   bookmark:    D1230
|    parent:      1:101726fb9ac6
|    user:        Piotr Zalewa <pzalewa@mozilla.com>
|    date:        Mon Jun 03 11:44:05 2019 +0200
|    summary:     Bug 1395238 - testing phabsend (1)

$ hg rebase --source 8 --dest 7
rebasing 8:5e371180e9cd "Bug 1394742 - Add TEST2" (D1340 tip)

$ moz-phab submit -r zalun .
Submitting 1 commit for review:
(D1340) 9:674e26e7b3ff Bug 1394742 - Add TEST2 r=zalun
!! You don't own this revision.  Normally, you should only
   update revisions you own.  You can "Commandeer" this
   revision from the web interface if you want to become
   the owner.
[...]
Completed
(D1340) 10:8dee26309590 Bug 1394742 - Add TEST2 r=zalun
-> https://phabricator-dev.allizom.org/D1340
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WORKSFORME

Note: upstream also has reported they are unable to reproduce this.

When(In reply to Steven MacLeod [:smacleod] from comment #4)

This actually appears to be an upstream Phabricator bug. It looks like Phabricator is considering the currently authenticated user uploading the diff as the "author", when it should be using the real author of the revision. Phabricator itself needs to change to pull the author from the proper spot.

Until this happens the workaround is to make sure you're not attempting to set yourself as a reviewer using moz-phab (so removing r=<username> from the message as padenot indicated).

Hi all,
I just had this exact problem, indeed it seems that Phabricator considers the submitter to be the author of the commit, might be a problem in certain cases.
I tried both with moz-phab with r=bbeurdouche and manually setting the reviewer in Phabricator, both ended with the error:
"The author of a revision can not be a reviewer".

Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Attached image Screenshot 2020-04-10 at 11.33.51.png (deleted) β€”
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: