Re: [PATCH] Adding rebase merge strategy

Re: [PATCH] Adding rebase merge strategy

[Tom Clarke]

On 8/30/07, Tom Clarke <tom@u2i.com> wrote:
> +               ( git log --pretty=format:"%s" ) >actual &&
> +       (
> +               echo "onbranch"
> +               echo "update"
> +               echo -n "initial"
> +       ) >expected &&
> +       git diff -w -u expected actual

One issue that I'm curious about. Is it expected that the git log
format above doesn't finish with a new line? I couldn't get this test
to work otherwise.

-Tom

Re: [PATCH] Adding rebase merge strategy

[Johannes Schindelin]

Hi,

On Thu, 30 Aug 2007, Tom Clarke wrote:

> On 8/30/07, Tom Clarke <tom@u2i.com> wrote:
> > +               ( git log --pretty=format:"%s" ) >actual &&
> > +       (
> > +               echo "onbranch"
> > +               echo "update"
> > +               echo -n "initial"
> > +       ) >expected &&
> > +       git diff -w -u expected actual
> 
> One issue that I'm curious about. Is it expected that the git log
> format above doesn't finish with a new line? I couldn't get this test
> to work otherwise.

Indeed, it does not end in a newline.  However, I'd solve the issue 
differently:

- I'd use --pretty=oneline (and use test_tick before every commit, to 
  guarantee consistent object names).

- If you _have_ to keep your way, please use "printf", since "echo -n" is 
  not portable AFAIK.

- Or just do "echo > actual".  And use "cat << EOF" like in most of the 
  other tests.

Ciao,
Dscho

[PATCH] Adding rebase merge strategy

[Tom Clarke]

In addition to adding git-merge-rebase.sh, git-merge.sh is modified
to handle the rebase strategy specially and avoids running update-ref
as rebase won't generate a merge commit.

Signed-off-by: Tom Clarke <tom@u2i.com>
---

Re-submitting this patch as we were in a pre-release freeze last time I submitted

 .gitignore                         |    1 +
 Documentation/merge-strategies.txt |    6 ++++++
 Makefile                           |    2 +-
 git-merge-rebase.sh                |   34 ++++++++++++++++++++++++++++++++++
 git-merge.sh                       |   14 +++++++++++---
 t/t3031-merge-rebase.sh            |   34 ++++++++++++++++++++++++++++++++++
 6 files changed, 87 insertions(+), 4 deletions(-)
 create mode 100755 git-merge-rebase.sh
 create mode 100755 t/t3031-merge-rebase.sh

diff --git a/.gitignore b/.gitignore
index e0b91be..fe5cdc4 100644
--- a/.gitignore
+++ b/.gitignore
@@ -73,6 +73,7 @@ git-merge-tree
 git-merge-octopus
 git-merge-one-file
 git-merge-ours
+git-merge-rebase
 git-merge-recursive
 git-merge-resolve
 git-merge-stupid
diff --git a/Documentation/merge-strategies.txt b/Documentation/merge-strategies.txt
index 7df0266..dff1909 100644
--- a/Documentation/merge-strategies.txt
+++ b/Documentation/merge-strategies.txt
@@ -33,3 +33,9 @@ ours::
 	merge is always the current branch head.  It is meant to
 	be used to supersede old development history of side
 	branches.
+
+rebase::
+	This rebases the current branch based on a single head.
+	Commits are rewritten as with git-rebase. This doesn't
+	produce a merge. The procedure for dealing with conflicts 
+	is the same as with git-rebase.
diff --git a/Makefile b/Makefile
index 8db4dbe..e6d3812 100644
--- a/Makefile
+++ b/Makefile
@@ -215,7 +215,7 @@ SCRIPT_SH = \
 	git-sh-setup.sh \
 	git-am.sh \
 	git-merge.sh git-merge-stupid.sh git-merge-octopus.sh \
-	git-merge-resolve.sh git-merge-ours.sh \
+	git-merge-resolve.sh git-merge-ours.sh git-merge-rebase.sh \
 	git-lost-found.sh git-quiltimport.sh git-submodule.sh \
 	git-filter-branch.sh \
 	git-stash.sh
