1513222 - Investigate if commandeering a revision and pushing fixups removes commit author information

Status: RESOLVED FIXED

(Conduit :: Phabricator, defect, P1)

Description

[Israel Madueme [:imadueme]]

> sfoster> I commandeered a revision to make a couple of changes to the patch (I
> was unable to moz-phab submit otherwise?) I've re-submitted it with moz-phab
> submit but now its complaining about the author - which phabricator thinks is me, 
> but the patch (correctly) has as the original author. How to fix? 
> The rev. in question: https://phabricator.services.mozilla.com/D13738

Diff 1 (https://phabricator.services.mozilla.com/D13738?id=41819, made by the original author) has the correct commit author info

Diff 2 (https://phabricator.services.mozilla.com/D13738?id=42731, presumably pushed by sfoster) has no commit author info

The problem could be with phabricator, arcanist, or mozphab.

Comment 1

[Israel Madueme [:imadueme]]

I was not able to reproduce this bug - specifically the case where a revision is uploaded, then commandeered, then updated and submitted by mozphab, leading to lando complaining about the author.

Can you clarify a few things?
- Are you using git-cinnabar or hg
- Do you use `arc patch` to pull down the changeset, or do you download and apply the raw diff?

Another oddity is that phabricator's api (https://phabricator.services.mozilla.com/conduit/method/differential.querydiffs/) says that both the original diff and the updated diff on D13738 were created via the web UI (the web UI doesn't properly store commit author information). You said you submitted the update with mozphab, so I don't know if this is a bug with phabricator, mozphab, or something inbetween. In my testing, even when the original diff was created via the web UI, all diffs uploaded by mozphab correctly say that they were created by `arc` (seen in the `creationMethod` field in the result of that api query).

If you're able to clarify or have steps to reproduce, that would be a great help! If you think this was just a one off fluke, feel free to close out this bug and reopen it if it happens again.

Comment 2

[Sam Foster [:sfoster] (he/him)]

(In reply to Israel Madueme [:imadueme] from comment #1)
> I was not able to reproduce this bug - specifically the case where a
> revision is uploaded, then commandeered, then updated and submitted by
> mozphab, leading to lando complaining about the author.
> 
> Can you clarify a few things?
> - Are you using git-cinnabar or hg
hg

> - Do you use `arc patch` to pull down the changeset, or do you download and
> apply the raw diff?

I used arc patch. 

> Another oddity is that phabricator's api
> (https://phabricator.services.mozilla.com/conduit/method/differential.
> querydiffs/) says that both the original diff and the updated diff on D13738
> were created via the web UI (the web UI doesn't properly store commit author
> information). You said you submitted the update with mozphab, so I don't
> know if this is a bug with phabricator, mozphab, or something inbetween.

I attempted to use mozphab submit but got a message saying there was a problem with the author information. I tried using the web UI but that wanted to create an entirely new revision and lose the history and r+ the current revision had. So in the end I think I commandeered the revision, pushed my changes and asked the original author to commandeer it again to land it. 
 
> If you're able to clarify or have steps to reproduce, that would be a great
> help! If you think this was just a one off fluke, feel free to close out
> this bug and reopen it if it happens again.

If you have a sandbox install I could try to reproduce. The essence of it is: 
1. Author A submits a patch using moz-phab
2. Reviewer adds r+ with nits. 
3. Author B or Reviewer wants to update the revision to fix those nits
4. Any of Author A, B or Reviewer want to be able to land the finalized revision using Lando, with the primary authorship of Author A and reviewer intact in the commit message.

Comment 3

[Israel Madueme [:imadueme]]

Attachment: Add a tip about preserving commit author info when commandering a revision (bug 1513222)

Help users know how to preserve the original commit author when updating
someone else's revision.

Comment 4

[Israel Madueme [:imadueme]]

Hi Sam,

Sorry for the delay on this. I've finally been able to figure out what you're describing.
As you say, when you submit a revision on behalf of someone else, the commit author information is removed and replaced by your own info because you created the commit on your machine. 

The original bug report sounded like 'no commit author info' is submitted (which I could never reproduce since my hgrc has my username set correctly). Commendering also isn't the cause of the problem, it just happens that you have to do so in order to submit a push to someone else's revision.

At this time, I'm not able to fix the root cause of arc/mozphab/phabricator replacing the commit info, but, I do have some info and a workaround for you.

- When you are updating someone else's revision you can use `hg commit --amend --user "Other Person <[e-mail]>"` or `git commit --amend --author="Other Person <[e-mail]>"` to ensure that the commit author is them. It is not ideal since you have to remember to do this and know their name/email, but, it is better than having the wrong author on the commit. (You can look at the "Commits" tab on a Phabricator Revision to see who the exisitng commit author is, if any).

- You mentioned that you comandeered the revision then asked the author to comandeer it back. You won't have to do this anymore. On a revision there is the  "Phabricator Author" and the "commit author". Normally these are the same except if you commandeer a revision then push a commit with the original author. This is fine, since Lando always uses the "commit author" information instead of the "Phabricator Author". You can commandeer, push with the correct commit author using the commands above, and Lando will land it with the correct author.
  - You might have to ask them to commandeer it back if you were the only reviewer on the revision.

- You can create an account on https://phabricator-dev.allizom.org/ to test things out.

I'll talk with the team to see if we or Phacility can have an option to automatically keep the original commit author. Thanks for the bug report!
I've updated https://wiki.mozilla.org/Phabricator/FAQ and made a patch to https://moz-conduit.readthedocs.io to spread this info. Let me know if you have any thoughts or discoveries!

Comment 5

[Sam Foster [:sfoster] (he/him)]

(In reply to Israel Madueme [:imadueme] from comment #4)

> - You can create an account on https://phabricator-dev.allizom.org/ to test
> things out.

That's good to know, I've made a note. 

> I've updated https://wiki.mozilla.org/Phabricator/FAQ and made a patch to
> https://moz-conduit.readthedocs.io to spread this info. Let me know if you
> have any thoughts or discoveries!

Perfect, thanks a lot, the extra docs help.

Comment 6

[:glob ✱]

looks like there isn't anything else to do here, closing to remove from radar.