Re: [RFC/PATCH] Fast forward strategies allow, never, and only

Re: [RFC/PATCH] Fast forward strategies allow, never, and only

[colin]

> What's lacking is "why this is a good idea".

Seconded.  A long time ago (and I'm too lazy to find a link), Linus
explained why disabling fast-forward merges was almost always a Bad Idea,
and nobody has come up with a good reason why you'd want one since.

But from memory, suppose that you have two developers, each working on
their own branch:

     a--a--a <-- A's head
    /
o--o
    \
     b--b--b <-- B's head

Then suppose that they merge back and forth to get to the same state.
With fast-forward merges, it will go like this:

A merges from B:
     a--a--a
    /       \
o--o         o <-- A's head
    \       /
     b--b--b <-- B's head

Then B merges from A:
     a--a--a
    /       \
o--o         o <-- Both heads
    \       /
     b--b--b


And look, they are in sync and can go on to develop from a common base
version.  Future merges will do nothing.


If, instead, you have every merge generate a commit, then you get:
     a--a--a
    /       \
o--o         o <-- A's head
    \       / \
     b--b--b---o <-- B's head

     a--a--a
    /       \
o--o         o---o <-- A's head
    \       / \ /
     b--b--b---o <-- B's head

     a--a--a
    /       \
o--o         o---o <-- A's head
    \       / \ / \
     b--b--b---o---o <-- B's head

.. and it never ends.  All of the merged commits are identical trees, but
if you insist on creating a new commit object each time, you can generate
an infinite number of bogus commits, and more to the point, A and B will
never actually agree on the current HEAD commit.

With more developers, you can make even more of a mess.

What use does the "--ff=never" option have except to generate this cruft?
Flexibility is useful only as long as it provides the ability to do
something desirable.  There's no point to having a button that should
never be pushed.

Re: [RFC/PATCH] Fast forward strategies allow, never, and only

[Lars Hjemli]

On Tue, Mar 11, 2008 at 10:35 AM,  <colin@horizon.com> wrote:
> > What's lacking is "why this is a good idea".
>
>  Seconded.  A long time ago (and I'm too lazy to find a link), Linus
>  explained why disabling fast-forward merges was almost always a Bad Idea,
>  and nobody has come up with a good reason why you'd want one since.

The reason for --no-ff was twofold:
* theoretical: when you want to record the integration of a topic branch
* practical: when merging git-svn branches in git, git-svn dcommit
would update the wrong svn 'branch' if the merge was a fast-forward

I originally needed --no-ff due to the 'practical' aspects (I used
git-svn when working with the day-job svn repository), but now that
we've switched to git (Hurray!) I'm still using --no-ff for the
'theoretical' reason: our topic branches tend to be named after
bugtracker tickets, so by recording the merge of such a branch we get
a very explicit note in our git log about when each ticket was
resolved.

YMMV.

--
larsh

Re: [RFC/PATCH] Fast forward strategies allow, never, and only

[Bruce Stephens]

colin@horizon.com writes:

>> What's lacking is "why this is a good idea".

[...]

> .. and it never ends.  All of the merged commits are identical trees, but
> if you insist on creating a new commit object each time, you can generate
> an infinite number of bogus commits, and more to the point, A and B will
> never actually agree on the current HEAD commit.
>
> With more developers, you can make even more of a mess.
>
> What use does the "--ff=never" option have except to generate this cruft?
> Flexibility is useful only as long as it provides the ability to do
> something desirable.  There's no point to having a button that should
> never be pushed.

IIUC what the new option is about is (optionally) forbidding merges.
So it's orthogonal to the existing --no-ff and --ff merge options.

So you *don't* get that kind of criss-crossing: if you've got a local
commit, the merge fails.  So you have to use rebase.  So it's not
making the history more complex, it's linearizing it.

Now surely you don't always want to do that, but it seems like a very
convenient option that you can generally have on, and switch off when
you intend to do a merge.

Re: [RFC/PATCH] Fast forward strategies allow, never, and only

[Bruce Stephens]

Bruce Stephens <bruce.stephens@isode.com> writes:

> colin@horizon.com writes:

[...]

> IIUC what the new option is about is (optionally) forbidding merges.
> So it's orthogonal to the existing --no-ff and --ff merge options.

I'm wrong.  My apologies.

[...]

Re: [RFC/PATCH] Fast forward strategies allow, never, and only

[Junio C Hamano]

colin@horizon.com writes:

>      a--a--a
>     /       \
> o--o         o---o <-- A's head
>     \       / \ /
>      b--b--b---o <-- B's head
>
>      a--a--a
>     /       \
> o--o         o---o <-- A's head
>     \       / \ / \
>      b--b--b---o---o <-- B's head
>
> .. and it never ends.  All of the merged commits are identical trees, but
> if you insist on creating a new commit object each time, you can generate
> an infinite number of bogus commits, and more to the point, A and B will
> never actually agree on the current HEAD commit.
>
> With more developers, you can make even more of a mess.
>
> What use does the "--ff=never" option have except to generate this cruft?

Judicious use of non-fast-forward has a justification that is not too
unreasonable.  That is, when you want to treat one lineage of history as
"more special than others".

If your workflow is always to branch from the special branch ("master")
when working on even a miniscule topic and merge that back to "master", if
you happen to have worked only on a single topic and the "master" was
never advanced during the time you worked on that topic, merging the topic
back to "master" will result in a fast-forward.  When you look back that
history, you won't be able to tell where the topic started and ended by
following the ancestry chain of the "master" branch.

Using "never fast forward" policy on such a special branch will be a way
to make sure that all commits on the first-parent ancestry of that special
branch will be merges from something else, and by computing $it^1..$it^2
for a merge commit $it on the special branch, which merges the topic fully
into it, you can tell what commits the topic consisted of.

When you have repeated merges from a topic to that special branch, this
computation needs to be a bit more than just $it^1..$it^2 of the last
merge commit that merges the topic into "master".  E.g. you would have two
"should have been fast forward but artificially made into a real merge for
the purpose of peeing in the snow" like this:

           o---o---o---o---o "topic"
          /     \           \
      ---o-------*-----------* "master"
 
By following the first-parent ancestry of "master", you can tell that the
first two changes on "topic" were accepted earlier and then three fixups
on top were incorporated much later, which is not something you can do if
you allowed fast-forward merge into "master".  Computing this history is
somewhat expensive but it is doable.  You have to follow the commit
ancestry of "topic", and for each commit you find, you would need to see
which commit on the first-parent ancestry of "master" can reach it
(e.g. the three topmost ones on "topic" can be reachable only by the last
merge on "master", while the remaining two can be reached by the previous
merge on "master").

In other words, if there is a globally special "master" history where
everybody meets, forcing an artificial merge can have value.  However, for
this to work, you can never commit anything directly on such a special
"master" branch, because directly committing on "master" is equivalent to
fork a small topic branch that has a single commit on it, and immediately
merging it back with a fast-forward merge to "master".  So an artificial
merge can have value but that value can be had only with a disciplined
workflow.

Last night I pulled a topic from Shawn which was a series of updates to
the bash completion script.  It was based on the tip of 'master' and
resulted in a fast forward.  In git.git circle, it happens that my
"master" history is not special at all.  I have "trivially correct fixups"
directly committed on "master" all the time, and fast-forwarding to the
tip of bash completion updates Shawn collected for me was exactly that,
with only different committer.  So even though I act as the top-level
integrator for git.git history, there was no reason to do non-fast-forward
merge at that point.  My tree is not that special.

On the other hand, I probably _could_ use non-ff to manage "next", which
will fork off of the tip of "master" after every major release.  In order
to treat the first topic that will be merged into "next" just like other
later topics, it should be merged without fast-forward.  The latter topics
will never fast-forward (because topics fork off of "master" or "maint"
and never from "next" itself) but the very first one can (because "master"
and "next" will be at the same at that point), and allowing fast-forward
would mean the first topic after a major release is treated differently
from others.  This is possible only because there is a fairly strict
discipline of not committing anything directly on top of "next" and not
forking off of it.

[RFC/PATCH] Fast forward strategies allow, never, and only

[Sverre Hvammen Johansen]

>From 795bd1b3e70f011b675061ecae322527a9f0695c Mon Sep 17 00:00:00 2001
From: Sverre Hvammen Johansen <sj@gmail.com>
Date: Sun, 9 Mar 2008 21:43:56 -0800
Subject: [PATCH] Fast forward strategies allow, never, and only.

New fast forward strategies, only, is introduced.  This new fast
forward strategy prevents real merges.

FF strategy "only" fails if the specified heads and HEAD can not
be reduced down to only one real parent.  The only allowed
outcome is a fast forward unless HEAD is up to date with
the specified heads.

This patch also uses the real heads found instead of those
specified for real merges.  This means that merge startegies
that only take two heads can now accept more than two heads
if they can be reduced down to only two real heads.  However,
fast-forward of head in combination with a real merge is
handled as before.

Signed-off-by: Sverre Hvammen Johansen <sj@black.local>
---
 Documentation/fast-forward-strategies.txt |   16 +
 Documentation/git-merge.txt               |    6 +-
 Documentation/git-pull.txt                |    2 +
 Documentation/merge-options.txt           |   11 +-
 git-merge.sh                              |  291 +++++++++++-----
 git-pull.sh                               |    4 +-
 t/t7601-merge-ff-strategies.sh            |  549 +++++++++++++++++++++++++++++
 7 files changed, 778 insertions(+), 101 deletions(-)
 create mode 100644 Documentation/fast-forward-strategies.txt
 create mode 100755 t/t7601-merge-ff-strategies.sh

diff --git a/Documentation/fast-forward-strategies.txt
b/Documentation/fast-forward-strategies.txt
new file mode 100644
index 0000000..1d6da26
--- /dev/null
+++ b/Documentation/fast-forward-strategies.txt
@@ -0,0 +1,16 @@
+FAST FORWARD STRATEGIES
+-----------------------
+
+allow::
+	Do not generate a merge commit if the merge resolved
+	as a fast-forward, only update the branch pointer.
+	This is the default behavior of git-merge.
+
+never::
+	Generate a merge commit even if the merge resolved
+	as a fast-forward.
+
+only::
+	Only allow a fast-forward.  The merge will fail
+	unless HEAD is up to date or the merge resolved as
+        a fast-forward.
diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index c136b10..fbf3ebe 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -9,7 +9,8 @@ git-merge - Join two or more development histories together
 SYNOPSIS
 --------
 [verse]
-'git-merge' [-n] [--summary] [--no-commit] [--squash] [-s <strategy>]...
+'git-merge' [-n] [--summary] [--no-commit] [--squash]
+	[-s <strategy>]... [--ff[=<fast forward strategy>]]
 	[-m <msg>] <remote> <remote>...
 'git-merge' <msg> HEAD <remote>...

@@ -37,6 +38,9 @@ include::merge-options.txt[]
 	least one <remote>.  Specifying more than one <remote>
 	obviously means you are trying an Octopus.

+
+include::fast-forward-strategies.txt[]
+
 include::merge-strategies.txt[]


diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index 7378943..99d205c 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -50,6 +50,8 @@ include::pull-fetch-param.txt[]

 include::urls-remotes.txt[]

+include::fast-forward-strategies.txt[]
+
 include::merge-strategies.txt[]

 DEFAULT BEHAVIOUR
diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 9f1fc82..848b786 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -29,12 +29,13 @@

 --no-ff::
 	Generate a merge commit even if the merge resolved as a
-	fast-forward.
+	fast-forward.  --on-ff is an alias for --ff=never.

---ff::
-	Do not generate a merge commit if the merge resolved as
-	a fast-forward, only update the branch pointer. This is
-	the default behavior of git-merge.
+--ff[=<fast forward strategy>]::
+	Select fast forward strategy.  --ff without any argument
+	is an alias for --ff=allow which is the default behavior
+	of git-merge.  It will not generate a merge commit if the
+	merge resolved as a fast-forward,

 -s <strategy>, \--strategy=<strategy>::
 	Use the given merge strategy; can be supplied more than
diff --git a/git-merge.sh b/git-merge.sh
index 7dbbb1d..873e4cb 100755
--- a/git-merge.sh
+++ b/git-merge.sh
@@ -12,7 +12,7 @@ summary              show a diffstat at the end of the merge
 n,no-summary         don't show a diffstat at the end of the merge
 squash               create a single commit instead of doing a merge
 commit               perform a commit if the merge sucesses (default)
-ff                   allow fast forward (default)
+ff?                  allow fast forward (default)
 s,strategy=          merge strategy to use
 m,message=           message to be used for the merge commit (if any)
 "
@@ -35,7 +35,7 @@ no_fast_forward_strategies='subtree ours'
 no_trivial_strategies='recursive recur subtree ours'
 use_strategies=

-allow_fast_forward=t
+fast_forward=allow
 allow_trivial_merge=t
 squash= no_commit=

@@ -153,8 +153,8 @@ parse_config () {
 		--summary)
 			show_diffstat=t ;;
 		--squash)
-			test "$allow_fast_forward" = t ||
-				die "You cannot combine --squash with --no-ff."
+			test "$fast_forward" = allow ||
+				die "You cannot combine --squash with --ff=never."
 			squash=t no_commit=t ;;
 		--no-squash)
 			squash= no_commit= ;;