diff --git a/git-merge-rebase.sh b/git-merge-rebase.sh
new file mode 100755
index 0000000..fc07331
--- /dev/null
+++ b/git-merge-rebase.sh
@@ -0,0 +1,34 @@
+#!/bin/sh
+#
+# Copyright (c) 2005 Linus Torvalds
+# Copyright (c) 2007 Tom Clarke
+#
+# Resolve two trees with rebase
+
+# The first parameters up to -- are merge bases; the rest are heads.
+bases= head= remotes= sep_seen=
+for arg
+do
+	case ",$sep_seen,$head,$arg," in
+	*,--,)
+		sep_seen=yes
+		;;
+	,yes,,*)
+		head=$arg
+		;;
+	,yes,*)
+		remotes="$remotes$arg "
+		;;
+	*)
+		bases="$bases$arg "
+		;;
+	esac
+done
+
+# Give up if we are given two or more remotes -- not handling octopus.
+case "$remotes" in
+?*' '?*)
+	exit 2 ;;
+esac
+
+git rebase $remotes || exit 2
diff --git a/git-merge.sh b/git-merge.sh
index 6c513dc..ea3cc16 100755
--- a/git-merge.sh
+++ b/git-merge.sh
@@ -16,11 +16,12 @@ test -z "$(git ls-files -u)" ||
 LF='
 '
 
-all_strategies='recur recursive octopus resolve stupid ours subtree'
+all_strategies='recur recursive octopus resolve stupid ours subtree rebase'
 default_twohead_strategies='recursive'
 default_octopus_strategies='octopus'
 no_fast_forward_strategies='subtree ours'
-no_trivial_strategies='recursive recur subtree ours'
+no_trivial_strategies='recursive recur subtree ours rebase'
+no_update_ref='rebase'
 use_strategies=
 
 allow_fast_forward=t
@@ -81,11 +82,18 @@ finish () {
 			echo "No merge message -- not updating HEAD"
 			;;
 		*)
-			git update-ref -m "$rlogm" HEAD "$1" "$head" || exit 1
+			case " $wt_strategy " in
+			*" $no_update_ref "*)
+				;;
+			*)
+				git update-ref -m "$rlogm" HEAD "$1" "$head" || exit 1
+				;;
+			esac
 			;;
 		esac
 		;;
 	esac
+
 	case "$1" in
 	'')
 		;;
diff --git a/t/t3031-merge-rebase.sh b/t/t3031-merge-rebase.sh
new file mode 100755
index 0000000..8e3641d
--- /dev/null
+++ b/t/t3031-merge-rebase.sh
@@ -0,0 +1,34 @@
+#!/bin/sh
+
+test_description='merge-rebase backend test'
+
+. ./test-lib.sh
+
+test_expect_success 'merging using rebase does not create merge commit' '
+	echo hello >a &&
+	git add a &&
+	test_tick && git commit -m initial &&
+
+	git checkout -b branch &&
+	echo hello >b &&
+	git add b &&
+	test_tick && git commit -m onbranch &&
+
+	git checkout master &&
+	echo update >a &&
+	git add a &&
+	test_tick && git commit -m update &&
+
+	git checkout branch &&
+	git merge -s rebase master >expect &&
+
+       	( git log --pretty=oneline ) >actual &&
+	(
+		echo "4db7a5a013e67aa623d1fd294e8d46e89b3ace8f onbranch"
+		echo "893371811dbd13e85c098b72d1ab42bcfd24c2db update"
+		echo "0e960b10429bf3f1e168ee2cc7d531ac7c622580 initial"
+	) >expected &&
+	git diff -w -u expected actual
+'
+
+test_done
-- 
1.5.3.rc7.3.g850f-dirty

Re: [PATCH] Adding rebase merge strategy

[Johannes Schindelin]

Hi,

On Fri, 28 Sep 2007, Tom Clarke wrote:

> diff --git a/git-merge-rebase.sh b/git-merge-rebase.sh
> new file mode 100755
> index 0000000..fc07331
> --- /dev/null
> +++ b/git-merge-rebase.sh
> @@ -0,0 +1,34 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2005 Linus Torvalds

Really?

> +# Copyright (c) 2007 Tom Clarke
> +#
> +# Resolve two trees with rebase
> +
> +# The first parameters up to -- are merge bases; the rest are heads.
> +bases= head= remotes= sep_seen=
> +for arg
> +do
> +	case ",$sep_seen,$head,$arg," in
> +	*,--,)
> +		sep_seen=yes
> +		;;
> +	,yes,,*)
> +		head=$arg
> +		;;
> +	,yes,*)
> +		remotes="$remotes$arg "
> +		;;
> +	*)
> +		bases="$bases$arg "
> +		;;
> +	esac
> +done
> +
> +# Give up if we are given two or more remotes -- not handling octopus.
> +case "$remotes" in
> +?*' '?*)
> +	exit 2 ;;
> +esac

You can check that much earlier, no?  IOW something like

	while test $# != 0
	do
		case "$1" in --) break ;; esac
		shift
	done

	test $# = 3 || die "merge stragey rebase needs exactly one ref"

	git rebase "$3"

