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()?

After arc diff creates a revision in Phabricator it amends the commit to include a link to the revision in the commit message.

Clean up the logic/nesting by returning early where possible. Add TODOs for work to be done still.

This is also shedding light on T13107 :)

In D21685, I've imposed stricter rules for which state transitions are allowed: for example, you can't issue a "pause" command to a build that is already pausing.

I take it these disparate trees have some limitations? Do they always apply to the working directory or something?

In Git, we can just create new commits floating out in the middle of nowhere -- not on any branch, and not the ancestor of any labeled commit -- that you can retrieve if you know the hash but aren't otherwise reachable or visible to users without using very low-level commands. This isn't normally useful for human users, but it's great for reducing side effects in arc land, since we can do all our work in a sort of temporary scratch area that automatically gets cleaned up later.

Woah, yea I don't think there's anything like this in Mercurial. I take it these disparate trees have some limitations? Do they always apply to the working directory or something?

I'm still piecing together Git's handling of branches in my mind as I have used Mercurial for so long.

Just thinking aloud to myself

Off-hand I don't know of a solution that could provide that same guarantee for cherry-picking. Our current process also requires that the engineer who authored the fix is the one responsible for merging it, rather than having release engineers or an unlucky engineer responsible for performing all merges into the subsequent versions.

This could probably be handled by having the server-side hook check for commits with the same task number, as we require all commits to start with a task number. It's not as "comfortable" as ensuring the same changeset exists in the branch but since the changesets require merge commits which can potentially introduce incorrect merges that comfort might be just as misleading.

The commits created as a side effect of arc land --hold aren't actually deleted explicitly, but they're effectively gone in Git as soon as you update your working copy to any other state. I'm not sure how closely this holds (or can hold) in Mercurial.

Ah yea this is one difference with Mercurial and Git's treatment of branches. In Mercurial there is no automatic cleanup, so things must be stripped/pruned manually before they will go away -- regardless of whether they're tagged with a bookmark or not. I'm still piecing together Git's handling of branches in my mind as I have used Mercurial for so long. Conceptually I know how I want to manipulate the repository but then things happen which reveal that how I'm thinking the repository is laid out isn't what Git's doing (Arcanist has been very useful in this respect).

Would the expectation then be that running arc land again would remove the rebased/squashed commits created by --hold, so the result would be as if they hadn't run --hold previously?