On merging strategies, fast forward and index merge

I recently read Junio's description of how to dig oneself out of a hole
using `git merge -s ours' (I'm learning to use the space...), and I've
realised there's a problem here.

The `ours' merge strategy is meant to create a merge commit whose tree
is in every way identical to that of the starting commit.  But `git
merge' won't always do this, because it doesn't always invoke the
strategy program.

Consider the command `git merge -s ours MESSAGE MASTER FAILED'.

  * If we've not actually messed with our MASTER since the FAILED branch
    departed, then MASTER is actually an ancestor of FAILED, and `git
    merge' will unhelpfully fast-forward us to the tip of the FAILED
    branch.  Instead of leaving the merge result like MASTER, it's made
    it entirely the wrong thing!

  * If both MASTER and FAILED have made changes, but to different files,
    then `git merge' will try an index-level merge, find that it
    succeeds, and leave us with a mixture of MASTER and FAILED files.
    Which is (in this case) entirely what we didn't want.

Additionally, it occurs to me that the fast-forwarding behaviour isn't
always what I want anyway.  Consider a merge of a topic branch:

  `git merge MESSAGE MASTER TOPIC'

If I allow fast-forward, I lose information about where the topic
started and ended.  This is a shame, particularly if I find other places
I want to apply those changes (either as a string of similar commits, or
squidged into a single one) onto other branches.

Because code speaks louder, I'll follow-up this article with a suggested
patch.

-- [mdw]

From: Mark Wooding <redacted>

These options disable some of git-merge's optimizations.  

--no-fast-forward
	Does what it says on the tin: git-merge will always make a
	commit as a result of this merge (or leave one in the pipeline,
	if --no-commit was given).

--direct
	Don't do anything clever: go directly to the merge strategy
	programs.  In particular, this forbids an attempt at in-index
	merging.

We also force direct merging with the `ours' strategy, since this is
obviously what was wanted.

Signed-off-by: Mark Wooding <redacted>
---

 Documentation/merge-options.txt |    9 ++++++++-
 git-merge.sh                    |   28 ++++++++++++++++++++++------
 git-pull.sh                     |   13 +++++++++++--
 3 files changed, 41 insertions(+), 9 deletions(-)

diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 53cc355..5b145a1 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt

@@ -6,7 +6,6 @@
 	not autocommit, to give the user a chance to inspect and
 	further tweak the merge result before committing.
 
-
 -s <strategy>, \--strategy=<strategy>::
 	Use the given merge strategy; can be supplied more than
 	once to specify them in the order they should be tried.

@@ -14,3 +13,11 @@
 	is used instead (`git-merge-recursive` when merging a single
 	head, `git-merge-octopus` otherwise).
 
+--no-ff, \--no-fast-forward::
+	Don't fast-forward, even when it looks possible.  There will
+	always be a commit to do at the end of the merge.
+
+--direct::
+	Don't do anything clever: go directly to the merge strategy
+	programs.  In particular, this forbids an attempt at in-index
+	merging.

diff --git a/git-merge.sh b/git-merge.sh
index cc0952a..d6a579f 100755
--- a/git-merge.sh
+++ b/git-merge.sh

@@ -13,6 +13,8 @@ LF='
 all_strategies='recursive octopus resolve stupid ours'
 default_strategies='recursive'
 use_strategies=
+ff=t
+index_merge=t
 if test "@@NO_PYTHON@@"; then
 	all_strategies='resolve octopus stupid ours'
 	default_strategies='resolve'

@@ -65,6 +67,12 @@ do
 		no_summary=t ;;
 	--no-c|--no-co|--no-com|--no-comm|--no-commi|--no-commit)
 		no_commit=t ;;
+	--no-f|--no-ff|--no-fa|--no-fas|--no-fast|--no-fast-|--no-fast-f|\
+		--no-fast-fo|--no-fast-for|--no-fast-forw|--no-fast-forwa|\
+		--no-fast-forwar|--no-fast-forward)
+		ff=f ;;
+	--d|--di|--dir|--dire|--direc|--direct)
+		ff=f index_merge=f ;;
 	-s=*|--s=*|--st=*|--str=*|--stra=*|--strat=*|--strate=*|\
 		--strateg=*|--strategy=*|\
 	-s|--s|--st|--str|--stra|--strat|--strate|--strateg|--strategy)

@@ -90,6 +98,10 @@ do
 	shift
 done
 
+# `ours' is a funny strategy and clever merging optimizations here make
+# it not work.
+case " $use_strategies " in *" ours "*) ff=f index_merge=f ;; esac
+
 test "$#" -le 2 && usage ;# we need at least two heads.
 
 merge_msg="$1"

@@ -118,18 +130,18 @@ case "$#" in
 esac
 echo "$head" >"$GIT_DIR/ORIG_HEAD"
 
-case "$#,$common,$no_commit" in
-*,'',*)
+case "$#,$ff,$index_merge,$common,$no_commit" in
+*,*,*,'',*)
 	# No common ancestors found. We need a real merge.
 	;;
-1,"$1",*)
+1,*,*,"$1",*)
 	# If head can reach all the merge then we are up to date.
 	# but first the most common case of merging one remote
 	echo "Already up-to-date."
 	dropsave
 	exit 0
 	;;
-1,"$head",*)
+1,t,*,"$head",*)
 	# Again the most common case of merging one remote.
 	echo "Updating from $head to $1"
 	git-update-index --refresh 2>/dev/null

@@ -139,11 +151,11 @@ case "$#,$common,$no_commit" in
 	dropsave
 	exit 0
 	;;