(It's not like you need the variables...) Hmm?

> +git rebase $remotes || exit 2
> diff --git a/git-merge.sh b/git-merge.sh
> index 6c513dc..ea3cc16 100755
> --- a/git-merge.sh
> +++ b/git-merge.sh
> @@ -81,11 +82,18 @@ finish () {
>  			echo "No merge message -- not updating HEAD"
>  			;;
>  		*)
> -			git update-ref -m "$rlogm" HEAD "$1" "$head" || exit 1
> +			case " $wt_strategy " in
> +			*" $no_update_ref "*)
> +				;;
> +			*)
> +				git update-ref -m "$rlogm" HEAD "$1" "$head" || exit 1
> +				;;
> +			esac

You may want to warn earlier, if a message was supplied with -s rebase, or 
even error out.

> diff --git a/t/t3031-merge-rebase.sh b/t/t3031-merge-rebase.sh
> new file mode 100755
> index 0000000..8e3641d
> --- /dev/null
> +++ b/t/t3031-merge-rebase.sh
> @@ -0,0 +1,34 @@
> +#!/bin/sh
> +
> +test_description='merge-rebase backend test'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'merging using rebase does not create merge commit' '
> +	echo hello >a &&
> +	git add a &&
> +	test_tick && git commit -m initial &&
> +
> +	git checkout -b branch &&
> +	echo hello >b &&
> +	git add b &&
> +	test_tick && git commit -m onbranch &&
> +
> +	git checkout master &&
> +	echo update >a &&
> +	git add a &&
> +	test_tick && git commit -m update &&

I like to have something like this in a "test_expect_success setup" part, 
and then have the meat of the test in its own test case.

But hey, you're the author, it's for you to decide.

Thanks,
Dscho

Re: [PATCH] Adding rebase merge strategy

[Tom Clarke]

On 9/28/07, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > +# Copyright (c) 2005 Linus Torvalds
>
> Really?
Well 90% of the file was copy and pasted from one of the other merge
strategies which was indeed written by Linus. So it seemed
appropriate. If that's not appropriate attribution I can remove :-).

-Tom

[PATCH] Adding rebase merge strategy

[Tom Clarke]

In addition to adding git-merge-rebase.sh, git-merge.sh is modified
to handle the rebase strategy specially and avoids running update-ref
as rebase won't generate a merge commit. It also adds a warning
if a message is supplied when the rebase isn't used as this
will be ignored.

Signed-off-by: Tom Clarke <tom@u2i.com>
---

Incorporated comments from Johannes Schindlen.

 .gitignore                         |    1 +
 Documentation/merge-strategies.txt |    6 +++++
 Makefile                           |    2 +-
 git-merge-rebase.sh                |   17 ++++++++++++++
 git-merge.sh                       |   24 +++++++++++++++++--
 t/t3031-merge-rebase.sh            |   44 ++++++++++++++++++++++++++++++++++++
 6 files changed, 90 insertions(+), 4 deletions(-)
 create mode 100755 git-merge-rebase.sh
 create mode 100755 t/t3031-merge-rebase.sh

diff --git a/.gitignore b/.gitignore
index e0b91be..fe5cdc4 100644
--- a/.gitignore
+++ b/.gitignore
@@ -73,6 +73,7 @@ git-merge-tree
 git-merge-octopus
 git-merge-one-file
 git-merge-ours
+git-merge-rebase
 git-merge-recursive
 git-merge-resolve
 git-merge-stupid
diff --git a/Documentation/merge-strategies.txt b/Documentation/merge-strategies.txt
index 7df0266..dff1909 100644
--- a/Documentation/merge-strategies.txt
+++ b/Documentation/merge-strategies.txt
@@ -33,3 +33,9 @@ ours::
 	merge is always the current branch head.  It is meant to
 	be used to supersede old development history of side
 	branches.
+
+rebase::
+	This rebases the current branch based on a single head.
+	Commits are rewritten as with git-rebase. This doesn't
+	produce a merge. The procedure for dealing with conflicts 
+	is the same as with git-rebase.
diff --git a/Makefile b/Makefile
index 8db4dbe..e6d3812 100644
--- a/Makefile
+++ b/Makefile
@@ -215,7 +215,7 @@ SCRIPT_SH = \
 	git-sh-setup.sh \
 	git-am.sh \
 	git-merge.sh git-merge-stupid.sh git-merge-octopus.sh \
-	git-merge-resolve.sh git-merge-ours.sh \
+	git-merge-resolve.sh git-merge-ours.sh git-merge-rebase.sh \
 	git-lost-found.sh git-quiltimport.sh git-submodule.sh \
 	git-filter-branch.sh \
 	git-stash.sh
