Paths

Table of Contentst

Differential D15468

Allow amending revisions without commandeering first
ClosedPublic
Actions

Authored by 20after4 on Mar 13 2016, 4:46 AM.

Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Maniphest Tasks

T10584: Allow users to amend revisions they don't own, without commandeering them

Commits

rARC737f5c0df976: Allow amending revisions without commandeering first

Summary

It is common practice in Wikimedia's projects to amend a contributor's
change without taking over authorship of the change. We found that
the only enforcement of commandeering before amending is in arcanist,
not validated server-side. While it would be fairly straightforward to
maintain this as a patch to arcanist, I thought I would see if upstream
is willing to support making this optional.

With this change, amending without commandeering is enabled by a flag in
.arcconfig and it defaults to the old behavior.

For background see wmf T121751

Test Plan

ran arc patch D146 to locally apply a revision that I did not author,
made a trivial change and amended the commit.
ran arc diff --update D146 HEAD^ to send the update to differential
Saw that https://phabricator.wikimedia.org/D146 updated as it should.

Diff Detail

Repository

rARC Arcanist

Branch

arcpatch-D15468

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 11598
Build 14497: Run Core Tests
Build 14496: arc lint + arc unit

Event Timeline

20after4 updated this revision to Diff 37280.Mar 13 2016, 4:46 AM

20after4 retitled this revision from to Allow amending revisions without commandeering first.

20after4 updated this object.

20after4 edited the test plan for this revision. (Show Details)

20after4 added a reviewer: epriestley.

20after4 added a subscriber: dereckson.

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptMar 13 2016, 4:46 AM

Herald added a subscriber: Korvin. · View Herald Transcript

Looks like the test failure is infrastructure, not caused by my change?

Luke081515.2 added a subscriber: Luke081515.2.Mar 13 2016, 8:52 PM

aklapper added a subscriber: aklapper.Mar 14 2016, 11:11 AM

epriestley mentioned this in T10584: Allow users to amend revisions they don't own, without commandeering them.Mar 14 2016, 12:24 PM

epriestley added a task: T10584: Allow users to amend revisions they don't own, without commandeering them.

I filed T10584 to discuss this since I have some thoughts that aren't related to the technical approach. To briefly summarize the relevant bit from there, I don't want to add an option for this, but I don't have any technical objection to turning it into a prompt in all cases.

Let's just change this into a prompt, unconditionally? (Remove the setting, always prompt everyone.)

You don't own revision "Dxxx: yyyy". Normally, you should only update revisions
you own. You can "Commandeer" this revision from the web interface if you want
to become the owner.

Update this revision anyway? [y/N]

I think the "suggest an update" workflow discussed in T10584 is probably the right pathway forward in the long term, but that's realistically tons of work (compared to this change, at least). This workflow still provides a fairly substantial amount of resistance for the "soul-crush the new hire" scenario, so I don't think it's going to cause any real harm unless people are already going pretty far out of their way to be relatively hostile (at least, as I think about it), in which case it's only making an already-bad scenario very slightly worse. It's not like we're adding a shiny new "be a jerk" button with a sparkle animation to the web UI or anything that even the nicest engineer couldn't resist clicking.

So I'm overall OK upstreaming this prompt in the short term, with the caveat that this workflow may become suggest a change to the author instead of immediately apply a change eventually.

(And, yeah, that test failure is unrelated -- just bad error messaging in Drydock that I haven't fixed yet.)

This revision now requires changes to proceed.Mar 14 2016, 6:26 PM

For posterity, the "upstream approved" use cases for this feature are things in this vein:

Open-Source Contributors: Contributors often submit small "near miss" patches and may not be prompt about responding to feedback. It's better for everyone involved if an experienced engineer takes responsibility for the change in effect, finishes it, and lands it, but the contributor retains credit (i.e., an actual "Commandeer" would complicate the process and steal credit to no benefit, since one goal of processes like this is to make contributors feel welcome and rewarded).
Interns: Interns or other inexperienced contributors (particularly in remote, part-time, or limited-timeframe situations) may have difficulty with complex operations like rebases or merges, and teaching them how to rebase might not be the best use of program time (and can be frustrating for everyone involved). As above, having a mentor push the change over the last hurdle while retaining full credit for the intern is the best outcome for all parties.

Use a prompt instead of a config setting.

In D15468#178369, @epriestley wrote:

For posterity, the "upstream approved" use cases for this feature are things in this vein:

Open-Source Contributors: Contributors often submit small "near miss" patches and may not be prompt about responding to feedback. It's better for everyone involved if an experienced engineer takes responsibility for the change in effect, finishes it, and lands it, but the contributor retains credit (i.e., an actual "Commandeer" would complicate the process and steal credit to no benefit, since one goal of processes like this is to make contributors feel welcome and rewarded).

Interns: Interns or other inexperienced contributors (particularly in remote, part-time, or limited-timeframe situations) may have difficulty with complex operations like rebases or merges, and teaching them how to rebase might not be the best use of program time (and can be frustrating for everyone involved). As above, having a mentor push the change over the last hurdle while retaining full credit for the intern is the best outcome for all parties.

This sounds reasonable. I've converted from using a config setting to using a prompt, with the wording you suggested and the answer defaults to no. I used an exception to abort the workflow if the user chooses no, hopefully that's the right way to do it?

Very minor nitpicks:

Maybe quote the D123 part, too: "D123: xxx" instead of D123: "xxx".
Maybe split the explanatory text and the question apart, and do this:

echo tsprintf(
  "%s\n",
  pht('You do not own ... big block of explanatory text ...'));

$prompt = pht('Update this revision anyway?');
$ok = phutil_console_confirm($prompt, ...);

The advantages to this approach are: if the user types "q" or "zebra" or something, only the question re-echoes which I think is a little easier to read; if the user creates a revision with ASCII terminal control characters in the title (bold, colors, etc), this will properly escape them by sending them through tsprintf().

I think phutil_console_confirm() adds [y/N] for you -- does this double-render it?
You can just throw new ArcanistUserAbortException(); in this case, with no message. It renders a simple "You chose not to continue." sort of message.

This revision is now accepted and ready to land.Apr 8 2016, 9:57 AM

greggrossmeier added a subscriber: greggrossmeier.Apr 8 2016, 2:31 PM

20after4 closed this revision.Apr 9 2016, 4:57 PM

epriestley mentioned this in T12242: arc commandeer.Feb 10 2017, 5:27 PM