I think the best fix here is to let arc land land something onto itself. At least in Git, I think there's no technical reason we can't do this, it just creates a bit of a complicated mess and we may end up in a state that's sort of hard to restore from. I think the error message which blocks this (at least in Git) was motivated more out of "It's not worth making this twice as complex to address a bizarre edge case workflow which rarely arises" than "users shouldn't do this / this isn't possible". The Mercurial case may be more subtle.

We shouldn't prevent arc amend on master, nor arc diff from master. There's definitely no reason to prevent these; they work fine and are useful in plenty of situations. I use both with some regularity.

Another attack on this would be to change the "Next Step" advice that Differential gives. For example, we could simply recommend "arc amend" if the branch is "master", which theoretically is good enough since the output of "arc amend" tells you to push. This is probably reasonable-ish in the original spirit of "Next Step" as advice/hint text, but it seems users often interpret it as an immutable decree. Detecting what branch is "master" is also a bit tricky, since it could change between the diff step and the amend step. "arc land" already tries to redirect here (saying "use arc amend instead") but apparently that's not effective.

I think the "autobranching" stuff isn't desirable: it's very roundabout, and it means that failure will leave the working copy in a substantially different state than it started. We've generally moved toward trying to keep the working copy in the same state after failure, even at the expense of destroying work performed while merging. See some discussion in T2945. It should also be unnecessary -- at least in Git, I believe we can execute both the squash and merge strategies without branching.

Broadly, this task seems kind of fluffy. In particular, this step seems somewhat dramatically presented compared to the actual behavior:

• "figure out how to fix this mess and get my commit landed"

The "mess" is actually this message:

Usage Exception: You can not land a branch onto itself -- you are trying to land
'master' onto 'master'. For more information on how to push changes, see
'Pushing and Closing Revisions' in 'Arcanist User Guide: arc diff' in the
documentation. You may be able to 'arc amend' instead.

The referenced document is the first hit on google for the text and describes all the options in pretty good detail. The hint at the end ("arc amend") is likely the right thing to do. The text from that command tells you to git push.

So, overall, I'm not sure this is worth the additional complexity of making arc land work from a branch onto itself. The technical implementation seems somewhat complex and somewhat messy, and the mess seems fairly easily resolvable by reading the error message and documentation. As far as I'm aware, no one else has run into difficulty with this.

In the absence of other context, my inclination would be not to change this for now, and to wait and see if other users run into similar issues.

What do you think?

Man I wish I talked to you sooner :) I agree with you 100% that landing onto yourself should be allowed. That was actually my original approach, but was told that we should do this instead. I think this came from the fact that we weren't sure if having master be special was some fundamental design choice or invariant and changing it could have far reaching consequences.

However, "arc amend" still leaves you in a state where "arc land" will not work. Are you recommending that after "arc amend", you do git push? That seems to undermine "arc land" and all of it's goodies.

With that said, I do think this is an issue. As someone who just started using this tool (from a git background), this was very confusing to me and got me. It also got other newbies as well. If you mess up (and don't create a branch or use arc feature), then you have to do something like I did above to be able to use arc land (at least as far as I can tell). Hence people may find it easier to just push the changes and not run arc land instead. Also, it puts less assumptions on using the tool, which I think would make it easier to use.

As far as I can tell technically, this is done so master can be used as a tracking branch (where the server is). This could possibly be useful for source control agnosticism, but in git it seems like it would just be better to use the origins, since that is essentially what they are there for. AFAIK, Mercurial doesn't seem to have a similar concept (maybe you may be able to query where the server's master is, but I am not sure), so you may actually need a master branch like this for some of the operations that are done, but I could be really wrong.

But I agree, this seems like a big risky change from looking at the code so I don't think it is in the scope for bootcamp or someone who is new to the code base.

With that said would you be willing to put the "fix up" code into arc land only. That way if you are on master it doesn't block you. We could create a temp branch name and no one would know. However, as you said it does do a series of big operations on your repo which could be confusing, especially if you the script was cancelled or errored at a perfectly bad time.

I run into this problem every now and then. I've wondered the same thing as to why it lets me get all the way through the process if I'm not allowed.

Two thoughts I had after reading your reply:

1. An arcconfig option to disallow it
2. A simple confirm message alerting the user to continue