diff --git a/git-merge-rebase.sh b/git-merge-rebase.sh
new file mode 100755
index 0000000..b75be3f
--- /dev/null
+++ b/git-merge-rebase.sh
@@ -0,0 +1,17 @@
+#!/bin/sh
+#
+# Copyright (c) 2007 Tom Clarke
+#
+# Resolve two trees with rebase
+
+# The first parameters up to -- are merge bases ignore them
+while test $1 != "--"; do shift; done
+shift
+
+# ignore the first head, it's not needed in a rebase merge
+shift
+
+# Give up if we are given two or more remotes -- not handling octopus.
+test $# = 1 || exit 2
+
+git rebase $1 || exit 2
diff --git a/git-merge.sh b/git-merge.sh
index 6c513dc..b58bee2 100755
--- a/git-merge.sh
+++ b/git-merge.sh
@@ -16,11 +16,12 @@ test -z "$(git ls-files -u)" ||
 LF='
 '
 
-all_strategies='recur recursive octopus resolve stupid ours subtree'
+all_strategies='recur recursive octopus resolve stupid ours subtree rebase'
 default_twohead_strategies='recursive'
 default_octopus_strategies='octopus'
 no_fast_forward_strategies='subtree ours'
-no_trivial_strategies='recursive recur subtree ours'
+no_trivial_strategies='recursive recur subtree ours rebase'
+no_update_ref='rebase'
 use_strategies=
 
 allow_fast_forward=t
@@ -81,11 +82,18 @@ finish () {
 			echo "No merge message -- not updating HEAD"
 			;;
 		*)
-			git update-ref -m "$rlogm" HEAD "$1" "$head" || exit 1
+			case " $wt_strategy " in
+			*" $no_update_ref "*)
+				;;
+			*)
+				git update-ref -m "$rlogm" HEAD "$1" "$head" || exit 1
+				;;
+			esac
 			;;
 		esac
 		;;
 	esac
+
 	case "$1" in
 	'')
 		;;
@@ -418,6 +426,16 @@ do
 	;;
     esac
 
+    # Check to see if there's a message in a merge type that won't produce a commit 
+    if test $have_message = "t"
+    then
+	case " $strategy " in
+	    *" $no_update_ref "*)
+	    echo >&2 "warning: Message is not used for $strategy merge strategy"
+	    ;;
+	esac
+    fi
+
     # Remember which strategy left the state in the working tree
     wt_strategy=$strategy
 
diff --git a/t/t3031-merge-rebase.sh b/t/t3031-merge-rebase.sh
new file mode 100755
index 0000000..daa03b1
--- /dev/null
+++ b/t/t3031-merge-rebase.sh
@@ -0,0 +1,44 @@
+#!/bin/sh
+
+test_description='merge-rebase backend test'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	echo hello >a &&
+	git add a &&
+	test_tick && git commit -m initial &&
+
+	git checkout -b branch &&
+	echo hello >b &&
+	git add b &&
+	test_tick && git commit -m onbranch &&
+
+	git checkout master &&
+	echo update >a &&
+	git add a &&
+	test_tick && git commit -m update
+'
+test_expect_success 'merging using rebase does not create merge commit' '
+	git checkout branch &&
+	git merge -s rebase master &&
+
+	( git log --pretty=oneline ) >actual &&
+	(
+		echo "4db7a5a013e67aa623d1fd294e8d46e89b3ace8f onbranch"
+		echo "893371811dbd13e85c098b72d1ab42bcfd24c2db update"
+		echo "0e960b10429bf3f1e168ee2cc7d531ac7c622580 initial"
+	) >expected &&
+	git diff -w -u expected actual
+'
+git reset --hard HEAD@{2}
+
+test_expect_success 'merging using rebase with message gives warning' '
+	git merge -m "a message" -s rebase master 2> actual &&
+	(
+		echo "warning: Message is not used for rebase merge strategy"
+	) >expected &&
+	git diff -w -u expected actual
+'
+
+test_done
-- 
1.5.3.rc7.3.g850f-dirty

Re: [PATCH] Adding rebase merge strategy

[Johannes Schindelin]

Hi,

On Mon, 1 Oct 2007, Tom Clarke wrote:

> Incorporated comments from Johannes Schindlen.

Thanks.

> +# Give up if we are given two or more remotes -- not handling octopus.
> +test $# = 1 || exit 2

I think the user wants to know in this case, too.  How about

test $# = 1 || {
	echo "Cannot handle octopus." >&2
	exit 2
}