@@ -163,11 +163,28 @@ parse_config () {
 		--no-commit)
 			no_commit=t ;;
 		--ff)
-			allow_fast_forward=t ;;
+			case "$2" in
+			allow|never|only)
+				fast_forward=$2 squash= no_commit= ; shift ;;
+			-*)
+				fast_forward=allow squash= no_commit= ;;
+			*)
+				die "available fast-forward strategies are: allow, newer, and only" ;;
+			esac
+			;;
+		--ff=*)
+			fast_forward=$(echo $1 |cut -d = -f 2) squash= no_commit=
+			case $fast_forward in
+			    allow|never|only)
+				;;
+			    *)
+				die "available fast-forward strategies are: allow, newer, and only" ;;
+			esac
+			;;
 		--no-ff)
 			test "$squash" != t ||
 				die "You cannot combine --squash with --no-ff."
-			allow_fast_forward=f ;;
+			fast_forward=never ;;
 		-s|--strategy)
 			shift
 			case " $all_strategies " in
@@ -279,24 +296,125 @@ do
 done
 set x $remoteheads ; shift

+echo "$head" >"$GIT_DIR/ORIG_HEAD"
+
+find_one_real_parent () {
+	# The real parent candidate
+	real_parent=$1
+	shift
+
+	# Other parents that are indepent of the real parent candidate
+	other_parents=
+
+	# Parents that need further processing to determine whether
+	# they are independent parents of the parent candidate or not
+	parents_x=
+
+	while test $# -gt 0
+	do
+		if test $real_parent = $1
+		then
+			# Found a parent that is equal to the real
+			# parent candidate
+			echo "Duplicate $(git rev-parse --short $1)"
+			echo "Ignoring $1"
+		else
+			common_b=$(git merge-base --all $real_parent $1)
+		
+			if test "$common_b" = $1
+			then
+				# Found a parent that is not
+				# independent of the real parent
+				# candidate
+				echo "Possible ff $(git rev-parse --short $1)..$(git rev-parse
--short $real_parent)."
+				echo "Ignoring $1"
+			elif test "$common_b" = $real_parent
+			then
+				# Found a better real parent candidate
+				echo "Possible ff $(git rev-parse --short $real_parent)..$(git
rev-parse --short $1)."
+				echo "Ignoring $real_parent"
+				real_parent=$1
+				parents_x="$other_parents"
+				other_parents=
+			else
+				# Found a parent that is independent
+				# of the real parent candidate
+				other_parents="$other_parents $1"
+			fi
+		fi
+		shift
+	done
+
+	# We have a real parent, some parents we know is independt of
+	# this real parent, and some parents that need further
+	# processing.
+
+	for b in $parents_x
+	do
+		common_b=$(git merge-base --all $real_parent $b)
+		if test "$common_b" != $b
+		then
+			other_parents="$other_parents $b"
+		fi
+	done
+
+	# We have a real parent and other parents we know is independent
+	# of this real parent
+}
+
+find_real_parents () {
+    find_one_real_parent $head "$@"
+    ff_head=$real_parent
+    real_parents=
+
+    while test -n "$other_parents"
+    do
+	find_one_real_parent $other_parents
+	real_parents="$real_parents $real_parent"
+    done
+}
+
+if test $fast_forward = never -o
+then
+	real_parents="$@"
+	ff_head=$head
+else
+	find_real_parents "$@"
+fi
+
+if test -n "$real_parents"
+then
+	case $fast_forward in
+	only)
+		die "Fast forward strategy only can only handle one real parent" ;;
+	never|allow)
+		if test $head != $ff_head
+		then
+			real_parents="$ff_head $real_parents"
+			ff_head=$head
+		fi
+		;;
+	esac
+fi
+
 case "$use_strategies" in
 '')