I most commonly encounter this when I've gone and done some work, got a code review, and realised I forgot to run arc feature blah before committing originally. In order to fix this, most of the time I just create and checkout a new branch at my current commit, reset master to origin/master and do arc land. Personally I'd prefer a syntax like arc feature --retroactive blah that does the previous steps for me, since @epriestley is correct; sometimes arc land doesn't work correctly and you need to get back to a sane state (and landing something onto itself and going wrong will be horrible to get back from since you'll need to look at the reflog in Git).

However, "arc amend" still leaves you in a state where "arc land" will not work. Are you recommending that after "arc amend", you do git push? That seems to undermine "arc land" and all of it's goodies.

Yeah -- arc amend; git push (or arc amend; git pull --rebase; git push, or whatever). This is more or less what arc land does.

I think partly this is a matter of perspective, perhaps: arc land originally came into existence as a very thin wrapper around VCS commands, and is still a fairly thin wrapper, at least in a technical sense (the only magic it really adds nowadays is a couple of sanity checks). But it's obviously much easier for users to think of it as magic-which-does-the-right-thing than to understand what's happening under the hood. As a wrapper around common VCS operations the current behavior isn't particularly unreasonable, but as "the command you run to commit code" it falls short.

But I agree, this seems like a big risky change from looking at the code so I don't think it is in the scope for bootcamp or someone who is new to the code base.

Yeah, I tend to agree. We also don't have any real workflow-level test coverage (and it would take a bit of infrastructure work to build it) so it's a fairly fragile change, too.

With that said would you be willing to put the "fix up" code into arc land only.

I think we should avoid that approach. I think a better approach in Git for the squash strategy is something like this:

git pull --rebase # Pull in new changes.
git commit --amend # Update commit message.
git push
# recover with:
git reset --hard <original hash> # Put master back the way it was.

For the merge strategy, something like this:

git reset --hard <the upstream> # Reset master to the origin. This kicks any local changes off master.
git pull # Pull in new changes.
git merge --no-ff <original hash> # Merge the original commits to master.
git push
# recover with:
git reset --hard <original hash> # Put master back like it was.

A disadvantage to this approach is that fataling in the middle will "lose" the user's work if they don't know how to use git reflog. We could take a more roundabout approach like this:

git fetch <the upstream>
git checkout <the upstream>
git merge --no-ff <original hash>
git checkout master
git reset --hard <the merge hash>
git push

The worst this does is leave the user in a disconnected state, although to recover we have to switch back to master in addition to resetting.

For Mercurial, I'd have to look at it more.

Neither of these strategies can share much code with normal arc land, but I think they should be easier to implement and recover from (and generally less confusing) than creating ephemeral branches and then immediately destroying them. The squash version is pretty clean; the merge version is messier but still seems less messy to me than creating named branches.