> diff --git a/t/t3031-merge-rebase.sh b/t/t3031-merge-rebase.sh
> new file mode 100755
> index 0000000..daa03b1
> --- /dev/null
> +++ b/t/t3031-merge-rebase.sh
>
> [...]
>
> +	( git log --pretty=oneline ) >actual &&

Please lose the parentheses here.

> +	(
> +		echo "4db7a5a013e67aa623d1fd294e8d46e89b3ace8f onbranch"
> +		echo "893371811dbd13e85c098b72d1ab42bcfd24c2db update"
> +		echo "0e960b10429bf3f1e168ee2cc7d531ac7c622580 initial"
> +	) >expected &&

Why not do it as is done elsewhere in the test suit: use a "cat << EOF" 
before "test_expect_success"?

> +	(
> +		echo "warning: Message is not used for rebase merge strategy"
> +	) >expected &&

Same here.

Other than that, I like it.

Ciao,
Dscho

Re: [PATCH] Adding rebase merge strategy

[Junio C Hamano]

Tom Clarke <tom@u2i.com> writes:

> diff --git a/Documentation/merge-strategies.txt b/Documentation/merge-strategies.txt
> index 7df0266..dff1909 100644
> --- a/Documentation/merge-strategies.txt
> +++ b/Documentation/merge-strategies.txt
> @@ -33,3 +33,9 @@ ours::
>  	merge is always the current branch head.  It is meant to
>  	be used to supersede old development history of side
>  	branches.
> +
> +rebase::
> +	This rebases the current branch based on a single head.
> +	Commits are rewritten as with git-rebase. This doesn't
> +	produce a merge. The procedure for dealing with conflicts 
> +	is the same as with git-rebase.

This would give a handier shortcut iff the rebase goes well, but
the workflow after stopping would be entirely different from the
normal "merge".  I am a bit worried about it giving confusion to
the end users.

> diff --git a/git-merge-rebase.sh b/git-merge-rebase.sh
> new file mode 100755
> index 0000000..b75be3f
> --- /dev/null
> +++ b/git-merge-rebase.sh
> @@ -0,0 +1,17 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2007 Tom Clarke
> +#
> +# Resolve two trees with rebase
> +
> +# The first parameters up to -- are merge bases ignore them
> +while test $1 != "--"; do shift; done
> +shift
> +
> +# ignore the first head, it's not needed in a rebase merge
> +shift
> +
> +# Give up if we are given two or more remotes -- not handling octopus.
> +test $# = 1 || exit 2
> +
> +git rebase $1 || exit 2

This code makes rebase strategy to signal the caller that rebase
it punted by exit status 2 when it spots conflicting changes.

At that point, what is the state of the branch tip, the index
and the work tree?

When a merge strategy exits with 1 ("I cannot handle this fully
but here is a partial attempt"), it is expected to leave
something that can be resolved in the working tree to be
committed (iow, do some edit, update-index and the a single
"commit" will conclude the whole "git merge" business).

When the strategy exits with 2 ("I cannot deal with this at
all"), git-merge will rewind the branch, index and the work tree
by calling restorestate to the pristine state (in order to clear
whatever mess the strategy may have created), say "Merge with
strategy rebase failed.", and exits with 2.

And at that point, what can the user do?

The user then can initiate the rebase process again from the
command line (perhaps even with "-i"):

	$ git rebase FETCH_HEAD

and deal manually with the incremental conflict resolution.
This needs to be documented a bit more clearly, I think, than
your change in the Documentation/merge-strategies.txt.

But more importantly, the above recovery is possible only if you
abort that failed rebase you did in your strategy module, isn't
it?

Although I haven't tried, I suspect that you need to change the
final "git rebase $1 || exit 2" in your script with something
like:

	git rebase "$1" || {
        	git rebase --abort
                exit 2
	}

Have you tested conflicting cases to see how well it works and
what the user experience would look like?

> diff --git a/git-merge.sh b/git-merge.sh
> index 6c513dc..b58bee2 100755
> --- a/git-merge.sh
> +++ b/git-merge.sh
> ...
> @@ -81,11 +82,18 @@ finish () {
>  			echo "No merge message -- not updating HEAD"
>  			;;
>  		*)
> -			git update-ref -m "$rlogm" HEAD "$1" "$head" || exit 1
> +			case " $wt_strategy " in
> +			*" $no_update_ref "*)
> +				;;
> +			*)
> +				git update-ref -m "$rlogm" HEAD "$1" "$head" || exit 1
> +				;;
> +			esac

Is this because a successful rebase strategy already updated the
ref?  I would not object from the end user experience point of
view to allow "git merge -s rebase" command, but I suspect the
changes to the merge frontend could to be restructured a bit to
handle this easier to read.

