[IRC/patches] Failed octopus merge does not clean up

Hi *

James "jammyd" Mulcahy pointed out on IRC that the octopus merge strategy doesn't properly clean up behind itself. To wit:

  git init
  echo initial > foo
  git add foo
  git commit -m initial
  echo a > foo
  git commit -m a foo
  git checkout -b b HEAD^
  echo b > foo
  git commit -m b foo
  git checkout -b c HEAD^
  echo c > foo
  git commit -m c foo
  git checkout master
  git merge b c

The merge says

  Trying simple merge with 5b3e4bb1c2d88d6967fb575729fbfc86df5eaec9
  Simple merge did not work, trying automatic merge.
  Auto-merging foo
  ERROR: Merge conflict in foo
  fatal: merge program failed
  Automated merge did not work.
  Should not be doing an Octopus.
  Merge with strategy octopus failed.

So far so good.  However, 'git status' claims

  #       unmerged:   foo

and indeed the contents of 'foo' are the conflicted merge between 'master' and 'b', yet there is no .git/MERGE_HEAD. This behaviour is identical for 1.5.6 and 1.6.0.2, so it is not caused by the merge rewrite as a builtin. Shouldn't it either really clean up, or really leave the repo in a conflicted merge state? (I'm following up with a patch that turns the above into a test. Octopus doesn't really have many tests, does it?)

On the code path to the "Merge with strategy %s failed" message, builtin-merge.c:1134 runs restore_state() which runs reset_hard(). But (as Miklos pointed out) that cannot actually do 'git reset --hard' because it is possible (though not recommended, see below) to start a merge with a dirty index.

Jakub mentioned that there are only three index stages for a three-way merge, so a conflicted n-way (simultaneous) merge is not really possible.

The merge manpage should warn about merging with uncommitted changes. It recommends 'git rebase --hard' to abort during conflicts, but does not mention that this throws away said changes. I'm following up with a patch for this.

- Thomas

Currently, a conflicted octopus merge leaves the repository in a "halfway into the merge state", where .git/MERGE_HEAD is missing but the files are listed as conflicted in the index and have conflict markers in them. It should either clean up everything, or add a MERGE_HEAD.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>

---
 t/t7607-merge-octopus-cleanup.sh |   31 +++++++++++++++++++++++++++++++
 1 files changed, 31 insertions(+), 0 deletions(-)

Merging in a dirty tree is usually a bad idea because you need to reset --hard to abort; but the docs didn't say anything about it.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>

---
 Documentation/git-merge.txt |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

Thomas Rast <trast@student.ethz.ch> writes:

Strictly speaking, you do not have to worry about anything if (1) all of your work tree changes are easily reproducible (Linus's keeping a new EXTRAVERSION in his Makefile, never staged nor committed is an often cited example), or (2) you know beforehand that a merge with the other party will not touch the part you have local changes that you care about.

In other words, you need to know what you are doing, and a warning with "usually it is a bad idea" would be an appropriate thing to do.

On Mon, Sep 15, 2008 at 7:42 PM, Junio C Hamano <gitster@pobox.com> wrote:

But how do you abort a *failed* merge in a situation like Linus's example? "git reset --hard HEAD" would destroy the unstaged Makefile change.

I would love to know the answer to this for my own work, and I guess it would be relevant to the documentation too.

Have fun,

Avery

"Avery Pennarun" <apenwarr@gmail.com> writes:

"All of your work tree changes are easily reproducible" implies you do not mind losing them, Ok?

Also "git reset HEAD" (that is, without --hard) would not touch the work tree changes. You need to remove the work-tree cruft left by new files yourself, if you go this route, though. New files are rare enough so it may be more appropriate (and you can "git clean -n $that_subdirectory" to enumerate them).

It all depends on your workflow.

On Mon, 15 Sep 2008, Avery Pennarun wrote:

As mentioned by others, sometimes you are simply willing to take the risk. If I have dirty data, I still want to merge, because (a) my dirty data is a _convenience_ and (b) the risk of me having to do a "git reset" is pretty low anyway.

That said, it's actually kind of sad that we don't expose a real capability that the git plumbing does have. Namely

	git read-tree -u -m HEAD ORIG_HEAD

should do the right thing if you want to undo a merge (except it doesn't actually write ORIG_HEAD to be the new head: you could use "git reset" with --soft to do that, or just git update-ref).

So it _may_ be that something like

	[alias]
		undo = !git read-tree -u -m HEAD ORIG_HEAD && git reset --soft ORIG_HEAD

would actually give you "git undo". 

So we have the technology, and we just don't _expose_ that capability as a "git reset" thing. And we probably should. In fact, that is often the thing people really want, and it would have made sense to have it as the default action, but the initial design for "git reset" was literally as a way to get you out of a sticky corner when you had unmerged entries and you just wanted to throw away crud.

NOTE NOTE NOTE! I did _not_ test that the git read-tree thing actually works, or that the above alias does the right thing. Caveat testor! You're on your own. But I agree that we should have something like that.

		Linus

On Thu, Sep 18, 2008 at 11:15 AM, Linus Torvalds <torvalds@linux-foundation.org> wrote:

In that case, my next question is how you pull off (b), because that's *way* better than just being able to undo when I get myself into trouble :) I do and then reset test merges all the time.

Hmm,

   $ git read-tree -u -m HEAD ORIG_HEAD
   fatal: you need to resolve your current index first

It appears that the above would be great for undoing a *non*conflicting merge, but that's not as important ;)

Note that if we were going to do an undo, it would be nice to be careful about allowing multiple consecutive undos. "git undo; git undo;" shouldn't be a no-op, it should undo two things. At least, that's how the rest of the world (okay, my text editor) works. "git redo" could be the opposite of "git undo".

I imagine some trick using the reflog would thus be better here than updating ORIG_HEAD.

Since that's a lot of work, I'd propose having a first pass at the undo operation use ORIG_HEAD and then just delete or rename ORIG_HEAD, so that 'git undo; git undo' is meaningless.

Have fun,

Avery

Linus Torvalds <torvalds@linux-foundation.org> writes:

I do not think this is quite enough. "read-tree --reset -u" is probably closer, but it may discard the entries added (and got conflicted) by the merge before actually updating the working tree (the right thing there would be to remove them).

Thomas Rast <trast@student.ethz.ch> writes:

Your analysis is correct --- this has been the correct/established behaviour of Octopus from day one.

Read the second from the last line of what you were told by git. Run "git reset --hard" after that, perhaps.

Junio C Hamano wrote:

[...]

[...]

[...]

Including not cleaning up the half-merged state? If the answer is "yes", then so be it, merge-octopus has been on this project longer than I have ;-)

However,

wasn't immediately obvious to the end-user (jammyd in this case), which started the discussion.

Junio C Hamano <gitster@pobox.com> writes:

By the way, there are three failure modes in Octopus.

 (0) your index (not work tree) is dirty; this is not limited to octopus
     merge but any merge will be refused;

 (1) while it merges other heads one-by-one, it gets conflicts on an
     earlier one, before it even attempts to merge all of them.  Recording
     the heads that it so far attempted to merge in MERGE_HEAD is wrong
     (the result won't be an Octopus the end user wanted to carete), and
     recording all the heads the user gave in MERGE_HEAD is even more
     wrong (it hasn't even looked at the later heads yet, so there is no
     way for the index or work tree to contain anything from them).

     The above is hitting this case.

 (2) it gets conflicts while merging the _last_ head.  It records
     MERGE_HEAD and allows you to record the resolved result.

I think originally we treated (1) and (2) the same way, because an Octopus is to record the most trivial merge with more than 2 legs, and a rough definition of "the most trivial" is "tracks of totally independent works; you could merge them one by one and _in any order_, and the result won't matter because they are logically independent. However if they are _so_ independent, why not record them as merged in one step (i.e. octopus), instead of insisting on recording in what order you merged them".

Obviously, if you get a conflict during Octopus creation, they were not tracks of totally independent works, and that is where the "Should not" in the message comes from.

We later relaxed it to allow a conflict at the last step, not because recording an Octopus with nontrivial merge is particuarly a good idea we should encourage, but because there simply weren't reason not to.

Dnia wtorek 16. września 2008 02:20, Junio C Hamano napisał:

The problem, as far as I understand it, is not that octopus merge fails. The problem is that it fails halfway, and it leaves working are in inconsistent state: git-status output with its "unmerged" suggests that we are in the middle of the merge, but we are not.

IIRC the idea of stashing away changes, doing merge, and then unstashing was shot down as encouraging bad workflows, and more often than not leading only to mess in workdir and index.

IMVHO the correct solution would be to rollback changes to the state before attempting a merge. I'd rather 'octopus' did its thing as transaction; contrary to ordinary merges if merge fails it doesn't mean necessary that it is in the state of resolvable conflict, state we can stop at. Perhaps (if it is still a shell script, doing git-stash at beginning, and either dropping or popping the stash at the end; or just saving old index if it is built-in).

BTW. does it mean that "git merge a b" might be not the same as "git merge b a"?

Well, octopus merge could simply fail when merge is nontrivial (not limited to being tree-level merge only).

Jakub Narebski wrote:

No. Git merges all the sub-things together and then merges the result of that jumble into the branch you're on.

Someone might have to correct me on that, but that's as far as I've understood it.

Andreas Ericsson wrote:

[...] 
 

mailing list about better implementation of and documenting finding merge bases for multiple heads, I think octopus merge is done by merging [reduced] heads one by one into given branch.

This means that "git merge a b" does internally "git merge a; git merge b" as I understand it.

On Wed, Sep 17, 2008 at 10:11:02AM +0200, Jakub Narebski <jnareb@gmail.com> wrote:

Sure, but given that both 'a' and 'b' merged (so none of them is subset of the other, for example so that reduce_heads() would drop one of them) the order of the parents will be different so the resulting commit will differ. The resulting tree will no, I think.

Miklos Vajna wrote:

I got it wrong (not wrt reduced heads, but still). My apologies.

If octopus (the program/strategy/whatever) continues to merge after a branch conflicting against the currently checked out branch (let's call it "master"), the resulting tree may not differ, but then again, it might. OTOH, if octopus quits the merge after having encountered a conflict, the order the branches to merge were passed will always have an impact.

Let's say you have two branches, "clean" and "conflict". Which one is which should be obvious here.

	git merge clean conflict

will produce a tree with 'master', 'clean' and a conflicted merge of 'conflict', while

	git merge conflict clean

will produce a tree with 'master' and a conflicted merge of 'conflict'.

In short, backing out the entire merge in case of a conflict is almost certainly the only sane thing to do. If one branch depends on another to merge cleanly, a different approach should have been taken (depending branch should have been rebased onto the dependent one prior to merging), but the merge *might* succeed depending on in which order the other branches are given as arguments. It's not a very clever idea to merge something like that though, as bisection will invariably have to mark the entire depending branch as BAD, although the merge itself could obviously be a good one.

Clearly, an octopus merge should not be undertaken without knowing very well what it is one is merging in.