@epriestley, your comment about arc land falling short as "the command you run to commit code" is dead-on (I'm the original author of this report).

I actually was not aware that it was possible (or advisable) to run git push manually. The training for the tools and repository always say to use arc land. I've been using Git for six years, but I've never had the inclination to dig into how the arc stuff works under the hood and potentially screw up a repository used by hundreds of engineers. (I use the repository very infrequently -- this would be different if I used it daily.)

So, maybe this is training/messaging issue, but I see arc land as even more than magic-which-does-the-right-thing: it's the way to commit code. Thus, when it doesn't work, I assume the fix is to make it work (rather than working around it by pushing directly).

For the infrequent user, making arc amend disallow committing on master would basically solve this.

(+ @durham / @zeeg)

When this gets touched, it would be a good time to support:

• arc land in Mercurial from a bookmark to a branch.
• arc land in Mercurial from a branch to a bookmark.
• In Git, the rebase workflow should rebase before testing for multiple revisions on the branch.

I'm using mercurial and it's painful to watch arc land fail when using anonymous branches.
An anonymous branch is a mercurial basic concept: you can have multiple heads for the same branch name.
Here: 13945 and 139647+ are both heads in "default" branch.

139644 and 139645 are both in a differential revision which has been approved.
However, when I ran arc land, I got :

D:\repo>arc land --preview
Landing current branch 'default'.
Usage Exception: You can not land a branch onto itself -- you are trying to land 'default' onto 'default'. For more information on how
to push changes, see 'Pushing and Closing Revisions' in 'Arcanist User Guide: arc diff' in the documentation.

D:\repo>

I've tried to use --onto and --revision, also manually updating the repo. But arc land stops anyway.

Therefore :

• It seems i can only use arc land when merging into another branch ;

Thus it imposes me to develop in a named branch, which is not the same in mercurial compared to git.
Branches are more meaningful when lasting in mercurial ; I don't want to use them for 3 commits in a row...
Branch name stays forever in the commit.

• I have to manually merge and push (or rebase + arc diff + push) to close the revision : arc land is useless.

In conclusion : it would be nice to be able to arc land default onto default with mercurial.

@TiTi - are you able to use hg bookmarks as a workaround for the time being? They are not tracked in the hg commit objects just as in git, so you should be able to bookmark your branches and arc land them that way.

Hi,

@cspeckmim it works but only by :

• adding a bookmark to the latest commit in the review
• adding a bookmark for the most recent commit in default, which is useless (There's always only 1 head for defaultbranch on the repo i'm pushing).
• using --squash

Here it is:

hg bookmarks --rev default myHead
hg bookmarks arc-patch-D23
arc land --onto myHead --squash

Full tests:

(already got a differential with 2 commits)

D:\Dev\test>arc land
Landing current branch 'default'.
Usage Exception: You can not land a branch onto itself -- you are trying to land 'default' onto 'default'. For more information on how
to push changes, see 'Pushing and Closing Revisions' in 'Arcanist User Guide: arc diff' in the documentation.

D:\Dev\test>hg bookmarks arc-patch-D23

D:\Dev\test>arc land
Landing current bookmark 'arc-patch-D23'. [Edit: so the bookmark is detected]
Usage Exception: Source arc-patch-D23 is a bookmark but destination default is a branch. When landing a bookmark, the destination must
also be a bookmark. Use --onto to specify a bookmark, or set arc.land.onto.default in .arcconfig.

D:\Dev\test>arc land arc-patch-D23
(same as previous)

D:\Dev\test>arc land --onto default
Landing current bookmark 'arc-patch-D23'.
Usage Exception: Source arc-patch-D23 is a bookmark but destination default is a branch. When landing a bookmark, the destination must
also be a bookmark. Use --onto to specify a bookmark, or set arc.land.onto.default in .arcconfig.

D:\Dev\test>arc land --onto tip
Landing current bookmark 'arc-patch-D23'.
Usage Exception: Source arc-patch-D23 is a bookmark but destination tip is a branch. When landing a bookmark, the destination must also be a bookmark. Use --onto to specify a bookmark, or set arc.land.onto.default in .arcconfig.

D:\Dev\test>hg bookmarks --rev default myHead

D:\Dev\test>arc land --onto myHead
Landing current bookmark 'arc-patch-D23'.
Updating myHead...
The following commit(s) will be landed:

a891f9f13b2f Fix 2/2
8c38dca195c1 Fix 1/2

Switched to bookmark arc-patch-D23. Identifying and merging...
Landing revision 'D23: Testing arc land default onto default with bookmarks'...
Usage Exception: --merge is not currently supported for hg repos.

doh!

D:\Dev\test>arc land --onto myHead --squash
Landing current bookmark 'arc-patch-D23'.
Updating myHead...
The following commit(s) will be landed:

a891f9f13b2f Fix 2/2
8c38dca195c1 Fix 1/2

Switched to bookmark arc-patch-D23. Identifying and merging...
Landing revision 'D23: Testing arc land default onto default with bookmarks'...
saved backup bundle to D:\Dev\test\.hg\strip-backup/8c38dca195c1-backup.hg
merging Text.js
Pushing change...

pushing to https://127.0.0.1:3000/test/
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 1 changes to 1 files
Closing revision D23 'Testing arc land default onto default with bookmarks'...

Cleaning up feature bookmark...
Done.

D:\Dev\test>

So it works, but I will have a hard time convincing others to add a specific bookmark for each review, and also on the head.
This goes into the spirit of this issue I guess : ability to land on same branch.

--onto is configurable in .arcconfig so no need to specify it each time on the command line because the bookmark can be push/pulled across all dev. repo.
Thus reduce the command line to :

arc land --squash

A bookmark is automatically available on the reviewer repo when he does "arc patch D23". But not on the author side.
So it's easier to do arc land on the reviewer side.
But it does not correspond to the philosophy where the original author do the push (see Differential User Guide).

That's another issue but it seems merge and rebase aren't available with mercurial :-/
I do not usually wan't to --squash commits because it can be seen as a loss of information, amongst other things.
So arc land is pretty limited with mercurial for now :-/

Thanks. It's not too burdensome for me to just add a big block of text to our exceptions in the case of failure letting our users know what they should probably do. But the state you get put in after a failed land is pretty bad in some cases, especially if you are unfamiliar with arcanist.

At Dropbox, the data shows that in our major git repository, 2-3 users hit the "You can not land a branch onto itself" message per day — so this isn't an uncommon occurrence. Is there something we can do to help prioritize this task, such that arc land DWIM in more cases?

Yes: move it into the "Contract Fix" column on the Dropbox workboard.

@epriestley, do you have a time estimate of how long this would take to fix?

About an hour.

I'm planning to leave this open since it also contains a lot of discussion about Mercurial and none of the recent changes affect Mercurial (particularly, they do not resolve the branch/bookmark issues). Of relevance here:

• In Git, you can now land stuff onto itself.
• In Git, we are now aggressive about restoring the original working copy state after we encounter any failure.
• See T9657 for a complete followup on the changes to the Git workflow.

This appears to have broken a couple workflows that I would assume to be common. Using c844669, I now see the following:

master and origin/master are behind upstream, local branch from them fails to rebase

$ git log --graph --oneline --decorate -n3
* 1070ef0 (HEAD, some-local-branch) A local commit
* e5648a1 (origin/master, origin/HEAD, master) A commit which is no longer actually the tip
* 590a044 A previous commit

$ arc land
Landing current branch 'some-local-branch'.
 TARGET  Landing onto "master", the default target under git.
 REMOTE  Using remote "origin", the default remote under git.
 FETCH  Fetching origin/master...
This commit will be landed:

      - 1070ef0 A local commit

Landing revision 'D12345:  A local commit'...
 BUILDS PASSED  Harbormaster builds for the active diff completed successfully.
Exception
Local "some-local-branch" does not merge cleanly into "origin/master". Merge or rebase local changes so they can merge cleanly.
(Run with `--trace` for a full exception trace.)

$ git log --graph --oneline --decorate -n 3 origin/master HEAD
* 1070ef0 (HEAD, some-local-branch) A local commit
| * 8de8ec1 (origin/master, origin/HEAD) A new commit which is the tip
|/  
* e5648a1 (master) A commit which is no longer actually the tip

$ git rebase origin/master
First, rewinding head to replay your work on top of it...
Applying: A local commit

Master has a local commit

$ git --no-pager log --graph --oneline --decorate -n3 origin/master HEAD
* e27a3be (HEAD, master) A commit on master ahead of the tip
* 8de8ec1 (origin/master, origin/HEAD) A new commit which is the tip
* ea7cf0a A commit which is no longer actually the tip

$ arc land
Landing current branch 'master'.
 TARGET  Landing onto "master", selected by following tracking branches upstream to the closest remote.
 REMOTE  Using remote "origin", selected by following tracking branches upstream to the closest remote.
 FETCH  Fetching origin/master...
This commit will be landed:

      - e27a3be A commit on master ahead of the tip

Landing revision 'D12345: A commit on master ahead of the tip'...
 BUILDS PASSED  Harbormaster builds for the active diff completed successfully.
Exception
Local "master" does not merge cleanly into "origin/master". Merge or rebase local changes so they can merge cleanly.
(Run with `--trace` for a full exception trace.)

Let's look at your second one first since I think that's a little simpler. I can't immediately reproduce it. Here's what I did:

I made a commit with these contents:

$ git diff
diff --git a/florida b/florida
index fa9aeb4..3309aac 100644
--- a/florida
+++ b/florida
@@ -36,3 +36,4 @@ cascade2
 dirty
 cascade
 tip
+Testing T3855

$ git status
On branch master
Your branch is ahead of 'origin/master' by 1 commit.
  (use "git push" to publish your local commits)
nothing to commit, working directory clean

$ git --no-pager log --graph --oneline --decorate -n3 origin/master HEAD
* 4ff3aea (HEAD -> master) wip
* 4db317a (origin/master, origin/HEAD) Junk on readme
* 0569870 Junk on readme

Here's the output I get with some test data:

$ arc land --conduit-uri=http://local.phacility.com/ --revision 96
Landing current branch 'master'.
 TARGET  Landing onto "master", selected by following tracking branches upstream to the closest remote.
 REMOTE  Using remote "origin", selected by following tracking branches upstream to the closest remote.
 FETCH  Fetching origin/master...
This commit will be landed:

      - 4ff3aea wip



    Revision 'D96: Junk on readme' has not been accepted. Continue anyway?
    [y/N] y

Landing revision 'D96: Junk on readme'...
 PUSHING  Pushing changes to "origin/master".
Counting objects: 3, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (3/3), done.
Writing objects: 100% (3/3), 402 bytes | 0 bytes/s, done.
Total 3 (delta 2), reused 0 (delta 0)
To ssh://dweller@secure.phabricator.com/diffusion/GITTEST/git-test.git
   4db317a..ecbc350  ecbc35053b335f7a3b34ffa9b076e7466051c1df -> master
 RESET  Local "master" landed into remote "origin/master", resetting local branch to remote state.
 DONE  Landed changes.

This created ecbc35053b335f7a3b34ffa9b076e7466051c1df on this server.

Do you have any idea what I might be doing differently than you?

Aha -- I have merge.ff = off in my ~/.gitconfig, which causes:

alexmv-linux ~/src/test-repo ((8de8ec1...)) $ git merge --no-stat --no-commit --squash -- 'e27a3be0fcd8cd797faa0c3eb5c3845d14f0d5f1'
fatal: You cannot combine --squash with --no-ff.

Ah, okay. If we put a no-op "--ff" in that command, does it overwrite the configuration option and work locally?

Yes, an explicit --ff makes it complete:

$ git merge --ff --no-stat --no-commit --squash -- 'e27a3be0fcd8cd797faa0c3eb5c3845d14f0d5f1'
Updating 8de8ec1..e27a3be
Fast-forward
Squash commit -- not updating HEAD

Cool, that's an easy fix.

We previously had --ff-only on the squash merge which is why this used to work in the presence of merge.ff, but we were doing this weird rebase-then-merge dance that I believe is unnecessary and was originally motivated by trying to put the user in a "useful" working copy state after a merge failure so they could manually resolve it, and was possibly an outgrowth of whatever arc land did in 2011 before Mercurial support and options like history.immutable.

Since we just reset back to the original state on failure now, the internals of the actual merge should be moot (probably).

D14365, applied locally, does fix both issues.

Unfortunately, rejects from pre-receive hooks appear to get ignored (treated as success), and the commit thus gets lost:

$ arc land
Landing current branch 'local-branch'.
 TARGET  Landing onto "master", the default target under git.
 REMOTE  Using remote "origin", the default remote under git.
 FETCH  Fetching origin/master...
This commit will be landed:

      - 9482dde Modify me

    Revision 'D149175: Modify me' has not been accepted. Continue anyway?
    [y/N] y

Landing revision 'D149175: Modify me'...
 BUILDS PASSED  Harbormaster builds for the active diff completed successfully.
 PUSHING  Pushing changes to "origin/master".
Counting objects: 11, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (3/3), done.
Writing objects: 100% (3/3), 355 bytes | 0 bytes/s, done.
Total 3 (delta 2), reused 0 (delta 0)
remote: ========Running pre-receive hooks=========
remote: Exception: Push rejected!
To ssh://repo@tails.corp.dropbox.com/diffusion/TESTREPO/test-repo.git
 ! [remote rejected] d71987d6974e636fe6c547b4fe47a397657125e3 -> master (pre-receive hook declined)
error: failed to push some refs to 'ssh://repo@tails.corp.dropbox.com/diffusion/TESTREPO/test-repo.git'
 UPDATE  Local "master" tracks target remote "origin/master", checking out and pulling changes.
 PULL  Checking out and pulling "master".
Cleaning up branch "local-branch"...
(Use `git checkout -b local-branch 9482dde32f5cabe37687419b47a9f8c24c56dcf0` if you want it back.)
 DONE  Landed changes.

D14365 should fix that too, now. Landing in a sec...