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

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

If it's helpful, I think it's reasonable to treat "add working copy changes to an existing commit" and "update the message of a commit" as two different operations, even if they both run mostly the same code. I believe we only care about "add working copy changes" after we make them ourselves (e.g., by applying lint patches) and maybe there's an upfront prompt at the beginning.

I think we should never reach this method with a "bad" working copy state (that is, with any changes in the working copy which we don't want to combine into the commit we're amending), except that there may (?) be untracked files. If we can today, we will no longer be able to after arc diff updates to use ArcanistRepositoryLocalState. So the stashing should be unnecessary, I think -- but maybe you found a route to get here with a dirty working copy.

I also imagine updating arc diff to work like arc land does (update the revisions for a list of one or more groups of commits, i.e. possibly update multiple revisions if you arc diff change4 and have made local changes to local commits it depends on that are associated with other revisions). I'm not sure if that will actually happen given my actual velocity, but Git will likely need a similar update if it does (to reach into history and amend commits not in the working copy). But this seems like a straightforward step toward that, regardless of where things go.

If the non-evolve workflow is a pain, I think it's also reasonable for arc to exit when you try to do this and tell you to enable evolve or use a simpler workflow -- that's no worse than what it does today, right?

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

If it's helpful, I think it's reasonable to treat "add working copy changes to an existing commit" and "update the message of a commit" as two different operations, even if they both run mostly the same code. I believe we only care about "add working copy changes" after we make them ourselves (e.g., by applying lint patches) and maybe there's an upfront prompt at the beginning.

I think we should never reach this method with a "bad" working copy state (that is, with any changes in the working copy which we don't want to combine into the commit we're amending), except that there may (?) be untracked files. If we can today, we will no longer be able to after arc diff updates to use ArcanistRepositoryLocalState. So the stashing should be unnecessary, I think -- but maybe you found a route to get here with a dirty working copy.

I didn't come across this through testing, but after I put up these changes I remembered I needed to look at other callers to amendCommit() aside from just the one I was aiming to address with arc diff changing the message after creating the revision. When finding other call sites that don't change the message I realized that the only other reason to call that method (at least for Mercurial) is to amend with changes, e.g. arc lint. Even in the case where it's being called intentionally to put in changes that are intentional, if it's not a head revision there are still potential conflicts that could occur with descendant revisions.

If we can today, we will no longer be able to after arc diff updates to use ArcanistRepositoryLocalState.

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

I also imagine updating arc diff to work like arc land does (update the revisions for a list of one or more groups of commits, i.e. possibly update multiple revisions if you arc diff change4 and have made local changes to local commits it depends on that are associated with other revisions). I'm not sure if that will actually happen given my actual velocity, but Git will likely need a similar update if it does (to reach into history and amend commits not in the working copy). But this seems like a straightforward step toward that, regardless of where things go.

I've been thinking about this too. It actually came up in discussions with colleagues today. We had someone who lost work because their process is: create diff, strip, patch down, make changes, update diff, strip, etc. When working with a set of dependent diffs they forgot to update every single revisions and were surprised to find out that when they stripped everything and re-patched that it was gone (and had conflicts applying all revisions because of that). To be clear this is not a process I have ever recommended or condoned, though apparently it's being recommended by several developers.

I think it's something I'd be interested in trying to implement though it might be ambitious for myself.

If the non-evolve workflow is a pain, I think it's also reasonable for arc to exit when you try to do this and tell you to enable evolve or use a simpler workflow -- that's no worse than what it does today, right?

Correct the fallback here would be the same existing behavior. If this becomes too tedious I will probably take this route, though I think one beneficial path that can be added is to still allow the non-head amend if it's only to change the commit message.

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

Sort of: the current behavior of arc diff is to refuse to run with dirty/unsaved state. If you try it, you'll be told to revert/commit your changes or prompted to amend them into the current commit before continuing. This happens immediately, before we do any interesting work.