-	case "$#" in
-	1)
-		var="`git config --get pull.twohead`"
+	case "$real_parents" in
+	?*" "?*)
+		var="`git config --get pull.octopus`"
 		if test -n "$var"
 		then
 			use_strategies="$var"
 		else
-			use_strategies="$default_twohead_strategies"
+			use_strategies="$default_octopus_strategies"
 		fi ;;
 	*)
-		var="`git config --get pull.octopus`"
+		var="`git config --get pull.twohead`"
 		if test -n "$var"
 		then
 			use_strategies="$var"
 		else
-			use_strategies="$default_octopus_strategies"
+			use_strategies="$default_twohead_strategies"
 		fi ;;
 	esac
 	;;
@@ -308,7 +426,7 @@ do
 	do
 		case " $s " in
 		*" $ss "*)
-			allow_fast_forward=f
+			fast_forward=never
 			break
 			;;
 		esac
@@ -323,88 +441,73 @@ do
 		esac
 	done
 done
-
-case "$#" in
-1)
-	common=$(git merge-base --all $head "$@")
-	;;
-*)
-	common=$(git show-branch --merge-base $head "$@")
-	;;
-esac
-echo "$head" >"$GIT_DIR/ORIG_HEAD"
-
-case "$allow_fast_forward,$#,$common,$no_commit" in
-?,*,'',*)
-	# No common ancestors found. We need a real merge.
-	;;
-?,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.
-	finish_up_to_date "Already up-to-date."
-	exit 0
-	;;
-t,1,"$head",*)
-	# Again the most common case of merging one remote.
-	echo "Updating $(git rev-parse --short $head)..$(git rev-parse --short $1)"
-	git update-index --refresh 2>/dev/null
-	msg="Fast forward"
-	if test -n "$have_message"
+if test -z "$real_parents"
+then
+	if test $head = $ff_head
 	then
-		msg="$msg (no commit created; -m option ignored)"
-	fi
-	new_head=$(git rev-parse --verify "$1^0") &&
-	git read-tree -v -m -u --exclude-per-directory=.gitignore $head "$new_head" &&
-	finish "$new_head" "$msg" || exit
-	dropsave
-	exit 0
-	;;
-?,1,?*"$LF"?*,*)
-	# We are not doing octopus and not fast forward.  Need a
-	# real merge.
-	;;
-?,1,*,)
-	# We are not doing octopus, not fast forward, and have only
-	# one common.
-	git update-index --refresh 2>/dev/null
-	case "$allow_trivial_merge" in
-	t)
-		# See if it is really trivial.
-		git var GIT_COMMITTER_IDENT >/dev/null || exit
-		echo "Trying really trivial in-index merge..."
-		if git read-tree --trivial -m -u -v $common $head "$1" &&
-		   result_tree=$(git write-tree)
-		then
-			echo "Wonderful."
-			result_commit=$(
-				printf '%s\n' "$merge_msg" |
-				git commit-tree $result_tree -p HEAD -p "$1"
-			) || exit
-			finish "$result_commit" "In-index merge"
-			dropsave
-			exit 0
-		fi
-		echo "Nope."
-	esac
-	;;
-*)
-	# An octopus.  If we can reach all the remote we are up to date.
-	up_to_date=t
-	for remote
-	do
-		common_one=$(git merge-base --all $head $remote)
-		if test "$common_one" != "$remote"
+		finish_up_to_date "Already up-to-date."
+		exit 0
+	elif test $fast_forward = never
+	then
+		real_parents="$ff_head"
+		ff_head=$head
+	else
+		echo "Updating $(git rev-parse --short $head)..$(git rev-parse
--short $ff_head)"
+		git update-index --refresh 2>/dev/null
+		msg="Fast forward"
+		if test -n "$have_message"
 		then
-			up_to_date=f
-			break
+			msg="$msg (no commit created; -m option ignored)"
 		fi
