Page MenuHomePhabricator

Document best practices to amend a change written by another contributor
Closed, ResolvedPublic

Description

Currently, we see on Gerrit the following workflow:

  1. A first contributor submits a charge. They're both author and committer.
  2. Another user amends the change (git review -d 10000 ; edit some files or do a rebase ; git commit --amend ; git review -R). The change of the PS is authored by the first contributor, committed by the second.
  3. This same user can review, and if all is fine, +1 or +2.

On Differential, first, you can very easily edit the commit message, without creating a new patchset: go to the UI, edit revision. So this is pretty straightforward (and matches the Gerrit automatic new PS creation for such change on Gerrit).

The tricky question is how do you amend the rest of the commit.

By default, it works like this on Differential:

  1. A first contributor submits a change. They're both author and committer.
  2. A second contributor gets and amends the change locally (arc patch D10000 ; edit some files ; git commit --amend).
  3. On the web UI, they commandeer the revision. At this moment, the former author is now a reviewer, and the commandeering user becomes the differential author.
  4. This contributor being the revision author, they can arc diff to send the amended change to Differential.
  5. As long as self accept and self review is technically allowed, the same contributor wants to accept the change, but the UI will show the change as self reviewed.

Upstream task: https://secure.phabricator.com/T10584

Related Objects

StatusAssignedTask
ResolvedDzahn
ResolvedCmjohnson
ResolvedDzahn
Resolveddemon
Resolveddemon
ResolvedDanny_B
ResolvedPaladox
ResolvedPaladox
ResolvedNemo_bis
Resolveddemon
ResolvedPaladox
ResolvedKrenair
Resolvedmmodell
InvalidNone
DeclinedNone
Resolveddemon
InvalidNone
InvalidNone
ResolvedQgil
DeclinedNone
DuplicateNone
Resolvedgreg
DuplicateNone
Resolveddemon
DeclinedNone
Resolvedmmodell
Resolvedgreg

Event Timeline

Dereckson raised the priority of this task from to Normal.
Dereckson updated the task description. (Show Details)
Dereckson added subscribers: cscott, Luke081515, Qgil and 7 others.

I think this is just a fundamental difference in how phabricator treats changes. The act of commandeering a revision is rather presumptuous and phabricator doesn't really encourage it. The only time that I can think of when it would be appropriate would be when the original contributor has disappeared and the patch is languishing.

Taking over someone's change is a rather heavy-handed thing to do so I don't think it's inappropriate that phabricator doesn't encourage the practice.

The problem:
In gerrit, you can correct a mistake made by a user, or correct the commit message, you can correct this, by uploading a new patch set. But at phab, you can't upload a set for a revision, if you are not the author, so you need to commandeer the revision, if you want to correct something at this set.

@mmodell Please take a look around Gerrit and see what's happening right now. There are more than 30% of the changes with two authors during the Gerrit review.

It's not presumptuous, it's a way to craft the better change possible.

greg moved this task from To Triage to Tooling on the Gerrit-Migration board.Jan 6 2016, 7:16 PM

The problem:
In gerrit, you can correct a mistake made by a user, or correct the commit message, you can correct this, by uploading a new patch set. But at phab, you can't upload a set for a revision, if you are not the author, so you need to commandeer the revision, if you want to correct something at this set.

You can edit the commit message directly in the differential web interface.

But not the commit content, for example if a user made a little mistake :-/

mmodell added a comment.EditedMar 7 2016, 6:42 PM

But not the commit content, for example if a user made a little mistake :-/

Why not just comment on the diff and let the author fix their mistake?

If you change someone's code it can cause problems for them because then they have to rebase on top of your changes and it could completely break their workflow.

Dereckson added a comment.EditedMar 7 2016, 6:48 PM

Actually, we have been using this workflow since 2012. This is the current Wikimedia practice to amend changes without any hesitation, probably as it inherits from the collaborative wiki culture.

The workflow currently includes a step "retrieve the amended" code:

  • git pull ... for those not using git-review
  • git review -d <change number> for git-review users
  • arc patch <differential ID> in the future under Phabricator

