Page MenuHomePhabricator

`arc land` detached head state with `--hold`
Closed, ResolvedPublic

Description

When doing an arc land of a local branch that was previously uploaded for review with arc diff, using the --hold option leaves you in a detached head state with your commit ahead of origin/your_landing_branch_here

List of steps to re-create here:

  • git checkout master
  • git pull
  • git checkout -b new_feature
  • vim testfile
  • edit some stuff
  • Git add, commit etc...
  • arc diff
  • Review (or not)
  • arc land --hold

arc land --hold --trace output can be viewed at P1906

Git history left in the following state (git log --oneline --decorate)

eb35b52 (HEAD) Test land --hold
e6ecc67 (origin/master, master) Changes to be landed
3f34ad8 test commit
b9aeefb Test change

Event Timeline

mrkanaly raised the priority of this task from to Needs Triage.
mrkanaly updated the task description. (Show Details)
mrkanaly changed the edit policy from "All Users" to "Custom Policy".
mrkanaly added projects: Bug Report, Arcanist.
mrkanaly added a subscriber: mrkanaly.

What do you expect it to do?

That is, why do you consider this behavior to be a bug?

I expect it to do what it used to do previously, which would be finish with a merge onto the desired landing branch. Rather now, if I use this, I have to do an additional

  • git checkout master (remembering the commit I am leaving behind)
  • git merge eb35b52

Then end with
git push

Additionally, the hold now does not cleanup the feature branch either

Why are you using --hold if your next steps are the steps that arc land would take anyway?

Well realistically, there more steps between the arc land --hold and git push

I am one of a few people who develop on some projects that have multiple submodules, all of which are reviewed.

So a typical change/task tends to look something like this

cd project_root
git co master && git pull
git co -b new_feature_project_code
cd lib/submod1/
git co master && git pull
git co -b new_feature_common_code
cd ../submod2/
git co master && git pull
git co -b new_feature_test_code
cd ../../ (back to project_root)

At this point, I've got local branches checked out in all the submodules, ready to work my changeset.

...develop...
...develop...
...develop...
...code...
...code...
...code...
..program to hardware...
...test...
...test...
...test...

So now, I start from the submodules, and create my reviews using arc diff, creating the review in the top-level repo last.

Then the team reviews the code, and I address any comments, adding new commits on local branches as needed.

Now, the greatness of --hold comes in for doing changes like this. Keep in mind that over the course of me developing the change, the master branches of each submodule and the top-level have progressed beyond where I branched away. So I arc land --hold every submodule, commit the new submodule revisions in the top-level, then arc land --hold the top-level. With everything landed, I want to verify that nothing was broken, and that the board still boots and runs. So I do a clean build of everything, bjam -a -j8 and re-program to hardware and double check my new change.

Then, I push each submodule, with the top-level pushed last.

Also, I noticed that the documentation for --hold is also out of date, in accordance with this bug report.

The change is merged into the target branch, following these rules:

In mutable repositories or with --squash, this will perform a
squash merge (the entire branch will be represented as one commit on
the target branch).

In immutable repositories or with --merge, this will perform a
strict merge (a merge commit will always be created, and local
commits will be preserved).

The resulting commit will be given an up-to-date commit message
describing the final state of the revision in Differential.

With --hold, execution stops here, before the change is pushed.

Since the change is not merge onto to the destination branch

The change is merged into the destination branch, but the branch reference is not updated to point to it. The detached HEAD you end up with is the correct state of a merge between the origin branch and the changes you are landing. The documentation could perhaps be more explicit about this.

You can continue from --hold with:

git push origin HEAD:master

...(where origin is the remote and master is the target branch), which is the next step arc land performs. This won't help with removing the feature branch or updating any local branches.

Your workflow is unusual, and at odds with a number of use cases described in T9657 and related tasks, which motivated the recent changes to arc behavior. These workflows include things like:

  • landing branches onto themselves;
  • having local master which does not track remote master;
  • having no local master;
  • landing some branch feature with unrelated, local changes in local master.

We can not accommodate these workflows and your workflow, because some of them rely on not updating the local master (or not even having a local master).

Although we could make --hold do cleanup, I think this risks being quite confusing and causing users to believe they have lost work if they aren't familiar with git reflog. I'd rather accommodate this through options to arc cleanup (see T3277).