-	done
-	if test "$up_to_date" = t
-	then
-		finish_up_to_date "Already up-to-date. Yeeah!"
+		new_head=$(git rev-parse --verify "$ff_head^0") &&
+		git read-tree -v -m -u --exclude-per-directory=.gitignore $head
"$new_head" &&
+		finish "$new_head" "$msg" || exit
+		dropsave
 		exit 0
 	fi
+else
+	if test $head != $ff_head -a $fast_forward = never
+	then
+		real_parents="$ff_head $real_parents"
+		ff_head=$head
+	fi
+fi
+
+case "$real_parents" in
+?*" "?*)
+	# We have more than one parent
+	common=$(git show-branch --merge-base $head $real_parents)
 	;;
+*)
+	# We have exactly one parent
+	common=$(git merge-base --all $ff_head $real_parents)
+	case "$common" in
+	?*"$LF"?*)
+		# We are not doing octopus and not fast forward.  Need a
+		# real merge.
+		;;
+	*)
+		git update-index --refresh 2>/dev/null
+		if test "$allow_trivial_merge" = t
+		then
+			# See if it is really trivial.
+			git var GIT_COMMITTER_IDENT >/dev/null || exit
+			echo "Trying really trivial in-index merge..."
+			if git read-tree --trivial -m -u -v $common $head $real_parents &&
+				result_tree=$(git write-tree)
+			then
+				echo "Wonderful."
+				result_commit=$(
+					printf '%s\n' "$merge_msg" |
+					git commit-tree $result_tree -p HEAD -p $real_parents
+				) || exit
+				finish "$result_commit" "In-index merge"
+				dropsave
+				exit 0
+			fi
+			echo "Nope."
+		fi ;;
+	esac ;;
 esac

 # We are going to make a new commit.
@@ -445,7 +548,7 @@ do
     # Remember which strategy left the state in the working tree
     wt_strategy=$strategy

-    git-merge-$strategy $common -- "$head_arg" "$@"
+    git-merge-$strategy $common -- "$head_arg" $real_parents
     exit=$?
     if test "$no_commit" = t && test "$exit" = 0
     then
@@ -481,11 +584,11 @@ done
 # auto resolved the merge cleanly.
 if test '' != "$result_tree"
 then
-    if test "$allow_fast_forward" = "t"
+    if test $fast_forward != never
     then
-        parents=$(git show-branch --independent "$head" "$@")
+        parents=$(git show-branch --independent "$head" $real_parents)
     else
-        parents=$(git rev-parse "$head" "$@")
+        parents=$(git rev-parse "$head" $real_parents)
     fi
     parents=$(echo "$parents" | sed -e 's/^/-p /')
     result_commit=$(printf '%s\n' "$merge_msg" | git commit-tree
$result_tree $parents) || exit
@@ -515,7 +618,7 @@ case "$best_strategy" in
 	echo "Rewinding the tree to pristine..."
 	restorestate
 	echo "Using the $best_strategy to prepare resolving by hand."
-	git-merge-$best_strategy $common -- "$head_arg" "$@"
+	git-merge-$best_strategy $common -- "$head_arg" $real_parents
 	;;
 esac

diff --git a/git-pull.sh b/git-pull.sh
index 3ce32b5..b78cfdd 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -4,7 +4,7 @@
 #
 # Fetch one or more remote refs and merge it/them into the current HEAD.

-USAGE='[-n | --no-summary] [--[no-]commit] [--[no-]squash]
[--[no-]ff] [-s strategy]... [<fetch-options>] <repo> <head>...'
+USAGE='[-n | --no-summary] [--[no-]commit] [--[no-]squash]
[--ff=<ff-strategy>] [-s strategy]... [<fetch-options>] <repo>
<head>...'
 LONG_USAGE='Fetch one or more remote refs and merge it/them into the
current HEAD.'
 SUBDIRECTORY_OK=Yes
 OPTIONS_SPEC=
@@ -41,6 +41,8 @@ do
 		no_ff=--ff ;;
 	--no-ff)
 		no_ff=--no-ff ;;
+	--ff=*)
+		no_ff=$1 ;;
 	-s=*|--s=*|--st=*|--str=*|--stra=*|--strat=*|--strate=*|\
 		--strateg=*|--strategy=*|\
 	-s|--s|--st|--str|--stra|--strat|--strate|--strateg|--strategy)