Nothing is changed there, the workflow will stay the same as far as the CLI commands are involved.

greg added a comment.Mar 7 2016, 7:05 PM

Actually, we have been using this workflow since 2012.

That, in and of itself, is not a reason to make it a blocking feature for migration to Differential. We have to accept that things will not be 100% like they are with Gerrit (for good reasons in many cases!).

Dereckson added a comment.EditedMar 7 2016, 7:11 PM

Yes, I concur, we should evaluate the cost/benefit of the collaborative commit crafting model per se, and not only because Gerrit allowed that.

And as there is a way to achieve this workflow through commandeer + a commit message change to credit original author, this is clearly not a blocker.

If we were modeling the wiki way, I would think that post-commit review would make more sense. But pre-commit review, which differential models, is a sort of saying "hey this is a rough sketch of what I want to do, your feedback please?" rather than "look at what I've done, I think it's finished but I'd like you to fix any mistakes I've made"

I constantly amend/finish up other authors changes. Either because I know the author is not going to respond any time soon and the change is not worth waiting (ex: change has a typo in code) or because it does not make sense to wait a day or so to get a configuration change deployed.

Currently I do:

  • git-review -d 12345
  • <fix / git amend -a
  • git-review
  • CR+2
  • deploy

The Author is the original author and I am just the committer. If I have done anything substantial I would additionally sign it off.

Another use case I have seen is people handling rebase/merge conflict for another. They are sometime tricky and definitely confusing for people not used to deal with diffs and merge. So it is done as a courtesy.

Dzahn added a subscriber: Dzahn.Mar 7 2016, 10:27 PM

Taking over someone's change is a rather heavy-handed thing to do so I don't think it's inappropriate that phabricator doesn't encourage the practice.

No, i do this all the time, it's very useful and in line with the wiki principle. Not being able to work on other people's patches would be a serious regression.

Taking over someone's change is a rather heavy-handed thing to do so I don't think it's inappropriate that phabricator doesn't encourage the practice.

No, i do this all the time, it's very useful and in line with the wiki principle. Not being able to work on other people's patches would be a serious regression.

You can do that in phabricator, it just uses the strong wording 'commandeer' to describe the action.

mmodell added a comment.EditedMar 7 2016, 10:40 PM

I constantly amend/finish up other authors changes. Either because I know the author is not going to respond any time soon and the change is not worth waiting (ex: change has a typo in code) or because it does not make sense to wait a day or so to get a configuration change deployed.

The Author is the original author and I am just the committer. If I have done anything substantial I would additionally sign it off.

Another use case I have seen is people handling rebase/merge conflict for another. They are sometime tricky and definitely confusing for people not used to deal with diffs and merge. So it is done as a courtesy.

You can do those things in with differential:

Instead of

git-review -d 12345
# fix
git amend -a
git-review
# CR+2
# deploy

You do

         # accept D12345 (equiv of CR+2), then run:
arc patch D12345
         # this applies D12345's change to your checkout of the repo.
         # now you can make some minor change to fix a typo
         # or rebase and fix minor conflict. then amend:
git commit --amend -a
         # finally, land the change and push:
arc land

arc land is just automation around git merge (or rebase) + git push. Phabricator takes care of updating the diff when it sees the pushed commit, so arc land is not actually required, you could just as well commit and push.

@mmodell straight forward :-} That address my concern!

greg added a comment.Mar 7 2016, 11:17 PM

I constantly amend/finish up other authors changes. Either because I know the author is not going to respond any time soon and the change is not worth waiting (ex: change has a typo in code) or because it does not make sense to wait a day or so to get a configuration change deployed.
The Author is the original author and I am just the committer. If I have done anything substantial I would additionally sign it off.
Another use case I have seen is people handling rebase/merge conflict for another. They are sometime tricky and definitely confusing for people not used to deal with diffs and merge. So it is done as a courtesy.

You can do those things in with differential:

Documented: https://www.mediawiki.org/wiki/Phabricator/Differential#Amend_another_author.27s_change

greg added a comment.Mar 7 2016, 11:21 PM

Taking over someone's change is a rather heavy-handed thing to do so I don't think it's inappropriate that phabricator doesn't encourage the practice.