>  			;;
>  		esac
>  		;;
>  	esac
> +
>  	case "$1" in
>  	'')
>  		;;

Probably this is a good readability improvement.

> @@ -418,6 +426,16 @@ do
>  	;;
>      esac
>  
> +    # Check to see if there's a message in a merge type that won't produce a commit 
> +    if test $have_message = "t"
> +    then

That quoting is backwards, isn't it?

Literal "t" is not empty and you know you do not need to quote;
you instead need to quote $have_message, because that could be
empty and will not even be given as a separate token to "test"
command without quoting when empty.

> +	case " $strategy " in
> +	    *" $no_update_ref "*)

You have an unnecessary extra indentation here.

> +	    echo >&2 "warning: Message is not used for $strategy merge strategy"
> +	    ;;
> +	esac

I had to spend 3 minutes thinking about this; if you had a
comment like "A strategy that updates the ref by itself (iow,
bypassing git-merge) does not give git-merge to record the merge
commit using the given message."  here, I did not have to.

Re: [PATCH] Adding rebase merge strategy

[Tom Clarke]

On 10/1/07, Junio C Hamano <gitster@pobox.com> wrote:
> This would give a handier shortcut iff the rebase goes well, but
> the workflow after stopping would be entirely different from the
> normal "merge".  I am a bit worried about it giving confusion to
> the end users.

Thanks for the ample feedback, you raise a number of interesting
issues. I am wondering now if making rebase a merge strategy is really
a good idea. Rebasing is not merging, a difference that could perhaps
be overlooked in the no-conflict scenario, but as you point out, is
glaringly obvious as soon as you have conflicts.

I'm happy to try to address the issues you raised, but I wonder if we
would do better to look back at my original proposal which was to add
a --rebase option to git-pull. git-pull is the main place there I see
need for using a rebase instead of a merge, as anywhere where you
might use git-merge directly, if what you really want is a rebase, you
can just run git-rebase.

-Tom

Re: [PATCH] Adding rebase merge strategy

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 874 bytes --]

On Mon, 1 Oct 2007 23:41:56 +0200, "Tom Clarke" wrote:
> Thanks for the ample feedback, you raise a number of interesting
> issues. I am wondering now if making rebase a merge strategy is really
> a good idea. Rebasing is not merging, a difference that could perhaps
> be overlooked in the no-conflict scenario, but as you point out, is
> glaringly obvious as soon as you have conflicts.

What I think I've always wanted is something like the following
behavior for "git pull":

  * Fast forward if possible

  * Otherwise, rebase, but only if there are no conflicts at all

  * Otherwise, do the merge as normal, (leave conflict markers in
    place allowing the user to fix them up and then commit).

Would it be straightforward to turn your rebase merge strategy into
something like the above? And if so, would that address the primary
concerns that Junio raised?

-Carl

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Adding rebase merge strategy

[Tom Clarke]

On 10/2/07, Carl Worth <cworth@cworth.org> wrote:
> What I think I've always wanted is something like the following
> behavior for "git pull":
>
>   * Fast forward if possible
>
>   * Otherwise, rebase, but only if there are no conflicts at all
>
>   * Otherwise, do the merge as normal, (leave conflict markers in
>     place allowing the user to fix them up and then commit).
>
> Would it be straightforward to turn your rebase merge strategy into
> something like the above? And if so, would that address the primary
> concerns that Junio raised?

Maybe we need a 'pull' strategy' - merge, rebase or <insert name for
strategy you describe above>.

-Tom

Re: [PATCH] Adding rebase merge strategy

[Junio C Hamano]

"Tom Clarke" <tom@u2i.com> writes:

> I'm happy to try to address the issues you raised, but I wonder if we
> would do better to look back at my original proposal which was to add
> a --rebase option to git-pull. git-pull is the main place there I see
> need for using a rebase instead of a merge, as anywhere where you
> might use git-merge directly, if what you really want is a rebase, you
> can just run git-rebase.

Yeah, we have taught "git-pull == git-fetch + git-merge" to our
users, and "-s strategy" has been a way to specify _how_ the
merge is done, and not about doing something that is not a
merge.

As you say, rebase is not doing a merge.  But neither is "squash".

"git-pull --rebase == git-fetch + git-rebase" might be simpler
for end users to understand.  I dunno.

Re: [PATCH] Adding rebase merge strategy

[Shawn O. Pearce]