diff --git a/t/t7601-merge-ff-strategies.sh b/t/t7601-merge-ff-strategies.sh
new file mode 100755
index 0000000..6c0a91a
--- /dev/null
+++ b/t/t7601-merge-ff-strategies.sh
@@ -0,0 +1,549 @@
+#!/bin/sh
+#
+# Copyright (c) 2007 Lars Hjemli
+#
+
+test_description='git-merge
+
+Testing basic merge operations/option parsing.'
+
+. ./test-lib.sh
+
+cat >file <<EOF
+1
+2
+3
+4
+5
+6
+7
+8
+9
+10
+11
+12
+EOF
+
+cat >file.1 <<EOF
+1 X
+2
+3
+4
+5
+6
+7
+8
+9
+10
+11
+12
+EOF
+
+cat >file.5 <<EOF
+1
+2
+3
+4
+5 X
+6
+7
+8
+9
+10
+11
+12
+EOF
+
+cat >file.9 <<EOF
+1
+2
+3
+4
+5
+6
+7
+8
+9 X
+10
+11
+12
+EOF
+
+cat  >result.0 <<EOF
+1
+2
+3
+4
+5
+6
+7
+8
+9
+10
+11
+12
+EOF
+
+cat  >result.1 <<EOF
+1 X
+2
+3
+4
+5
+6
+7
+8
+9
+10
+11
+12
+EOF
+
+cat >result.1-5 <<EOF
+1 X
+2
+3
+4
+5 X
+6
+7
+8
+9
+10
+11
+12
+EOF
+
+cat >result.1-5-9 <<EOF
+1 X
+2
+3
+4
+5 X
+6
+7
+8
+9 X
+10
+11
+12
+EOF
+
+cat >result.1-5-9-13 <<EOF
+1 X
+2
+3
+4
+5 X
+6
+7
+8
+9 X
+10
+11
+12
+13 x
+EOF
+
+cat >result.1-5-13 <<EOF
+1 X
+2
+3
+4
+5 X
+6
+7
+8
+9
+10
+11
+12
+13 x
+EOF
+
+cat >result.5-13 <<EOF
+1
+2
+3
+4
+5 X
+6
+7
+8
+9
+10
+11
+12
+13 x
+EOF
+
+cat >result.1-13 <<EOF
+1 X
+2
+3
+4
+5
+6
+7
+8
+9
+10
+11
+12
+13 x
+EOF
+
+cat >extend <<EOF
+13 x
+EOF
+
+
+create_merge_msgs() {
+	echo "Merge commit 'c2'" >msg.1-5 &&
+	echo "Merge commit 'c2'; commit 'c3'" >msg.1-5-9 &&
+	echo "Squashed commit of the following:" >squash.1 &&
+	echo >>squash.1 &&
+	git log --no-merges ^HEAD c1 >>squash.1 &&
+	echo "Squashed commit of the following:" >squash.1-5 &&
+	echo >>squash.1-5 &&
+	git log --no-merges ^HEAD c2 >>squash.1-5 &&
+	echo "Squashed commit of the following:" >squash.1-5-9 &&
+	echo >>squash.1-5-9 &&
+	git log --no-merges ^HEAD c2 c3 >>squash.1-5-9
+}
+
+verify_diff() {
+	if ! diff -u "$1" "$2"
+	then
+		echo "$3"
+		false
+	fi
+}
+
+verify_merge() {
+	verify_diff "$2" "$1" "[OOPS] bad merge result" &&
+	if test $(git ls-files -u | wc -l) -gt 0
+	then
+		echo "[OOPS] unmerged files"
+		false
+	fi &&
+	if ! git diff --exit-code
+	then
+		echo "[OOPS] working tree != index"
+		false
+	fi &&
+	if test -n "$3"
+	then
+		git show -s --pretty=format:%s HEAD >msg.act &&
+		verify_diff "$3" msg.act "[OOPS] bad merge message"
+	fi
+}
+
+verify_head() {
+	if test "$1" != "$(git rev-parse HEAD)"
+	then
+		echo "[OOPS] HEAD != $1"
+		false
+	fi
+}
+
+verify_parents() {
+	i=1
+	while test $# -gt 0
+	do
+		if test "$1" != "$(git rev-parse HEAD^$i)"
+		then
+			echo "[OOPS] HEAD^$i != $1"
+			return 1
+		fi
+		i=$(expr $i + 1)
+		shift
+	done
+}
+
+verify_mergeheads() {
+	i=1
+	if ! test -f .git/MERGE_HEAD
+	then
+		echo "[OOPS] MERGE_HEAD is missing"
+		false
+	fi &&
+	while test $# -gt 0
+	do
+		head=$(head -n $i .git/MERGE_HEAD | tail -n 1)
+		if test "$1" != "$head"
+		then
+			echo "[OOPS] MERGE_HEAD $i != $1"
+			return 1
+		fi
+		i=$(expr $i + 1)
+		shift
+	done
+}
+
+verify_no_mergehead() {
+	if test -f .git/MERGE_HEAD
+	then
+		echo "[OOPS] MERGE_HEAD exists"
+		false
+	fi
+}
+
+
+test_expect_success 'setup' '
+	git add file &&
+	test_tick &&
+	git commit -m "commit 0" &&
+	git tag c0 &&
+	c0=$(git rev-parse HEAD) &&
+
+	cp file.1 file &&
+	git add file &&
+	test_tick &&
+	git commit -m "commit 1" &&
+	git tag c1 &&
+	c1=$(git rev-parse HEAD) &&
+	test_tick &&
+
+	git reset --hard "$c0" &&
+	cp file.5 file &&
+	git add file &&
+	git commit -m "commit 2" &&
+	test_tick &&
+	git tag c2 &&
+	c2=$(git rev-parse HEAD) &&
+
+	git reset --hard "$c0" &&
+	cp file.9 file &&
+	git add file &&
+	test_tick &&
+	git commit -m "commit 3" &&
+	git tag c3 &&
+	c3=$(git rev-parse HEAD) &&
+	test_tick &&
+
+	git reset --hard "$c1" &&
+	cat extend >>file &&
+	git add file &&
+	git commit -m "commit 4" &&
+	git tag x1 &&
+	x1=$(git rev-parse HEAD) &&
+	test_tick &&
+
+	git reset --hard "$c1" &&
+	git merge "$c2" &&
+	git tag x0 &&
+	x0=$(git rev-parse HEAD) &&
+	test_tick &&
+
+	git reset --hard "$c2" &&
+	cat extend >>file &&
+	git add file &&
+	git commit -m "commit 5" &&
+	git tag x2 &&
+	x2=$(git rev-parse HEAD) &&
+	test_tick &&
+
+	git reset --hard "$x1" &&
+	git merge "$x0" &&
+	git tag y1 &&
+	y1=$(git rev-parse HEAD) &&
+	test_tick &&
+
+	git reset --hard "$x0" &&
+	git merge "$x2" &&
+	git tag y2 &&
+	y2=$(git rev-parse HEAD) &&
+	test_tick &&
+
+	git reset --hard "$y1" &&
+	git merge "$y2" &&
+	git tag y3 &&
+	y3=$(git rev-parse HEAD) &&
+	test_tick &&
+	git reset --hard "$c0" &&
+	create_merge_msgs &&
+
+	git reset --hard x1 &&
+	git clone .git clone &&
+	git config remote.clone.url clone &&
+	git config remote.clone.fetch "+refs/heads/*:refs/remotes/clone/*" &&
+
+	(mkdir new && cd new && git init && cp ../file.9 file2 && git add
file2 && test_tick && git commit -m "commit new") &&
+	git config remote.new.url new &&
+	git config remote.new.fetch "+refs/heads/*:refs/remotes/new/*"
+'
+
+test_debug 'gitk --all'
+
+test_expect_success 'merge c0 with c1 (ff-only overrides no-ff)' '
+	git reset --hard c0 &&
+	git config branch.master.mergeoptions "--no-ff" &&
+	git merge --ff=only c1 &&
+	verify_merge file result.1 &&
+	verify_head $c1
+'
+
+test_debug 'gitk --all'
+
+test_expect_success 'merge c0 with c1 (--ff=only in config)' '
+	git reset --hard c0 &&
+	git config branch.master.mergeoptions "--ff=only" &&
+	git merge c1 &&
+	test_tick &&
+	verify_merge file result.1 &&
+	verify_head $c1
+'
+
+test_debug 'gitk --all'
+
+test_expect_success 'merge c1 with c0 (--ff=only in config)' '
+	git reset --hard c1 &&
+	git config branch.master.mergeoptions "--ff=only" &&
+	git merge c0 &&
+	verify_merge file result.1 &&
+	verify_head $c1
+'
+
+test_debug 'gitk --all'
+
+test_expect_success 'merge c1 with c2 (--ff=only in config)' '
+	git reset --hard c1 &&
+	test_tick &&
+	git config branch.master.mergeoptions "--ff=only" &&
+	if git merge c2
+	then
+		false
+	else
+		verify_merge file result.1 &&
+		verify_head $c1
+	fi
+'
+
+test_debug 'gitk --all'
+
+test_expect_success 'merge c0 with c1 (--ff=only)' '
+	git reset --hard c0 &&
+	test_tick &&
+	git merge --ff=only c1 &&
+	verify_merge file result.1 &&
+	verify_head $c1
+'
+
+test_debug 'gitk --all'
+
+test_expect_success 'merge c1 with c0 (--ff=only)' '
+	git reset --hard c1 &&
+	test_tick &&
+	git merge --ff=only c0 &&
+	verify_merge file result.1 &&
+	verify_head $c1
+'
+
+test_debug 'gitk --all'
+
+test_expect_success 'merge c0 with c1 and c2 (--ff=only)' '
+	git reset --hard c0 &&
+	if git --ff=only merge c1 c2
+	then
+		false
+	else
+		verify_merge file result.0 &&
+		verify_head $c0
+	fi
+'
+
+test_debug 'gitk --all'
+
+test_expect_success 'merge c1 with c0 (--ff=only)' '
+	git reset --hard c1 &&
+	test_tick &&
+	git merge --ff=only c0 &&
+	verify_merge file result.1 &&
+	verify_head $c1
+'
+
+test_debug 'gitk --all'
+
+test_expect_success 'merge c1 with c2 (--ff=only overrides no-ff)' '
+	git reset --hard c1 &&
+	git config branch.master.mergeoptions "--no-ff" &&
+	test_tick &&
+	if git merge c2 --ff=only
+	then
+		false
+	else
+		verify_merge file result.1 &&
+		verify_head $c1
+	fi
+'
+
+test_debug 'gitk --all'
+
+test_expect_success 'merge c0 with c1 (no-ff overrides --ff=only)' '
+	git reset --hard c0 &&
+	git config branch.master.mergeoptions "--ff=only" &&
+	test_tick &&
+	git merge --no-ff c1 &&
+	verify_merge file result.1 &&
+	verify_parents $c0 $c1
+'
+
+test_debug 'gitk --all'
+
+test_expect_success 'merge c1 with c2 (ff owerrides --ff=only)' '
+	git reset --hard c1 &&
+	git config branch.master.mergeoptions "--ff=only" &&
+	test_tick &&
+	git merge --ff c2 &&
+	verify_merge file result.1-5 &&
+	verify_parents $c1 $c2
+'
+
+test_debug 'gitk --all'
+
+test_expect_success 'merge c0 with c1 and c2' '
+	git reset --hard c0 &&
+	git config branch.master.mergeoptions "" &&
+	test_tick &&
+	git merge c1 c2 &&
+	verify_merge file result.1-5 &&
+	verify_parents $c1 $c2
+'
+
+test_debug 'gitk --all'
+
+test_expect_success 'merge c1 with c0, c2, c0, and c1' '
+	git reset --hard c1 &&
+	git config branch.master.mergeoptions "" &&
+	test_tick &&
+	git merge c0 c2 c0 c1 &&
+	verify_merge file result.1-5 &&
+	verify_parents $c1 $c2
+'
+
+test_debug 'gitk --all'
+
+test_expect_success 'merge y2 with x0, c3, and c0' '
+	git reset --hard y2 &&
+	git config branch.master.mergeoptions "" &&
+	test_tick &&
+	git merge x0 c3 c0 &&
+	verify_merge file result.1-5-9-13 &&
+	verify_parents $y2 $c3
+'
+
+test_debug 'gitk --all'
+
+test_expect_success 'merge x0 with y2, c3, and c0' '
+	git reset --hard x0 &&
+	git config branch.master.mergeoptions "" &&
+	test_tick &&
+	git merge y2 c3 c0 &&
+	verify_merge file result.1-5-9-13 &&
+	verify_parents $y2 $c3
+'
+
+test_debug 'gitk --all'
+
+test_done
-- 
1.5.3.3