No, i do this all the time, it's very useful and in line with the wiki principle. Not being able to work on other people's patches would be a serious regression.

You can do that in phabricator, it just uses the strong wording 'commandeer' to describe the action.

https://www.mediawiki.org/wiki/Phabricator/Differential#Take_over_another_author.27s_change

greg added a comment.Mar 7 2016, 11:23 PM

With these things documented, is this now resolvable?
https://www.mediawiki.org/wiki/Phabricator/Differential

Why not just comment on the diff and let the author fix their mistake?

Sometimes it's much easier to show than to explain.

If you change someone's code it can cause problems for them because then they have to rebase on top of your changes and it could completely break their workflow.

It could, but when done sensibly it's always a pleasure to see two or more people working on the same change.

In short, I agree that not all of Gerrit's peculiarities make sense, but it'd be a pity to lose a best practice (or to make it harder to apply) only because the folks at Phacility are not comfortable with it.

Dzahn added a comment.Mar 9 2016, 4:09 PM

+100 to what @Ricordisamoa said above. pleeeeeassse let us keep working with multiple people on patches like we can do with Gerrit.

greg added a subscriber: epriestley.Mar 9 2016, 4:30 PM

See upstream's https://secure.phabricator.com/T6008

Evan said:

Or, put another way, I estimate the cost of this feature is far greater than all other features WMF has ever asked for combined, and that it's also near the bottom of the list in terms of utility to other installs and overlap with the "natural" upstream roadmap. This makes it very hard for us to get excited about, and if we did prioritize it it would be at the cost of everything else.

@greg: Are you sure you quoted/referenced the right upstream task?

This task is about amending/commandeering someone else's differential diff. https://secure.phabricator.com/T6008 seems to be about editing and committing to git repos directly from phabricator's web interface.

Or did I just miss some context?

+100 to what @Ricordisamoa said above. pleeeeeassse let us keep working with multiple people on patches like we can do with Gerrit.

You can collaborate on a patch in differential. I have simply been making the argument that it may not always be a best practice to amend someone else's patch, especially if they aren't expecting it. I think that phabricator rightly discourages it by using strong language like 'commandeer'

Also, I don't think anything prevents you from submitting a revision without commandeering... It works the same way as Gerrit in all the important dimensions.

Dereckson added a comment.EditedMar 9 2016, 5:07 PM

Also, I don't think anything prevents you from submitting a revision without commandeering... It works the same way as Gerrit in all the important dimensions.

No, you can't upload a new revision to a Differential item you don't own, you must commandeer it first.

Dzahn added a comment.Mar 10 2016, 6:00 PM

I was always hoping we could go the opposite direction and have more amending, encourage people to work together on the same patch and just fix that missing comma, the wrong indentation, like you would fix a typo on a wiki page, rather than just slapping a -1 on it and moving on. I have often amended other people's changes to make a tiny fix and then merged them. The same way it's really cool to wake up and see somebody else already fixed your thing and moved it forward vs. just seeing a bunch of negative reviews telling you what to fix.

The word "commandeering" sounds quite negative and like you say that is on purpose to discourage doing this, so i'm a bit sad we will see less of this than we had on Gerrit. (which could have had more of that in the first place).

I was always hoping we could go the opposite direction and have more amending, encourage people to work together on the same patch and just fix that missing comma, the wrong indentation, like you would fix a typo on a wiki page, rather than just slapping a -1 on it and moving on. I have often amended other people's changes to make a tiny fix and then merged them. The same way it's really cool to wake up and see somebody else already fixed your thing and moved it forward vs. just seeing a bunch of negative reviews telling you what to fix.
The word "commandeering" sounds quite negative and like you say that is on purpose to discourage doing this, so i'm a bit sad we will see less of this than we had on Gerrit. (which could have had more of that in the first place).

Despite @Dereckson's assertion above, you can correct small errors and then merge a patch, without commandeering it - with one caveat: only someone with push rights can do that. This is how it works:

1# Get the patch locally:
2arc patch Dxxx
3# Fix something minor:
4vim somefile
5# Ammend the HEAD with your change:
6git commit -a --amend
7# or commit your change separately:
8git commit -m 'Fixed X minor typo.'
9# Merge and publish the change:
10arc land

There is, however, a much better method for collaboratively working together on changes, where it's easy to just fix things and collaboration is encouraged, wiki-style. And that is to use git, publish a branch and use the audit tool to do code review on commits after they are pushed.

Pre-commit code review and collaborative development are somewhat incompatible, at least in gerrit and differential (which are very similar to each other, at least in the low level technical details). Audit is a different, lighter-weight, code review mechanism and it's much more compatible with working the way that @Dzahn has described above.

Actually, P2738 uses arc land, and as such, it's a procedure to amend a change at merge time, not at the middle of the review. This furthermore makes the assumption we want to drop Zuul and not use gating anymore to push changes.

In the procedure you offer, you accept a change, decide to merge it, edit it locally, land it. The land operation is a git push, with no certitude tests pass for the content you push, which is a regression compared to our currently gated system.

Currently, our changes are gated by Zuul, which offer us the following benefits:

  • tests run a last time before the merge, so is if an incompatible change merged between the last test run for this patch and the merge operation, the master branch won't break
  • we can mark a change depending on another in another repository in the commit message, forget this detail, Zuul won't forget and will defer merge operation until the dependent change is merged

If we want to keep zuul the final wokflow won't be an arc land operation but instead:

  • accept the change
  • once the change is accepted, Zuul picks it

Another possibility is to add a specific workflow to Phabricator (a little like the former "Land to GitHub" workflow, but here probably relying on harbormaster instead) to explicitly send the change to be picked by Zuul.

Actually, P2738 uses arc land, and as such, it's a procedure to amend a change at merge time, not at the middle of the review. This furthermore makes the assumption we want to drop Zuul and not use gating anymore to push changes.

AFAIK that is already a foregone conclusion. Zuul only works with gerrit and Phabricator has it's own tooling (harbormaster and drydock) for achieving similar functionality.

Also, I believe you can mark dependencies in Differential from the commit message... I'll look into that further.

greg moved this task from Tooling to Documentation on the Gerrit-Migration board.Mar 10 2016, 11:42 PM

Also, I believe you can mark dependencies in Differential from the commit message... I'll look into that further.

Yes, with Depends on Dxxx.

Yes, with Depends on Dxxx.

So how is this inferior to gerrit?

My statement "we can mark [On Gerrit] a change depending on another in another repository in the commit message, forget this detail, Zuul won't forget and will defer merge operation until the dependent change is merged" never implied it were inferior, equal or greater to Gerrit.

It were an explanation on how Zuul works, and, per your comment as T121751#2108547, how it would be interesting to recreate a merge system in Harbormaster. This seems by the way out of scope of this task, which is not about how to recreate the merge pipeline.


So, to summarize, this task have seen several contributors interested by the capability of amend a change. You've demonstrated a good workflow to perform such change outside the review, before the merge.

But that doesn't address one of the need expressed by @hashar and @Ricordisamoa: the rebase as a courtesy. Newcomers, not familiar with Git are lost in easily lost rebase a change. When in a middle of a change, and not at the end, someone is stuck with a merge conflict, we currently can rebase it, with the intent the contributor continues to amend the change later. With your workflow, it's not possible.

This is especially true and critical during the Outreachy, Google Summer of Code, and Google Code-In programs.

Now, could we explore a simple way to make everyone happy to work as they wish?

If so, I would recommend to assert the cost/benefit to maintain locally a patch to bypass the commandeer security, so:

  • We can amend a change, and Differential accept it: if we left all other bits like there are, the action should normally be visible in the story, and the Differential properties will still be to the initial author.
  • When someone feels there is a need to "takeover" the credit and become the author/committer, they can use the commandeer feature

Excepted if a surprise occurs, this could be rather simple to implement, in the best case a simple reject condition to remove.

By the way, we would also need to check Arcanist too, as sometimes there are verification in both. See for example this Arcanist commit I authored in October 2012 removing such a check.