Tom Clarke <tom@u2i.com> wrote:
> On 10/2/07, Carl Worth <cworth@cworth.org> wrote:
> > What I think I've always wanted is something like the following
> > behavior for "git pull":
> >
> >   * Fast forward if possible
> >
> >   * Otherwise, rebase, but only if there are no conflicts at all
> >
> >   * Otherwise, do the merge as normal, (leave conflict markers in
> >     place allowing the user to fix them up and then commit).
> >
> > Would it be straightforward to turn your rebase merge strategy into
> > something like the above? And if so, would that address the primary
> > concerns that Junio raised?
> 
> Maybe we need a 'pull' strategy' - merge, rebase or <insert name for
> strategy you describe above>.

`git pull -s <name>` takes any merge strategy that git-merge will
accept for its -s option.  There is also the config option of
pull.twohead that indicates what the default merge/pull strategy
should be for a two head merge.  Most people don't set this,
letting the code default to 'recursive'.

But I have to agree with (was it Junio who said this?) doing a rebase
in a merge strategy doesn't make sense when conflicts come into play.
It gets confusing fast for the end-user as the conflict resolution
process is different than for a merge.  A long time ago I wrote a
git-merge-rebase strategy and gave up on it for basically the same
reason.  I posted it to the mailing list and I think Linus said
"Why?!".  That was the end of that thread as I wound up agreeing
with him.

Multiple merge strategies can be given (and attempted).  A rebase
strategy could be attempted before recursive, and if the rebase
fails but the recursive succeeds then you get (roughly) what is
being described above by Carl.  But that still requires a rebase
merge strategy.  :-\

-- 
Shawn.

Re: [PATCH] Adding rebase merge strategy

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 2754 bytes --]

On Mon, 1 Oct 2007 18:30:50 -0400, "Shawn O. Pearce" wrote:
> `git pull -s <name>` takes any merge strategy that git-merge will
> accept for its -s option.  There is also the config option of
> pull.twohead that indicates what the default merge/pull strategy
> should be for a two head merge.  Most people don't set this,
> letting the code default to 'recursive'.

Ah, "pull.twohead". I don't think I ever would have guessed that. (And
I was just about to ask if there was a nice place to find all these
options, but then found it myself on my first guess with "man
git-config". Thanks everyone for writing that!).

> But I have to agree with (was it Junio who said this?) doing a rebase
> in a merge strategy doesn't make sense when conflicts come into play.

Sure. Rebase alone isn't useful as a complete merge strategy. But a
rebase strategy that simply fails in the face of a conflict,
(deferring to a subsequent merge strategy), could be very useful.

> It gets confusing fast for the end-user as the conflict resolution
> process is different than for a merge.  A long time ago I wrote a
> git-merge-rebase strategy and gave up on it for basically the same
> reason.  I posted it to the mailing list and I think Linus said
> "Why?!".  That was the end of that thread as I wound up agreeing
> with him.

Yes, I thought I recalled seeing a rebase strategy go by in the past,
but I had never gotten around to trying it out. I'll try to do better
on this try.

> Multiple merge strategies can be given (and attempted).  A rebase
> strategy could be attempted before recursive, and if the rebase
> fails but the recursive succeeds then you get (roughly) what is
> being described above by Carl.  But that still requires a rebase
> merge strategy.  :-\

Yes, this sounds exactly like what I want. So, I put "rebase
recursive" in place as the value for the pull.twohead configuration?
An then make sure that the rebase strategy aborts as "failed" instead
of "conflicted and left for user to resolve"? I saw Junio talking
about return values up above in the thread but didn't pay attention to
details, (2 vs. 1 or something)?

Has anyone tried this rebase then recursive strategy yet? I'm
definitely interested in trying it out, as I think I'd
find it quite nice as a default for pull in my usage.

Though actually I'd like it even more if there was some way to mark a
commit as having been "published" and the rebase strategy would refuse
to rebase published commits. Maybe that's a per-branch
"last-published" reference? I think I'd even like git-push to update
the last-published reference for each pushed branch by default, but
then perhaps have an option to mark a particular remote so that
pushing to that remote doesn't count as publishing.

-Carl

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Adding rebase merge strategy

[J. Bruce Fields]

On Mon, Oct 01, 2007 at 03:17:28PM -0700, Carl Worth wrote:
> What I think I've always wanted is something like the following
> behavior for "git pull":
> 
>   * Fast forward if possible
> 
>   * Otherwise, rebase, but only if there are no conflicts at all
> 
>   * Otherwise, do the merge as normal, (leave conflict markers in
>     place allowing the user to fix them up and then commit).
> 
> Would it be straightforward to turn your rebase merge strategy into
> something like the above? And if so, would that address the primary
> concerns that Junio raised?

Surely the job of a merge strategy is to take two heads and produce a
single merge commit?

