Thanks! Couple of minor inlines but I didn't catch anything substantial.

On a non-head dirty commit I ran arc diff --trace and could confirm these ran properly

$ hg --encoding utf-8 --config 'extensions.arg-hg=/Users/cspeckrun/Source/phacility/arcanist/support/hg/arc-hg.py' arc-amend --logfile /private/var/folders/cg/364w44254v5767ydf_x1tg_80000gn/T/cglmx89uf3ks4gow/45330-yqYFDu

Simplify some of the code by splitting out the bit which produces the flag to enable the extension and the bit that combines everything into an array of arguments to be passed into e.g. execxLocal().

Okay I think this is ready for review. I'm not too certain I'm handling the command pattern population properly but I learned more about PHP reflection.

This happens when a recipient list includes an Owners package which has been destroyed. Specifically, we'll exit this section of PhabricatorMetaMTAMemberQuery with out the PHID in $package_map, and then fail to return it:

Oh, maybe I'm mixing up running arc diff B against running plain arc diff while being on B

Marking this as resolved by D21700. If there are further details or input we can re-open to further address.

Update formatting

There a bit of fancier indent/formatting stuff in some of the newer code (e.g., in ArcanistRefView->newLines()) but it hasn't really generalized into something you could easily apply here yet. I think this is fine for now and we could revisit it to make it fancier later on if the newest display stuff gets generalized a bit.

Maybe the output could be two separate lines:

Launching editor ("nano")...
Supply commit message for uncommitted changes, then save and exit.

Then neither part needs to care about the other part, and things are likely more translatable and such.

Yea that works a lot better. Though I'm on the fence about the order of the two messages. Logically my brain wants the "Launching..." message to be second but I don't think it really matters

Update the display format to not try and combine two things

Maybe the output could be two separate lines:

That all looks good to me.

Remove the stdout text matching, I confirmed that using arc diff with uncommitted changes on a head changeset properly amended the change into the changeset.

Revert changes to GitLocalState

If the git changes are too much I can pull those out- I think a longer-term solution will be to do version-checking for whether git stash create is supported and use that instead but I can do that in a separate change.

I really like that idea. I generalized the comments on the methods and then added a stack to GitLocalState. I ran two basic tests for this, running arc diff with uncommited changes and running arc land with uncommitted changes. For arc diff it properly amended the uncommitted changes into my diff (unsure if stash is used here), and for arc land it properly stashed my change during the land and restored it afterwards.

Update comments, include an internal stack in ArcanistGitLocalState for tracking the order of save/restore

We could conceivably prevent that error by making GitLocalState keep a trivial stack (just a stack of true) and return its current depth (0, 1, 2, ...), then throw if the passed $ref was not the current stack depth.

I'm also surprised that rebase extension isn't enabled by default. I guess I have been turning it on in my default setup.

I like that idea. I'll take a look.

Since modern Arcanist usage with Mercurial has required the arc-ls-markers for a while now and nobody else has reported issues, I think it's fine to not try to address these issues for older versions.

...perhaps we should consider a way to do this automatically or at least less-manually.

This makes me think that in this second scenario where running arc diff initially includes multiple commits that the revision should be updated to include the commit hashes for each, and then might result in having better detect this scenario if it's searching for revisions with associated commit hashes.

From https://secure.phabricator.com/D21680?id=51633#inline-60703

Still, I'd expect arc land <head2> to look at the ancestors between head2 and master, find the "Differential Revision" commit, and then figure out that "head2" can only be part of that revision, possibly prompting you ("Local range includes commits which don't match the revision, are you sure you want to land them?").

Fix up comments

Updating arc-amend to support some older versions of Mercurial

Remove unused import from arc-hg.py

I went back to test this with Mercurial 4.0 just as a test and the extension file fails to load properly. I traced the issue down to this issue
https://stackoverflow.com/questions/52117289/simple-mercurial-extension-fails-to-import

Use arc-amend instead of a long Mercurial dance

In D21686#276011, @epriestley wrote:

Sounds good. I spent 15 seconds on trying to write hg arc-amend, and this does something and does appear to produce an amended commit that looks ballpark-correct (and can amend non-heads), but emits some warnings and I have no clue if it functions across Mercurial versions:

diff --git a/support/hg/arc-hg.py b/support/hg/arc-hg.py
index 01ac3907..82202767 100644
--- a/support/hg/arc-hg.py
+++ b/support/hg/arc-hg.py
@@ -28,6 +28,12 @@ _ = i18n._
 cmdtable = {}
 command = registrar.command(cmdtable)
 