But that doesn't address one of the need expressed by @hashar and @Ricordisamoa: the rebase as a courtesy. Newcomers, not familiar with Git are lost in easily lost rebase a change. When in a middle of a change, and not at the end, someone is stuck with a merge conflict, we currently can rebase it, with the intent the contributor continues to amend the change later. With your workflow, it's not possible.
This is especially true and critical during the Outreachy, Google Summer of Code, and Google Code-In programs.
Now, could we explore a simple way to make everyone happy to work as they wish?
If so, I would recommend to assert the cost/benefit to maintain locally a patch to bypass the commandeer security, so:

  • We can amend a change, and Differential accept it: if we left all other bits like there are, the action should normally be visible in the story, and the Differential properties will still be to the initial author.
  • When someone feels there is a need to "takeover" the credit and become the author/committer, they can use the commandeer feature

Excepted if a surprise occurs, this could be rather simple to implement, in the best case a simple reject condition to remove.

This sounds like a reasonable thing to do. I'll look into it, maybe it's not too difficult.

By the way, we would also need to check Arcanist too, as sometimes there are verification in both. See for example this Arcanist commit I authored in October 2012 removing such a check.

It looks like the only thing enforcing revision ownership is in arcanist, not differential: ArcanistDiffWorkflow.php#2524. I couldn't find anywhere else that it's enforced.

@Dereckson after disabling the check in arcanist, I was able to amend D146: Update docs to use subcommands rather than scripts and everything appears to have worked as it should.

Dereckson added a comment.EditedMar 13 2016, 2:42 AM

Et voilà, we have our amend workflow fully operational, that's a good news.

This has a cost of a small edit to perform in the tool code an edit for each user, to distribute a patched version or to persuade upstream an option would be here valuable.

If we take the option road, there is the location issue: in .arcrc as an user responsibility to configure the tool how they want or in .arcconfig as a repository configuration stating this practice is socially acceptable?

Dereckson updated the task description. (Show Details)Mar 13 2016, 8:37 PM
hashar moved this task from Backlog to Patch proposed upstream on the Upstream board.
mmodell updated the task description. (Show Details)Mar 14 2016, 4:35 PM
mmodell set Security to None.
mmodell updated the task description. (Show Details)

There is a whole lot of discussion in upstream diff https://secure.phabricator.com/D15468 and task https://secure.phabricator.com/T10584

http://www.loper-os.org/?p=1545 has an interesting idea about signing a revision with 1 byte of information to represent things like "I created most, or modified the original beyond recognition." as well as level of trust and extent of evaluation. Seems like an interesting way of tagging a code review with more nuance than a simple binary 'accept' plus it handles the multiple authorship case.

Just food for thought :)

Paladox added a subscriber: Paladox.Apr 2 2016, 4:09 PM

Actually, P2738 uses arc land, and as such, it's a procedure to amend a change at merge time, not at the middle of the review. This furthermore makes the assumption we want to drop Zuul and not use gating anymore to push changes.

AFAIK that is already a foregone conclusion. Zuul only works with gerrit and Phabricator has it's own tooling (harbormaster and drydock) for achieving similar functionality.

But woulden we be able to update zuul to support phabricator too. Since zuul makes it easy to view what tests are running.

Paladox added a comment.EditedApr 2 2016, 4:13 PM

Ive created this task https://secure.phabricator.com/T10712 for zuul

Qgil removed a subscriber: Qgil.Apr 3 2016, 7:29 AM

merged upstream: https://secure.phabricator.com/D15468

arcanist now allows you to amend a patch submitted by someone else.

greg moved this task from To Triage to Backlog on the Differential board.May 12 2016, 10:37 PM
greg closed this task as Resolved.May 13 2016, 5:18 PM
greg claimed this task.

I think we're good here now, yes? https://www.mediawiki.org/wiki/Phabricator/Differential#Amend_another_author.27s_change

My last update, which makes it clear you don't have to land the change to amend it, you can just amend it: https://www.mediawiki.org/w/index.php?title=Phabricator%2FDifferential&type=revision&diff=2127002&oldid=2115929

Restricted Application added a project: User-greg. · View Herald TranscriptMay 13 2016, 5:18 PM