epriestley claimed this task.
  • We can't give arc land the old behavior without breaking other (more common) use cases.
  • You can push directly from the detached head, per above.
  • arc help land is now more clear that merging takes place in a detached state and local branch references are not updated.
  • arc cleanup (T3277) may eventually make it easier to clean up these branches.
  • It sounds like the old workflow was still fairly labor intensive. Writing a script to help you manage submodules might simplify your workflow overall, as a possible workaround. This script could use slightly lower-level operations than arc land (like git operations and arc amend) to give you more control. arc land does not do anything magical, and is just intended as a convenience wrapper around merging in the most common cases.
joshuaspence renamed this task from arc land detached head state with --hold to `arc lan`d detached head state with `--hold`.Dec 12 2015, 11:24 PM
joshuaspence renamed this task from `arc lan`d detached head state with `--hold` to `arc land` detached head state with `--hold`.
joshuaspence updated the task description. (Show Details)
joshuaspence updated the task description. (Show Details)
joshuaspence updated the task description. (Show Details)
joshuaspence added a subscriber: joshuaspence.

I've often told people that they can use arc land --hold to inspect the changes before they are pushed (mostly for people who were not completely trusting of arc land. Whilst this is still technically true, I suspect that many people would not know what git push origin HEAD:master does (I personally had never seen this command before this task). It might be useful to output some sort of "Next Step" text when using arc land --hold.

Sure. The best "Next Step" is to just run arc land <branch> again without --hold, although we can be explicit about that and fill in branch in case it was run as plain arc land originally.

That's sort of messy since we should really echo all the other arguments, too, and can't necessarily reconstruct --trace and --conduit-uri and such very easily, and reconstructing --conduit-token is maybe sort of a bad idea.

epriestley triaged this task as Wishlist priority.Dec 13 2015, 12:10 AM

I believe this change will negatively affect the workflows we have at our workplace (when I return there I'll know for sure). Basically, we advise everyone to land with arc land --hold due to issues where interactions with msysGit and arc will cause commits to revert part of the branch (I can't find the task, it was filed ages and ages ago). So not being able to just check the landed commit to make sure it's okay and then hitting push will be a regression in the workflow that people are currently using...

Actually it looks like msysgit finally released Git 2.x for Windows, which is supposed to have fixed the race condition that was present on Windows in Git (that was supposedly the cause of the issue). When I return I'll get everyone to update in the hope that fixes the issue.

I am in the same boat as @hach-que right now. By destroying the original behavior, it really doesn't leave me any choices but to orphan everyone that I have trained to use arcanist on a very old version of the tool. I am 100% trusting of arcanist and its operations, but many people are afraid of tools like this. So they like options like --hold that allow them to inspect everything before they push it away. But now they would be instructed by the tool to run some funny command whose syntax they don't understand, and then also have to run

  • git checkout master
  • git pull

to get to where they wanted to be.

Personally, I really don't think my workflow is that uncommon. Anybody who might be developing with submodules would have almost the same flow. Arcanist perfectly complimented the workflow by doing exactly what it was advertised to do, simplify the amount of SCM work you have to do. Disappointingly, that is no longer the case.

I'm not open to reverting to the old behavior. The new behavior solves a large number of problems for many users, discussed in T9657.

If the only acceptable pathway forward for you is to retain the old behavior, you can lock Arcanist and Phabricator at 2015 Week 43 forever, which was the last stable cut before the changes.

Broadly, if you're using arc land --hold <feature> to preview changes, there are three ways to proceed:

  • Run the "push" command arc gives you (like git push -- origin 231d7741881ca5a1fac3389feffd6ad506eb945d:master) to move forward. If you don't understand what this command does, you can use git help push to understand the behavior of git push. Then update or delete local branches manually.
  • Run the "undo" command arc gives you (like git checkout feature --).
  • Run arc land <feature>, without --hold. Because arc land --hold no longer touches local branches, this will do the same thing the first command did and produce an identical intermediate state, and then push, update, and cleanup normally. This will only produce a different result in reasonable situations: the remote has moved forward; you've mucked with the working copy itself; or the revision on the web has been modified.

@mrkanaly I had a similar issue. Putting the following in my .vimrc allows me to type arcl to get the same behavior as the old arc land command:

arcl() {
   arc land --merge --hold "$@" && git checkout master && git merge --ff-only HEAD@{1}
}

Since I found this looking for a solution figured I'd post what I did; might help someone else.