If it's worth automating the steps you describe above, I think it'd be
better to choose an entirely different name for the command.

--b.

Re: [PATCH] Adding rebase merge strategy

[Junio C Hamano]

Carl Worth <cworth@cworth.org> writes:

> Though actually I'd like it even more if there was some way to mark a
> commit as having been "published" and the rebase strategy would refuse
> to rebase published commits.

That is not "though actually", but an independent topic.

The need is indeed so real that there is a hook git-rebase pays
attention to in order to help you with that.  I use it to
prevent myself from accidentally rebasing the topic branches
that I already merged to 'next'.

Unsurprisingly, it is called pre-rebase hook.

Re: [PATCH] Adding rebase merge strategy

[Johannes Schindelin]

Hi,

On Mon, 1 Oct 2007, Shawn O. Pearce wrote:

> But I have to agree with (was it Junio who said this?) doing a rebase
> in a merge strategy doesn't make sense when conflicts come into play.

In contrast, I think that it makes sense, absolutely.  If you asked for 
"rebase", you _have_ to know what is coming.

It's all about convenience: in many repos, I just to "git pull", because 
there is really only one upstream.

But in one repo, the upstream is svn, and I mistakenly checked in a merge.  
Not wanting to know svn deeply, I have no nice way (as I would have with 
git) to cover up my mistake.  So in this repo, I would have liked to set 
branch.master.mergeOptions to '-s rebase'.

Ciao,
Dscho

Re: [PATCH] Adding rebase merge strategy

[Tom Clarke]

On 10/2/07, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> It's all about convenience: in many repos, I just to "git pull", because
> there is really only one upstream.
>
> But in one repo, the upstream is svn, and I mistakenly checked in a merge.
> Not wanting to know svn deeply, I have no nice way (as I would have with
> git) to cover up my mistake.  So in this repo, I would have liked to set
> branch.master.mergeOptions to '-s rebase'.

That's a good point, in addition to being able to do a git pull that
uses rebase, it would be useful make this configurable so you can
always safely do 'git pull'.

So it's perhaps the question is whether rebasing should be treated as
a kind of merging, or as an alternative to merging when pulling.
Incidentally, are there any other cases other than pulling where using
rebase as an alternative merge strategy is useful?

-Tom

Re: [PATCH] Adding rebase merge strategy

[Junio C Hamano]

"Tom Clarke" <tom@u2i.com> writes:

> So it's perhaps the question is whether rebasing should be treated as
> a kind of merging, or as an alternative to merging when pulling.
> Incidentally, are there any other cases other than pulling where using
> rebase as an alternative merge strategy is useful?

Very well put; I like concise summary of the point in a
discussion thread like this.

And the question in your last sentence is not merely incidental,
but I think is the most crucial one to decide which way we
should proceed.

I do not offhand think of a place other than "git pull" that
would make sense to sometimes be able to rebase when you
normally use merge, so I am inclined to say it would be easier
to teach that "'git pull' is usually a 'git fetch' followed by
'git merge', but in certain workflow it is handier to 'git
fetch' and then 'git rebase', and here are the ways to get the
rebasing behaviour...".

Re: [PATCH] Adding rebase merge strategy

[Tom Clarke]

On 10/2/07, Junio C Hamano <gitster@pobox.com> wrote:
> I do not offhand think of a place other than "git pull" that
> would make sense to sometimes be able to rebase when you
> normally use merge, so I am inclined to say it would be easier
> to teach that "'git pull' is usually a 'git fetch' followed by
> 'git merge', but in certain workflow it is handier to 'git
> fetch' and then 'git rebase', and here are the ways to get the
> rebasing behaviour...".

I agree. I'll revisit teaching pull to be able to use rebase.

Thanks,

-Tom

Re: [PATCH] Adding rebase merge strategy

[Johannes Schindelin]

Hi,

On Wed, 3 Oct 2007, Tom Clarke wrote:

> On 10/2/07, Junio C Hamano <gitster@pobox.com> wrote:
> > I do not offhand think of a place other than "git pull" that
> > would make sense to sometimes be able to rebase when you
> > normally use merge, so I am inclined to say it would be easier
> > to teach that "'git pull' is usually a 'git fetch' followed by
> > 'git merge', but in certain workflow it is handier to 'git
> > fetch' and then 'git rebase', and here are the ways to get the
> > rebasing behaviour...".
> 
> I agree. I'll revisit teaching pull to be able to use rebase.

In that case, may I request a config variable to set this behaviour 
automatically when calling "git pull <nick>"?

Had we stayed with the merge strategy approach, that would have come for 
free with the --no-ff patch series.

Ciao,
Dscho