We must do this (guarantee the working copy isn't dirty) because even if arc diff could technically run in a dirty working copy, the output of arc unit isn't meaningful if you run tests against some state which isn't the state you're sending for review. If we continue with a dirty working copy and then produce some kind of result out of arc unit like "tests passed", we can't really claim that the tests actually pass against the code we're uploading, since we can't tell the unit test suite to use the state in commit abcd1234 instead of the actual working copy state. Running with a dirty working copy voids every kind of assertion or guarantee we're trying to make in operations that work against the working copy (arc lint, arc unit today; maybe arc rebuild-artifacts-after-rebase later) even if we're able to do it in a technical sense.

And it's super hard to lint and amend lint changes in a working copy with random other dirty change, since a lot of linters only accept working copy state as input rather than, e.g., data on stdin.

The possible exception to to this is that arc diff may let you run with untracked files. I don't remember what the current behavior is: historically, users want to run with untracked files. I think this is easily in the top 5 list of the worst things users want to do. Different versions of arc have permitted or prevented this to varying degrees and I don't remember what the current behavior is, but I think it's no weaker than "prompt to continue".

The most recent code for "shove all the working copy garbage into the corner before we do anything", ArcanistRepositoryLocalState, used in arc land, can completely shove everything into the corner, including untracked files (git stash push --include-untracked).

arc diff doesn't use this most-powerful effect (ArcnaistRepositoryLocalState) to sweep the working copy under the rug yet; it uses older code in requireCleanWorkingCopy(). It looks like the behavior may be to ignore untracked files with a dumb garbage flag, --allow-untracked, which dates from the long-ago before-times. That's definitely getting nuked if arc diff gets any work.

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. The actual behavior should pretty much be this today, just with some asterisks around how we actually make that guarantee and what happens with untracked files.

To be clear this is not a process I have ever recommended or condoned, though apparently it's being recommended by several developers.

At first glance, this workflow seems bad-with-no-advantages to me, but who knows. A popular Git flow is "stacked diffs" which I also think is bad-with-no-advantages. My workflow has been essentially unchanged for 10 years, except that arc now gets a more of it right automatically more often.

A lot of the forces around workflow selection are wholly opaque to me and I don't think I've ever really convinced anyone that my workflow is an improvement over whatever they're doing in many years of discussing things.

I don't think I've ever convinced anyone who likes keeping untracked files in their working copy that they shouldn't do that, except maybe one guy at Facebook who completely broke production by making an untracked-file error about 20 minutes after I tried to convince him that his workflow was bad and prone to those specific kinds of preventable error.

I don't think I've ever convinced anyone who likes using "git push" to "save changes" into the remote that it isn't a great idea.

I don't think I've ever convinced anyone who insists on running git rebase on every local change after every commit, and running arc diff after every git rebase, that there's no benefit to doing that.

I think one of the most popular extensions for Phabricator causes Jenkins to post hundreds of spam comments on every revision.

Companies routinely deploy stateful services on autoscaling containers and apparently have no capacity to operate even a minimally stateful service.

Happily, I no longer have to try to deal with balancing these concerns.

I think all arc diff flows where you're affecting more than one revision and that revision isn't exactly the current working copy state are bad, but imagine aligning behavior with arc land just for purity/consistency.

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*

    C'
    |
    B'
    |
    A'
   /
o  master

Assuming any dirty changes are produced by arc and are cleared for amending, those changes will need to be amended to A' however the first step, rebasing to create a copy of A, will fail if the working directory isn't clean. I think I have to tuck away the lint changes until after the first rebase so they can be untucked and amended to A' while it's still a head.

Though drawing all that out I realize my current approach has the edge case where A is the first changeset in a repository.

At first glance, this workflow seems bad-with-no-advantages to me, but who knows. A popular Git flow is "stacked diffs" which I also think is bad-with-no-advantages. My workflow has been essentially unchanged for 10 years, except that arc now gets a more of it right automatically more often.

I think my workflow is very similar to yours - single continually-amended commit per conceptual/effectual change - and I'm guessing that may be why I haven't run into many issues. Despite writing up a KB outlining my entire process, configuration, etc. and trying to share it and refer to it when people run into issues, I still come across interesting workflows. I think the constant strip thing is either, developers who don't like having multiple local heads of development, or developers who only want to use arc for the nice commit messages (tbf really nice).

Are "stacked diffs" effectively dependent revisions? I usually avoid that scenario where possible but it does come up on occasions, usually to make code review easier by splitting a refactoring out from feature changes.

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

In Git, we'd always run amend first (creating A', with a new message that includes any dirty working copy changes), then rebase from there. You can't (or can't easily?) change messages with a git rebase -- and git is fine with you amending any commit anywhere in history, it just creates a new commit floating out in space -- so the "create A'" and "rebase children" steps would have to be separate.

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.

Are "stacked diffs" effectively dependent revisions?

They're the "Facebook" workflow described in the bottom half of the document: the good workflow (local dependent revisions), except you put everything on one branch named feature (a big stack of commits under a single label, corresponding to multiple revisions) and use git rebase -i (walk through history, stopping the working copy at arbitrary points) any time you need to update things. In the abstract, they're the same workflow, but the "stacked diffs" workflow is weirdly cumbersome and obtuse for no reason that's clear to me.

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

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

$ hg commit --amend
abort: cannot amend changeset with children
$ hg --config experimental.evolution.allowunstable=True --config experimental.evolution.createmarkers=True commit --amend
1 new orphan changesets

It otherwise seems somewhat possible to call cmdutil.amend() and skip the check in an extension, although that also skips a bunch of other code and I'm not sure if the other code does anything.

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)

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?

I'll diff up the changes because it still works for the other cases

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

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.

I also think aiming toward "arc requires evolve to work with Mercurial" is a reasonable approach to consider.

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

(Yeah, I just went spelunking in the source code.)

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.

I also think aiming toward "arc requires evolve to work with Mercurial" is a reasonable approach to consider.

I'm on board lol. It's probably a little ways off -- evolve still does not officially ship with Mercurial.

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.

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.

I’ll do some experiments with this

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

I added some modification to the extension to get this working

cmdtable = {}
if "command" in registrar:
  command = registrar.command(cmdtable)
else:
  command = cmdutil.command(cmdtable)

But then ran into the same issue with remoteopts used by arc-ls-markers which I think was changed around the same time so I made a similar modification

if "remoteopts" in cmdutil:
  remoteopts = cmdutil.remoteopts
else:
  remoteopts = commands.remoteopts

But then I got another issue which I wasn't sure was worth continuing down this path, argument of type 'module' is not iterable -- nothing immediately stood out.

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.

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.

(Agreed on this.)

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.

This behavior is ultimately caused by a limitation of Git 2.21.1 -- we'd prefer to do what Mercurial does, but can't in older Git:

// NOTE: We'd prefer to "git stash create" here, because using "push"
// and "pop" means we're affecting the stash list as a side effect.

// However, under Git 2.21.1, "git stash create" exits with no output,
// no error, and no effect if the working copy contains only untracked
// files. For now, accept mutations to the stash list.

So this could be prevented more robustly by using git stash create in recent git (and returning a meaningful $ref like in Mercurial) and a synthetic stack-based reference in older Git.

I think I figured the chance anyone would save and restore local states in a non-stacked way was smaller than the cost of all this complexity.

(I'm fine with whatever level of guard rail you want to put on it, though.)

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.

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.

That all looks good to me.

Adding some feedback regarding these changes (I should’ve made a task for this first). Last week I had two developers separately approach me with issues they ran into with arc diff. I had them upgrade to the master branch and this resolved their issues.