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.

In T121751#1910137, @Luke081515 wrote:

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 :-/

In T121751#2093243, @Luke081515 wrote:

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.

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.

In T121751#2095467, @Dereckson wrote:

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!).

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.

In T121751#1910101, @mmodell wrote:

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.

In T121751#2097093, @Dzahn wrote:

In T121751#1910101, @mmodell wrote:

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.

In T121751#2097091, @hashar wrote:

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!

In T121751#2097164, @mmodell wrote:

In T121751#2097091, @hashar wrote:

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

In T121751#2097103, @mmodell wrote:

In T121751#2097093, @Dzahn wrote:

In T121751#1910101, @mmodell wrote:

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

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

In T121751#2095410, @mmodell wrote:

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.

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

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

Evan said:

In T6008#74784, @epriestley wrote:

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?

In T121751#2103356, @Dzahn wrote:

+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.

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.

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).

In T121751#2108052, @Dzahn wrote:

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:

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.

In T121751#2108356, @Dereckson wrote:

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.

In T121751#2108555, @mmodell wrote:

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

Yes, with Depends on Dxxx.

In T121751#2114281, @Dereckson wrote:

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.

In T121751#2114288, @Dereckson wrote:

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.

In T121751#2114288, @Dereckson wrote:

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} and everything appears to have worked as it should.

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?

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

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 :)

In T121751#2108547, @mmodell wrote:

In T121751#2108356, @Dereckson wrote:

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.

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

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

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

<3