+@command(
+  b'arc-amend')
+def amend(ui, repo, source=None, **opts):
+  cmdutil.amend(ui, repo, repo[b'.'], {}, [], opts)
+  return 0
+
 @command(
   b'arc-ls-markers',
   [(b'', b'output', b'',

Still, it seems possible that this is the least-messy pathway for non-evolve support since we need the extension anyway and all the "can't amend non-head" stuff seems advisory rather than technical, given that flags exist to disable the checks and the low-level call seems to produce the desired repository state.

Fix some typos and comments before @epriestley spots them

Added a workflow argument to the amendCommit() path so it can be set on the local state for logging purposes.
Fixed the issue with the terminal, Ph'nglui mglw'nafh Cthulhu R'lyeh wgah'nagl fhtagn!

• Cryptographically Random Orbit Sander

Sounds good. I spent 15 seconds on trying to write hg arc-amend, and this does something and does appear to produce an amended commit that looks ballpark-correct (and can amend non-heads), but emits some warnings and I have no clue if it functions across Mercurial versions:

Given the mess of dealing with the non-evolve case, what do you think about splitting this into a smaller "make this work under evolve" change and landing that now (which should solve everything for your org, since everyone has evolve?) since all that code looks good to me, and then maybe revisiting the non-evolve case if it other users hit it and/or diff gets some work and it's easier, and/or we figure out a reasonable way to hg force-amend or do some other less-tortured operation here.

Unfortunately not everyone at my org uses evolve, in fact most don't. I've never gotten a solid reason for why but I suspect it's just "new" and frightening. I started looking at this change initially because someone ran into an error when trying to arc diff a non-head revision. I'll try to investigate what's happening - it totally hoses my console and I can't scroll up to view the history when using --trace so I'll have to pipe the output to a file or something. If I can't figure it out I might just throw an error stating that evolve is required here when trying to amend changes to a non-head commit. I'll extract out the non-evolve-local-changes amend to a separate function.

My only guess is you've delved into the Mercurial source base (I've poked around a little)

I don't think LocalState should z̵̰̲͚͖̊͗́̓̓a̷̛̱̯̠͐̿̅̅͛̓͗ĺ̵̨̻̠͓̼̗̥̥̻̲̳̎͜ͅg̴̛͙͗̀̄̈́̃̂̓̄͝o̶̥̘̻͙̬͇͚̥̪͌̔̃͛̆̀̂̚ͅ your terminal!

Attempt to stash unsaved changes before doing the rebase dance. Not currently in a working state.

You can probably stash/restore by using ArcanistMercurialLocalState, although I'm not 100% sure it's reentrancy-safe and I'd imagine arc diff may already be holding a LocalState object somewhere above this callsite in the future.

Might that manifest in ways like this?

Holy moly I'm not sure I would've found that out lol. My only guess is you've delved into the Mercurial source base (I've poked around a little)

This is probably not a great path to walk down, but there are secret flags that disable the "cannot amend changeset with children" check:

Another possible approach might be to copy the amend extension to create hg arc-amend which just lets you amend anything, assuming the "head only" rule is a safety guard rail rather than a fundamental issue with Mercurial (which it seems like it should be).

Ohhhhh, sorry, I didn't connect the dots and understand that you're only sometimes running the amend (since you can't amend for non-heads).

The desired behavior is that you can not enter any interesting workflow in arc diff if your working copy is dirty, so amendCommit() "should" be able to safely assume that any working-copy dirtiness has been made by arc, likely in arc lint, and is appropriate to amend into repository state.

Ah the stashing I'm thinking needs done is due to how (I think) amending a non-head changeset without evolve has to happen.

C
|
B
|
A* (changes from lint, want to amend)
|
master

Create a copy of A with the new commit message

C
|
B
|
A
|
A*  A'
|  /
o  master

Rebase B:: onto A'

    C'
    |
    B'
    |
A*  A'
|  /
o  master

And then strip A*

This ("Queued at") looks suspicious:

I'm not catching on to what you're referring to. Are you saying there are other changes which will have already stashed unsaved state away before calling amendCommit()?

Just for completeness for future readers: this behavior depends on history.immutable today, but arc would generally like to do this and Mercurial users broadly seem more open to treating local history as mutable today than they did in ~2011 (as does Mercurial itself, with changes like evolve).

Would this be good to comment on ArcanistMercurialAPI::amendCommit()?