-1,?*"$LF"?*,*)
+1,*,*,?*"$LF"?*,*)
 	# We are not doing octopus and not fast forward.  Need a
 	# real merge.
 	;;
-1,*,)
+1,*,t,*,)
 	# We are not doing octopus, not fast forward, and have only
 	# one common.  See if it is really trivial.
 	git var GIT_COMMITTER_IDENT >/dev/null || exit

@@ -164,6 +176,10 @@ case "$#,$common,$no_commit" in
 	fi
 	echo "Nope."
 	;;
+1,*,*,*,)
+	# Only a single remote, but we've been told not to try anything
+	# clever.  Skip to real merge.
+	;;
 *)
 	# An octopus.  If we can reach all the remote we are up to date.
 	up_to_date=t

diff --git a/git-pull.sh b/git-pull.sh
index 17fda26..229cec7 100755
--- a/git-pull.sh
+++ b/git-pull.sh

@@ -8,7 +8,7 @@ USAGE='[-n | --no-summary] [--no-commit]
 LONG_USAGE='Fetch one or more remote refs and merge it/them into the current HEAD.'
 . git-sh-setup
 
-strategy_args= no_summary= no_commit=
+strategy_args= no_summary= no_commit= noff= direct=
 while case "$#,$1" in 0) break ;; *,-*) ;; *) break ;; esac
 do
 	case "$1" in

@@ -17,6 +17,12 @@ do
 		no_summary=-n ;;
 	--no-c|--no-co|--no-com|--no-comm|--no-commi|--no-commit)
 		no_commit=--no-commit ;;
+	--no-f|--no-ff|--no-fa|--no-fas|--no-fast|--no-fast-|--no-fast-f|\
+		--no-fast-fo|--no-fast-for|--no-fast-forw|--no-fast-forwa|\
+		--no-fast-forwar|--no-fast-forward)
+		noff=--no-fast-forward ;;
+	--d|--di|--dir|--dire|--direc|--direct)
+		direct=--direct ;;
 	-s=*|--s=*|--st=*|--str=*|--stra=*|--strat=*|--strate=*|\
 		--strateg=*|--strategy=*|\
 	-s|--s|--st|--str|--stra|--strat|--strate|--strateg|--strategy)

@@ -92,4 +98,7 @@ case "$strategy_args" in
 esac
 
 merge_name=$(git-fmt-merge-msg <"$GIT_DIR/FETCH_HEAD")
-git-merge $no_summary $no_commit $strategy_args "$merge_name" HEAD $merge_head
+git-merge \
+	$no_summary $no_commit $noff $direct \
+	$strategy_args \
+	"$merge_name" HEAD $merge_head

Mark Wooding [off-list ref] writes:

These options disable some of git-merge's optimizations.  

While there is no question about the part of the proposed change
to bypass "trivial merge", I do not necessarily agree with
"skipping fast forward" part.

It is a problem that "ours" strategy cannot be used as a way to
recover from the "accidentally rewound head" situation, because
it is prevented from running under certain conditions as you
described.  But that does not necessarily mean we should make
"ours" to work in these situations.

You accidentally discarded B while somebody picked it up:

                 o---o
                /     \
    ---o---o---A---o---o---B
               ^your head

Now you would want to recover.  With your patch, it would create
this:

                 o---o
                /     \
    ---o---o---A---o---o---B
                \           \
                 ------------M
                             ^your updated head,
                              having the same tree as A

But recording A as a parent of M is not necessary.  I think what
we want to have as the result is this instead:

                 o---o
                /     \
    ---o---o---A---o---o---B---M
                               ^your updated head,
                                having the same tree as A

This is something you cannot do within the current git-merge
framework; it is set up to either just fast forward or make a
multi-parent commit.  You would want have a "revert to this
state" [*1*], something like this (assuming you have rewound to
A and currently your index matches A):

        $ git reset --soft B
        $ git commit -m 'Discard A..B and revert to A'

I did "ours" primarily as a demonstration of a funky thing
people could do with the consolidated driver "git-merge".  I did
not have a useful use-case in mind back then, but it turned out
to be the ideal way to recover from "accidentally rewound head"
situation, except that making a merge commit between A and B is
not always the way to recover from it.  If we wanted to, we
could have a special purpose command that does "git merge -s
ours" if there will be a new commit, otherwise the above two
commands sequence if it will be a fast forward, but the
"recovering from accidentally rewound head" _is_ really a
special purpose, so I do not know if it is worth it.

In your cover letter, you talked about using --no-fast-forward
to collapse your sole topic branch into your master branch.  I
do not think smudging the development history with extra merge
commits for that is justfied either.  There is no reason for you
to discard your topic branch heads after you merged them into
master.  If they get in the way of your normal workflow, you can
stash them away as tags that you do not usually see in "git
branch" output [*2*].

Also I am already unhappy that git-merge knows about the
specifics of strategies [*3*], e.g. it knows octopus is
currently the only strategy that can do more than two heads.
Your patch gives more strategy specific knowledge to it, but I
do not know how to avoid it.


[Footnotes]

*1* As opposed to "git revert X" which means "revert the effect
of commit X", you would want "revert to the state X".

*2* I keep some of my old topic branch heads under
.git/tags/attic/.

*3* Another thing I am unhappy about is the list of available
strategies.  I initially wanted to allow users to write their
own merge strategies and have them on their PATH (not even
necessarily in GIT_EXEC_PATH directory), so that you can do a
git-merge-mdw secretly, keep it in ~mdw/bin and cook it for a
while using yourself as a guinea pig, and then share that with
the community later, _without_ touching git-merge.