-- 
Sverre Hvammen Johansen

Re: [RFC/PATCH] Fast forward strategies allow, never, and only

[Sverre Hvammen Johansen]

Hi,

I have split the original patch I had for the fast forward strategies
in two.  I hope to get something close to this patch accepted.  I need
this feature for the Accurev integration I am working on.  I will be
able to spend time on this the next two weeks.

-- 
Sverre Hvammen Johansen

Re: [RFC/PATCH] Fast forward strategies allow, never, and only

[Ping Yin]

On Tue, Mar 11, 2008 at 11:18 AM, Sverre Hvammen Johansen
<hvammen@gmail.com> wrote:
> Hi,
>
>  I have split the original patch I had for the fast forward strategies
>  in two.  I hope to get something close to this patch accepted.  I need
>  this feature for the Accurev integration I am working on.  I will be
>  able to spend time on this the next two weeks.
>

Expecting it will be accepted. It's a useful feature for me.

>
>  --
>  Sverre Hvammen Johansen
>  --
>  To unsubscribe from this list: send the line "unsubscribe git" in
>  the body of a message to majordomo@vger.kernel.org
>  More majordomo info at  http://vger.kernel.org/majordomo-info.html
>



-- 
Ping Yin

Re: [RFC/PATCH] Fast forward strategies allow, never, and only

[Junio C Hamano]

"Sverre Hvammen Johansen" <hvammen@gmail.com> writes:

>>From 795bd1b3e70f011b675061ecae322527a9f0695c Mon Sep 17 00:00:00 2001
> From: Sverre Hvammen Johansen <sj@gmail.com>
> Date: Sun, 9 Mar 2008 21:43:56 -0800
> Subject: [PATCH] Fast forward strategies allow, never, and only.

Please do not do this.  The first line is not part of anything but is only
a mail message boundary in mbox format.  Reproducing From: is fine
especially if the patch author is different from the person who is sending
the patch, but I do not think it is necessary in this case.  Date and
Subject should also go, as taking them from the e-mail header is just as
good.

> New fast forward strategies, only, is introduced.  This new fast
> forward strategy prevents real merges.

"What it does".

> FF strategy "only" fails if the specified heads and HEAD can not
> be reduced down to only one real parent.  The only allowed
> outcome is a fast forward unless HEAD is up to date with
> the specified heads.

"What it does" continues.

> This patch also uses the real heads found instead of those
> specified for real merges.  This means that merge startegies
> that only take two heads can now accept more than two heads
> if they can be reduced down to only two real heads.  However,
> fast-forward of head in combination with a real merge is
> handled as before.

"What it does" continues further.

What's lacking is "why this is a good idea".

> Signed-off-by: Sverre Hvammen Johansen <sj@black.local>

Please check "[user] email" section in your .git/config file.

> @@ -0,0 +1,16 @@
> +FAST FORWARD STRATEGIES
> +-----------------------

In the context of "merge", the word "strategy" is already taken to mean
something quite different, and I am afraid that reusing the word may cause
confusion.  Probably this should be called fast forward options.

> +allow::
> +	Do not generate a merge commit if the merge resolved
> +	as a fast-forward, only update the branch pointer.
> +	This is the default behavior of git-merge.

Makes one wonder if git-pull uses different default from the default for
the git-merge command.

> +never::
> +	Generate a merge commit even if the merge resolved
> +	as a fast-forward.
> +
> +only::
> +	Only allow a fast-forward.  The merge will fail
> +	unless HEAD is up to date or the merge resolved as
> +        a fast-forward.

Funny indentation on the last line...

> @@ -29,12 +29,13 @@
>
>  --no-ff::
>  	Generate a merge commit even if the merge resolved as a
> -	fast-forward.
> +	fast-forward.  --on-ff is an alias for --ff=never.

Really?

> @@ -153,8 +153,8 @@ parse_config () {
>  		--summary)
>  			show_diffstat=t ;;
>  		--squash)
> -			test "$allow_fast_forward" = t ||
> -				die "You cannot combine --squash with --no-ff."
> +			test "$fast_forward" = allow ||
> +				die "You cannot combine --squash with --ff=never."

Why does the user get this message after saying --ff=only?

>  			squash=t no_commit=t ;;
>  		--no-squash)
>  			squash= no_commit= ;;
> @@ -163,11 +163,28 @@ parse_config () {
>  		--no-commit)
>  			no_commit=t ;;
>  		--ff)
> -			allow_fast_forward=t ;;
> +			case "$2" in
> +			allow|never|only)
> +				fast_forward=$2 squash= no_commit= ; shift ;;
> +			-*)
> +				fast_forward=allow squash= no_commit= ;;
> +			*)
> +				die "available fast-forward strategies are: allow, newer, and only" ;;

Shouldn't "squash= no_commit=" be shared across case arms?

How does this code parse "git merge --ff my_other_branch"?

Shouldn't you issue the same error message for these two inputs?

	"git merge --ff=never --squash" 
	"git merge --squash --ff=never"

> +		--ff=*)
> +			fast_forward=$(echo $1 |cut -d = -f 2) squash= no_commit=

I do not know the reason why, and theoretically there shouldn't be any
correlation, but somehow I see "cut" used more often in shell scripts that
are sloppily done.

At least you would need to quote "$1" above (and ideally protect yourself
against nonsense input like "--ff=-n"), but in this case, I think it is
simpler to say:

	fast_forward=${1#--ff=}

> @@ -279,24 +296,125 @@ do
>  done
>  set x $remoteheads ; shift
>
> +echo "$head" >"$GIT_DIR/ORIG_HEAD"
> +
> +find_one_real_parent () {

There are too many leftover debugging output in this function.

> +	while test $# -gt 0
> +	do
> +		if test $real_parent = $1
> +		then
> +			# Found a parent that is equal to the real
> +			# parent candidate
> +			echo "Duplicate $(git rev-parse --short $1)"
> +			echo "Ignoring $1"
> +		else
> +			common_b=$(git merge-base --all $real_parent $1)
> +		
> +			if test "$common_b" = $1
> +			then
> +				# Found a parent that is not
> +				# independent of the real parent
> +				# candidate
> +				echo "Possible ff $(git rev-parse --short $1)..$(git rev-parse
> --short $real_parent)."

Linewrapped by MUA.

> +find_real_parents () {
> +    find_one_real_parent $head "$@"
> +    ff_head=$real_parent
> +    real_parents=
> +
> +    while test -n "$other_parents"
> +    do
> +	find_one_real_parent $other_parents
> +	real_parents="$real_parents $real_parent"
> +    done
> +}

What does this complex double loop compute differently from what "git
show-branch --independent" gives you?  Aside from that you will run slower
but you can take more than 25 branches?

More generally, I doubt it is really useful to let the user throw millions
of potentially duplicate refs and have the merge silently record a
filtered out results.  Yes, you made the process of culling duplicates too
chatty in the above part of the patch, and fmt-merge-msg will hopefully
still show what the user gave on the command line, but the heads used by
the real merge process now is very different from it.  The merge comment
is totally disconnected from the reality.  Why is this an improvement?

If the goal is to allow Octopus that is more complex than the simplest
kind, don't.  Octopus was deliberately written to allow the most simple
kind and nothing else for a reason: bisectability.

The user should know what he is merging; throwing many heads that he does
not even know how they relate to each other, and call the resulting mess a
merge feels like a sure way to encourage a bad workflow.  Perhaps I am
missing something obvious that you are trying to automate, but I do not
see it, as there were no justification in the proposed commit log message
nor in the documentation.

> +if test $fast_forward = never -o

-o?

> +then
> +	real_parents="$@"
> +	ff_head=$head
> +else
> +	find_real_parents "$@"
> +fi

This part is simply unacceptable.  At least please do not needlessly call
find_real_parents in the most common case of giving only one remote head.

> diff --git a/t/t7601-merge-ff-strategies.sh b/t/t7601-merge-ff-strategies.sh
> new file mode 100755
> index 0000000..6c0a91a
> --- /dev/null
> +++ b/t/t7601-merge-ff-strategies.sh
> @@ -0,0 +1,549 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2007 Lars Hjemli
> +#

Really?

> -- 
> 1.5.3.3

Makes reviewers suspect that the patch submitter is not keeping up to
date.

Re: [RFC/PATCH] Fast forward strategies allow, never, and only

[Sverre Hvammen Johansen]

On Mon, Mar 10, 2008 at 10:19 PM, Junio C Hamano <gitster@pobox.com> wrote:
> "Sverre Hvammen Johansen" <hvammen@gmail.com> writes:
>
>  > @@ -153,8 +153,8 @@ parse_config () {
>  >               --summary)
>  >                       show_diffstat=t ;;
>  >               --squash)
>  > -                     test "$allow_fast_forward" = t ||
>  > -                             die "You cannot combine --squash with --no-ff."
>  > +                     test "$fast_forward" = allow ||
>  > +                             die "You cannot combine --squash with --ff=never."
>
>  Why does the user get this message after saying --ff=only?

Bug.  What should the semantic be?  To me it makes sense that a squash
oweride the ff options instead of giving a error, but specifying a ff
option after --squash is an error.

>  How does this code parse "git merge --ff my_other_branch"?

It is getting late so I need to get back to you about this.

>  Shouldn't you issue the same error message for these two inputs?
>
>         "git merge --ff=never --squash"
>         "git merge --squash --ff=never"

Allow the first one since --ff=never can be in the config file and
give error on the last one.

>  What does this complex double loop compute differently from what "git
>  show-branch --independent" gives you?  Aside from that you will run slower
>  but you can take more than 25 branches?

The main issue is that show-branch --independent does not give me the
desired order for these branches.  I want the first branch to be head
or something that can be fast forwarded from head.  The second branch
should be the next branch in the specified list that have not been
eliminated or something that can be fast forwarded from this, and so
on and so forth.  This is an absolute requirement for the first
argument (head).  If show-branch had a documented order and meet the
absolute requirement above I would prefer show-branch --independentr
instead of this nasty loop.

>  More generally, I doubt it is really useful to let the user throw millions
>  of potentially duplicate refs and have the merge silently record a
>  filtered out results.  Yes, you made the process of culling duplicates too
>  chatty in the above part of the patch, and fmt-merge-msg will hopefully
>  still show what the user gave on the command line, but the heads used by
>  the real merge process now is very different from it.  The merge comment
>  is totally disconnected from the reality.  Why is this an improvement?

We already do this in the case where we have head pluss one branch.
If it results in only one real parent we throw one of them away
resulting in a fast forward or an "up to date".  The suggested patch
is just a generalization over this to the case where we have head and
more than one branch.

>  If the goal is to allow Octopus that is more complex than the simplest
>  kind, don't.  Octopus was deliberately written to allow the most simple
>  kind and nothing else for a reason: bisectability.

That is not the goal.

>  The user should know what he is merging; throwing many heads that he does
>  not even know how they relate to each other, and call the resulting mess a
>  merge feels like a sure way to encourage a bad workflow.

We do merges all the time without knowing what we actually are
merging.  That is something that happen in many work flows.  I assume
that you don't want a real merge in the case that you are "up to date"
with your remote or your head can be fast forward.  For the users
convenience we do a fast forward or report it to be "up to date".
Exactly the same argument holds where there are more than one remote
involved.  The user may not know who is ahead and who is behind and he
usually want the commit to record a simple history as possible.

>  > +then
>  > +     real_parents="$@"
>  > +     ff_head=$head
>  > +else
>  > +     find_real_parents "$@"
>  > +fi
>
>  This part is simply unacceptable.  At least please do not needlessly call
>  find_real_parents in the most common case of giving only one remote head.

I now keep common_b in common so subsequent calls to git merge-base
--all can be optimized away for the most common case.

-- 
Sverre Hvammen Johansen

Re: [RFC/PATCH] Fast forward strategies allow, never, and only

[Sverre Hvammen Johansen]

On Tue, Mar 11, 2008 at 9:46 PM, Sverre Hvammen Johansen
<hvammen@gmail.com> wrote:
>  To me it makes sense that a squash oweride the ff options instead
>  of giving a error, but specifying a ff option after --squash is an error.

After some consideration I am convinced we should allow --squash
combined with any of the --ff options.  The same for --no-commit.
--squash combined with any of the --ff options means that the index
and the tree will only be updated if a merge without --squash would
have done the same.

That mean that t7600-merge.sh will break as follows:

* FAIL 19: combining --squash and --no-ff is refused

                test_must_fail git merge --squash --no-ff c1 &&
                test_must_fail git merge --no-ff --squash c1

-- 
Sverre Hvammen Johansen

Re: [RFC/PATCH] Fast forward strategies allow, never, and only

[Sverre Hvammen Johansen]

On Mon, Mar 10, 2008 at 10:19 PM, Junio C Hamano <gitster@pobox.com> wrote:
> "Sverre Hvammen Johansen" <hvammen@gmail.com> writes:
>  >               --ff)
>  > -                     allow_fast_forward=t ;;
>  > +                     case "$2" in
>  > +                     allow|never|only)
>  > +                             fast_forward=$2 squash= no_commit= ; shift ;;
>  > +                     -*)
>  > +                             fast_forward=allow squash= no_commit= ;;
>  > +                     *)
>  > +                             die "available fast-forward strategies are: allow, newer, and only" ;;
>
>  How does this code parse "git merge --ff my_other_branch"?

git rev-parse (in git-sh-setup.sh) will rewrite this to "git merge
--ff -- my_other_branch".  However, it will also rewrite "git merge
--ff=only my_other_branch" to "git merge --ff only --
my_other_branch".  Options in the config file are parsed directly by
parse_config without these rewrites.  This means that second case
above is the case where --ff don't have any arguments.  First and last
case is the case where --ff have an argument.

-- 
Sverre Hvammen Johansen

Re: [RFC/PATCH] Fast forward strategies allow, never, and only

[Jakub Narebski]

"Sverre Hvammen Johansen" <hvammen@gmail.com> writes:

> +FAST FORWARD STRATEGIES
> +-----------------------

I think this should be named FAST FORWARD OPTIONS or something like
that.

> +
> +allow::
> +	Do not generate a merge commit if the merge resolved
> +	as a fast-forward, only update the branch pointer.
> +	This is the default behavior of git-merge.

This is equivalent of current '--ff'; perhaps this should be mentioned
as well in this option description.

> +never::
> +	Generate a merge commit even if the merge resolved
> +	as a fast-forward.

This is equivalent of current '--no-ff'; nevertheless I think that it
would be better to name this strategy 'commit' or 'merge', as in
--ff=merge, or --ff=commit.

> +only::
> +	Only allow a fast-forward.  The merge will fail
> +	unless HEAD is up to date or the merge resolved as
> +     a fast-forward.

This is equivalent of '--ff-only' or '--strategy=ff'... Errr...
I'm sorry, such option does not exist, and it would be I guess
useful addition to default non '+' fetch refspec allowing fast-forward
only, and to receive.denyNonFastForwards to control push behavior.


>  --no-ff::
>  	Generate a merge commit even if the merge resolved as a
> -	fast-forward.
> +	fast-forward.  --on-ff is an alias for --ff=never.

Typo: '--on-ff' instead of '--no-ff'.

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [RFC/PATCH] Fast forward strategies allow, never, and only

[Sverre Hvammen Johansen]

On Tue, Mar 11, 2008 at 1:15 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>  > +never::
>  > +     Generate a merge commit even if the merge resolved
>  > +     as a fast-forward.
>
>  This is equivalent of current '--no-ff'; nevertheless I think that it
>  would be better to name this strategy 'commit' or 'merge', as in
>  --ff=merge, or --ff=commit.

If there is consensus to change this I will.

>  > +only::
>  > +     Only allow a fast-forward.  The merge will fail
>  > +     unless HEAD is up to date or the merge resolved as
>  > +     a fast-forward.
>
>  This is equivalent of '--ff-only' or '--strategy=ff'... Errr...
>  I'm sorry, such option does not exist, and it would be I guess
>  useful addition to default non '+' fetch refspec allowing fast-forward
>  only, and to receive.denyNonFastForwards to control push behavior.

I agree, but it is over my head to implement this now.

-- 
Sverre Hvammen Johansen

Re: [RFC/PATCH] Fast forward strategies allow, never, and only

[Junio C Hamano]

"Sverre Hvammen Johansen" <hvammen@gmail.com> writes:

> On Tue, Mar 11, 2008 at 1:15 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>>  > +never::
>>  > +     Generate a merge commit even if the merge resolved
>>  > +     as a fast-forward.
>>
>>  This is equivalent of current '--no-ff'; nevertheless I think that it
>>  would be better to name this strategy 'commit' or 'merge', as in
>>  --ff=merge, or --ff=commit.
>
> If there is consensus to change this I will.

I do not think Jakub's suggestion makes much sense.  If --ff stands for
"fast forward", then --ff=merge could be explained (very unnaturally) as
"(in a) fast forward (situation, create a) merge", which might make some
sense as an incomplete sentence, but I cannot explain "commit" like that
even with a broken sentence.

Using "fast forward" as a verb ("instead of creating a needless merge,
just move the head"), then the mode of operations your patch proposes can
be described much clearer.  "Never fast forward, always create an
artificial merge if needed", "Only fast forward is allowed, never advance
this head by creating a true merge", etc.

So I think the wording is fine.  What's more necessary in the documention
is how and why these restrictions are useful in what situations, workflows
and management policies.

Re: [RFC/PATCH] Fast forward strategies allow, never, and only

[Sverre Hvammen Johansen]

On Tue, Mar 11, 2008 at 8:50 PM, Junio C Hamano <gitster@pobox.com> wrote:
>  So I think the wording is fine.  What's more necessary in the documention
>  is how and why these restrictions are useful in what situations, workflows
>  and management policies.

I agree.  I plan a revised patch later this week.  I may need some
help with the documentation.

-- 
Sverre Hvammen Johansen