[PATCH 0/5] rebase: improve the keep-empty

[PATCH 0/5] rebase: improve the keep-empty

[Felipe Contreras]

Hi,

I've been analyzing 'git rebase' and found that the --keep-empty option
triggers a very very different behavior. Here's a bunch of patches that make it
behave like the 'am' does does for the most part.

There's only a few minor changes, after which it might be possible to replace
the whole 'am' mode to use cherr-pick instead.

Felipe Contreras (5):
  rebase: split the cherry-pick stuff
  rebase: fix 'cherry' mode storage
  rebase: fix sequence continuation
  rebase: fix abort of cherry mode
  rebase: fix cherry-pick invocations

 .gitignore            |  1 +
 Makefile              |  1 +
 git-rebase--am.sh     | 65 ++++++++++++++++++++++-----------------------------
 git-rebase--cherry.sh | 55 +++++++++++++++++++++++++++++++++++++++++++
 git-rebase.sh         |  8 +++++++
 5 files changed, 93 insertions(+), 37 deletions(-)
 create mode 100644 git-rebase--cherry.sh

-- 
1.8.3.rc3.312.g47657de

[PATCH 1/5] rebase: split the cherry-pick stuff

[Felipe Contreras]

They do something completely different from 'git am', it belongs in a
different file.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 .gitignore            |  1 +
 Makefile              |  1 +
 git-rebase--am.sh     | 65 ++++++++++++++++++++++-----------------------------
 git-rebase--cherry.sh | 34 +++++++++++++++++++++++++++
 git-rebase.sh         |  4 ++++
 5 files changed, 68 insertions(+), 37 deletions(-)
 create mode 100644 git-rebase--cherry.sh

diff --git a/.gitignore b/.gitignore
index 6669bf0..284fc8f 100644
--- a/.gitignore
+++ b/.gitignore
@@ -113,6 +113,7 @@
 /git-read-tree
 /git-rebase
 /git-rebase--am
+/git-rebase--cherry
 /git-rebase--interactive
 /git-rebase--merge
 /git-receive-pack
diff --git a/Makefile b/Makefile
index 0f931a2..a3cd4bc 100644
--- a/Makefile
+++ b/Makefile
@@ -469,6 +469,7 @@ SCRIPT_SH += git-web--browse.sh
 SCRIPT_LIB += git-mergetool--lib
 SCRIPT_LIB += git-parse-remote
 SCRIPT_LIB += git-rebase--am
+SCRIPT_LIB += git-rebase--cherry
 SCRIPT_LIB += git-rebase--interactive
 SCRIPT_LIB += git-rebase--merge
 SCRIPT_LIB += git-sh-setup
diff --git a/git-rebase--am.sh b/git-rebase--am.sh
index f84854f..ee1b1b9 100644
--- a/git-rebase--am.sh
+++ b/git-rebase--am.sh
@@ -19,52 +19,43 @@ esac
 test -n "$rebase_root" && root_flag=--root
 
 ret=0
-if test -n "$keep_empty"
-then
-	# we have to do this the hard way.  git format-patch completely squashes
-	# empty commits and even if it didn't the format doesn't really lend
-	# itself well to recording empty patches.  fortunately, cherry-pick
-	# makes this easy
-	git cherry-pick --allow-empty "$revisions"
-	ret=$?
-else
-	rm -f "$GIT_DIR/rebased-patches"
-
-	git format-patch -k --stdout --full-index --ignore-if-in-upstream \
-		--src-prefix=a/ --dst-prefix=b/ --no-renames --no-cover-letter \
-		$root_flag "$revisions" >"$GIT_DIR/rebased-patches"
-	ret=$?
 
-	if test 0 != $ret
-	then
-		rm -f "$GIT_DIR/rebased-patches"
-		case "$head_name" in
-		refs/heads/*)
-			git checkout -q "$head_name"
-			;;
-		*)
-			git checkout -q "$orig_head"
-			;;
-		esac
+rm -f "$GIT_DIR/rebased-patches"
 
-		cat >&2 <<-EOF
+git format-patch -k --stdout --full-index --ignore-if-in-upstream \
+	--src-prefix=a/ --dst-prefix=b/ --no-renames --no-cover-letter \
+	$root_flag "$revisions" >"$GIT_DIR/rebased-patches"
+ret=$?
 
-		git encountered an error while preparing the patches to replay
-		these revisions:
+if test 0 != $ret
+then
+	rm -f "$GIT_DIR/rebased-patches"
+	case "$head_name" in
+	refs/heads/*)
+		git checkout -q "$head_name"
+		;;
+	*)
+		git checkout -q "$orig_head"
+		;;
+	esac
 
-		    $revisions
+	cat >&2 <<-EOF
 
-		As a result, git cannot rebase them.
-		EOF
-		exit $?
-	fi
+	git encountered an error while preparing the patches to replay
+	these revisions:
 
-	git am $git_am_opt --rebasing --resolvemsg="$resolvemsg" <"$GIT_DIR/rebased-patches"
-	ret=$?
+	    $revisions
 
-	rm -f "$GIT_DIR/rebased-patches"
+	As a result, git cannot rebase them.
+	EOF
+	exit $?
 fi
 
+git am $git_am_opt --rebasing --resolvemsg="$resolvemsg" <"$GIT_DIR/rebased-patches"
+ret=$?
+
+rm -f "$GIT_DIR/rebased-patches"
+
 if test 0 != $ret
 then
 	test -d "$state_dir" && write_basic_state
diff --git a/git-rebase--cherry.sh b/git-rebase--cherry.sh
new file mode 100644
index 0000000..cbf80f9
--- /dev/null
+++ b/git-rebase--cherry.sh
@@ -0,0 +1,34 @@
+#!/bin/sh
+#
+# Copyright (c) 2010 Junio C Hamano.
+#
+
+case "$action" in
+continue)
+	git am --resolved --resolvemsg="$resolvemsg" &&
+	move_to_original_branch
+	exit
+	;;
+skip)
+	git am --skip --resolvemsg="$resolvemsg" &&
+	move_to_original_branch
+	exit
+	;;
+esac
+
+test -n "$rebase_root" && root_flag=--root
+
+# we have to do this the hard way.  git format-patch completely squashes
+# empty commits and even if it didn't the format doesn't really lend
+# itself well to recording empty patches.  fortunately, cherry-pick
+# makes this easy
+git cherry-pick --allow-empty "$revisions"
+ret=$?
+
+if test 0 != $ret
+then
+	test -d "$state_dir" && write_basic_state
+	exit $ret
+fi
+
+move_to_original_branch
diff --git a/git-rebase.sh b/git-rebase.sh
index 2c692c3..2754985 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -379,6 +379,10 @@ elif test -n "$do_merge"
 then
 	type=merge
 	state_dir="$merge_dir"
+elif test -n "$keep_empty"
+then
+	type=cherry
+	state_dir="$apply_dir"
 else
 	type=am
 	state_dir="$apply_dir"
-- 
1.8.3.rc3.312.g47657de

Re: [PATCH 1/5] rebase: split the cherry-pick stuff

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> They do something completely different from 'git am', it belongs in a
> different file.

I would prefer to see it called --cherry-pick, not --cherry, as they
are different commands (the latter may be useful when deciding which
one to use the former).

>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  .gitignore            |  1 +
>  Makefile              |  1 +
>  git-rebase--am.sh     | 65 ++++++++++++++++++++++-----------------------------
>  git-rebase--cherry.sh | 34 +++++++++++++++++++++++++++
>  git-rebase.sh         |  4 ++++
>  5 files changed, 68 insertions(+), 37 deletions(-)
>  create mode 100644 git-rebase--cherry.sh
>
> diff --git a/.gitignore b/.gitignore
> index 6669bf0..284fc8f 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -113,6 +113,7 @@
>  /git-read-tree
>  /git-rebase
>  /git-rebase--am
> +/git-rebase--cherry
>  /git-rebase--interactive
>  /git-rebase--merge
>  /git-receive-pack
> diff --git a/Makefile b/Makefile
> index 0f931a2..a3cd4bc 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -469,6 +469,7 @@ SCRIPT_SH += git-web--browse.sh
>  SCRIPT_LIB += git-mergetool--lib
>  SCRIPT_LIB += git-parse-remote
>  SCRIPT_LIB += git-rebase--am
> +SCRIPT_LIB += git-rebase--cherry
>  SCRIPT_LIB += git-rebase--interactive
>  SCRIPT_LIB += git-rebase--merge
>  SCRIPT_LIB += git-sh-setup
> diff --git a/git-rebase--am.sh b/git-rebase--am.sh
> index f84854f..ee1b1b9 100644
> --- a/git-rebase--am.sh
> +++ b/git-rebase--am.sh
> @@ -19,52 +19,43 @@ esac
>  test -n "$rebase_root" && root_flag=--root
>  
>  ret=0
> +rm -f "$GIT_DIR/rebased-patches"
>  
> -		cat >&2 <<-EOF
> +git format-patch -k --stdout --full-index --ignore-if-in-upstream \
> +	--src-prefix=a/ --dst-prefix=b/ --no-renames --no-cover-letter \
> +	$root_flag "$revisions" >"$GIT_DIR/rebased-patches"
> +ret=$?
>  
> +if test 0 != $ret
> +then
> +	rm -f "$GIT_DIR/rebased-patches"
> +	case "$head_name" in
> +	refs/heads/*)
> +		git checkout -q "$head_name"
> +		;;
> +	*)
> +		git checkout -q "$orig_head"
> +		;;
> +	esac
>  
> -		    $revisions
> +	cat >&2 <<-EOF
>  
> +	git encountered an error while preparing the patches to replay
> +	these revisions:
>  
> +	    $revisions
>  
> +	As a result, git cannot rebase them.
> +	EOF
> +	exit $?
>  fi
>  
> +git am $git_am_opt --rebasing --resolvemsg="$resolvemsg" <"$GIT_DIR/rebased-patches"
> +ret=$?
> +
> +rm -f "$GIT_DIR/rebased-patches"
> +
>  if test 0 != $ret
>  then
>  	test -d "$state_dir" && write_basic_state

OK, this part seems a straightforward reindentation of "else" side.

> diff --git a/git-rebase--cherry.sh b/git-rebase--cherry.sh
> new file mode 100644
> index 0000000..cbf80f9
> --- /dev/null
> +++ b/git-rebase--cherry.sh
> @@ -0,0 +1,34 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2010 Junio C Hamano.
> +#
> +
> +case "$action" in
> +continue)
> +	git am --resolved --resolvemsg="$resolvemsg" &&
> +	move_to_original_branch
> +	exit
> +	;;
> +skip)
> +	git am --skip --resolvemsg="$resolvemsg" &&
> +	move_to_original_branch
> +	exit
> +	;;
> +esac
> +
> +test -n "$rebase_root" && root_flag=--root

Hmm, do we want to duplicate this part between the two?  Perhaps it
will be unified in a later patch---I'll read on.

> +# we have to do this the hard way.  git format-patch completely squashes
> +# empty commits and even if it didn't the format doesn't really lend
> +# itself well to recording empty patches.  fortunately, cherry-pick
> +# makes this easy
> +git cherry-pick --allow-empty "$revisions"
> +ret=$?
> +
> +if test 0 != $ret
> +then
> +	test -d "$state_dir" && write_basic_state
> +	exit $ret
> +fi
> +
> +move_to_original_branch

OK.  This is the "then" side of the original, with clean-up phase
copied.

> diff --git a/git-rebase.sh b/git-rebase.sh
> index 2c692c3..2754985 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -379,6 +379,10 @@ elif test -n "$do_merge"
>  then
>  	type=merge
>  	state_dir="$merge_dir"
> +elif test -n "$keep_empty"
> +then
> +	type=cherry
> +	state_dir="$apply_dir"
>  else
>  	type=am
>  	state_dir="$apply_dir"

[PATCH 2/5] rebase: fix 'cherry' mode storage

[Felipe Contreras]

We don't use the 'rebase-apply'.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 git-rebase--cherry.sh | 4 ++++
 git-rebase.sh         | 5 ++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/git-rebase--cherry.sh b/git-rebase--cherry.sh
index cbf80f9..ab1f8b7 100644
--- a/git-rebase--cherry.sh
+++ b/git-rebase--cherry.sh
@@ -18,6 +18,9 @@ esac
 
 test -n "$rebase_root" && root_flag=--root
 
+mkdir "$state_dir" || die "Could not create temporary $state_dir"
+: > "$state_dir"/cherry || die "Could not mark as cherry"
+
 # we have to do this the hard way.  git format-patch completely squashes
 # empty commits and even if it didn't the format doesn't really lend
 # itself well to recording empty patches.  fortunately, cherry-pick
@@ -32,3 +35,4 @@ then
 fi
 
 move_to_original_branch
+rm -rf "$state_dir"
diff --git a/git-rebase.sh b/git-rebase.sh
index 2754985..b7759d5 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -174,6 +174,9 @@ then
 	then
 		type=interactive
 		interactive_rebase=explicit
+	elif test -f "$merge_dir"/cherry
+	then
+		type=cherry
 	else
 		type=merge
 	fi
@@ -382,7 +385,7 @@ then
 elif test -n "$keep_empty"
 then
 	type=cherry
-	state_dir="$apply_dir"
+	state_dir="$merge_dir"
 else
 	type=am
 	state_dir="$apply_dir"
-- 
1.8.3.rc3.312.g47657de

Re: [PATCH 2/5] rebase: fix 'cherry' mode storage

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> We don't use the 'rebase-apply'.

s/.$/; we will use rebase-merge instead./ I think.

> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  git-rebase--cherry.sh | 4 ++++
>  git-rebase.sh         | 5 ++++-
>  2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/git-rebase--cherry.sh b/git-rebase--cherry.sh
> index cbf80f9..ab1f8b7 100644
> --- a/git-rebase--cherry.sh
> +++ b/git-rebase--cherry.sh
> @@ -18,6 +18,9 @@ esac
>  
>  test -n "$rebase_root" && root_flag=--root
>  
> +mkdir "$state_dir" || die "Could not create temporary $state_dir"
> +: > "$state_dir"/cherry || die "Could not mark as cherry"

Style:

	>"$state_dir/cherry-pick"

I am not sure if the user _cares_ that internally we use cherry-pick
when s/he asks us to do a keep-empty, and I suspect "mark as cherry"
incomprehensible.  I do not even know what is going on at this point
(yet).

I _suspect_ that you are using cherry-pick when --keep-empty is given,
so it might even make sense to talk in the end-users' terms,
i.e. call this "$state_dir/keep-empty" (I am not sure if calling
this split half "git-rebase--keep-empty.sh" makes sense, yet).

> +
>  # we have to do this the hard way.  git format-patch completely squashes
>  # empty commits and even if it didn't the format doesn't really lend
>  # itself well to recording empty patches.  fortunately, cherry-pick
> @@ -32,3 +35,4 @@ then
>  fi
>  
>  move_to_original_branch
> +rm -rf "$state_dir"
> diff --git a/git-rebase.sh b/git-rebase.sh
> index 2754985..b7759d5 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -174,6 +174,9 @@ then
>  	then
>  		type=interactive
>  		interactive_rebase=explicit
> +	elif test -f "$merge_dir"/cherry
> +	then
> +		type=cherry
>  	else
>  		type=merge
>  	fi
> @@ -382,7 +385,7 @@ then
>  elif test -n "$keep_empty"
>  then
>  	type=cherry
> -	state_dir="$apply_dir"
> +	state_dir="$merge_dir"
>  else
>  	type=am
>  	state_dir="$apply_dir"

Re: [PATCH 2/5] rebase: fix 'cherry' mode storage

[Felipe Contreras]

Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > We don't use the 'rebase-apply'.
> 
> s/.$/; we will use rebase-merge instead./ I think.

We could use 'rebase-apply' or any directory, but currently we don't use any,
and 'rebase-apply' is for 'git am'.

> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> > ---
> >  git-rebase--cherry.sh | 4 ++++
> >  git-rebase.sh         | 5 ++++-
> >  2 files changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/git-rebase--cherry.sh b/git-rebase--cherry.sh
> > index cbf80f9..ab1f8b7 100644
> > --- a/git-rebase--cherry.sh
> > +++ b/git-rebase--cherry.sh
> > @@ -18,6 +18,9 @@ esac
> >  
> >  test -n "$rebase_root" && root_flag=--root
> >  
> > +mkdir "$state_dir" || die "Could not create temporary $state_dir"
> > +: > "$state_dir"/cherry || die "Could not mark as cherry"
> 
> Style:
> 
> 	>"$state_dir/cherry-pick"

Fix that in git-rebase--interactive.sh then, where this code came from.

> I am not sure if the user _cares_ that internally we use cherry-pick
> when s/he asks us to do a keep-empty, and I suspect "mark as cherry"
> incomprehensible.  I do not even know what is going on at this point
> (yet).

It's the same we do in git-rebase--interactive.sh.

> I _suspect_ that you are using cherry-pick when --keep-empty is given,
> so it might even make sense to talk in the end-users' terms,
> i.e. call this "$state_dir/keep-empty" (I am not sure if calling
> this split half "git-rebase--keep-empty.sh" makes sense, yet).

If git-rebase--cherry(-pick)?.sh does the same as 'git-rebase--am.sh', we can
replace it, and make yet another step of replacing scripts (git am) with C code
(git cherry-pick). I thought I explained that somewhow, maybe I didn't.

It still makes sense to fix the broken --keep-empty behavior.

-- 
Felipe Contreras

[PATCH 3/5] rebase: fix sequence continuation

[Felipe Contreras]

We are not in am mode.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 git-rebase--cherry.sh | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/git-rebase--cherry.sh b/git-rebase--cherry.sh
index ab1f8b7..ca78b1b 100644
--- a/git-rebase--cherry.sh
+++ b/git-rebase--cherry.sh
@@ -5,13 +5,15 @@
 
 case "$action" in
 continue)
-	git am --resolved --resolvemsg="$resolvemsg" &&
-	move_to_original_branch
+	git cherry-pick --continue &&
+	move_to_original_branch &&
+	rm -rf "$state_dir"
 	exit
 	;;
 skip)
-	git am --skip --resolvemsg="$resolvemsg" &&
-	move_to_original_branch
+	git cherry-pick --skip &&
+	move_to_original_branch &&
+	rm -rf "$state_dir"
 	exit
 	;;
 esac
-- 
1.8.3.rc3.312.g47657de

Re: [PATCH 3/5] rebase: fix sequence continuation

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> We are not in am mode.

That may make sense, but shouldn't this part be like so from the
very beginning?  In other words, this looks like an "oops, 1/5 was
buggy and this is a hotfix".

>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  git-rebase--cherry.sh | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/git-rebase--cherry.sh b/git-rebase--cherry.sh
> index ab1f8b7..ca78b1b 100644
> --- a/git-rebase--cherry.sh
> +++ b/git-rebase--cherry.sh
> @@ -5,13 +5,15 @@
>  
>  case "$action" in
>  continue)
> -	git am --resolved --resolvemsg="$resolvemsg" &&
> -	move_to_original_branch
> +	git cherry-pick --continue &&
> +	move_to_original_branch &&
> +	rm -rf "$state_dir"
>  	exit
>  	;;
>  skip)
> -	git am --skip --resolvemsg="$resolvemsg" &&
> -	move_to_original_branch
> +	git cherry-pick --skip &&
> +	move_to_original_branch &&
> +	rm -rf "$state_dir"
>  	exit
>  	;;
>  esac

Re: [PATCH 3/5] rebase: fix sequence continuation

[Felipe Contreras]

Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > We are not in am mode.
> 
> That may make sense, but shouldn't this part be like so from the
> very beginning?  In other words, this looks like an "oops, 1/5 was
> buggy and this is a hotfix".

How can 1/5 be introducing a bug? It's merely splitting the code without
introducing *ANY* functional changes.

The bug is already there in 'master'.

-- 
Felipe Contreras

[PATCH 4/5] rebase: fix abort of cherry mode

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 git-rebase.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/git-rebase.sh b/git-rebase.sh
index b7759d5..48bd1b8 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -335,6 +335,7 @@ skip)
 	run_specific_rebase
 	;;
 abort)
+	test "$type" == "cherry" && git cherry-pick --abort
 	git rerere clear
 	read_basic_state
 	case "$head_name" in
-- 
1.8.3.rc3.312.g47657de

[PATCH 5/5] rebase: fix cherry-pick invocations

[Felipe Contreras]

So that all the tests pass.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 git-rebase--cherry.sh | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/git-rebase--cherry.sh b/git-rebase--cherry.sh
index ca78b1b..c3a2ac9 100644
--- a/git-rebase--cherry.sh
+++ b/git-rebase--cherry.sh
@@ -23,11 +23,26 @@ test -n "$rebase_root" && root_flag=--root
 mkdir "$state_dir" || die "Could not create temporary $state_dir"
 : > "$state_dir"/cherry || die "Could not mark as cherry"
 
+if test -n "$rebase_root"
+then
+	revisions="$onto...$orig_head"
+else
+	revisions="$upstream...$orig_head"
+fi
+
 # we have to do this the hard way.  git format-patch completely squashes
 # empty commits and even if it didn't the format doesn't really lend
 # itself well to recording empty patches.  fortunately, cherry-pick
 # makes this easy
-git cherry-pick --allow-empty "$revisions"
+if test -n "$keep_empty"
+then
+	extra="--allow-empty"
+else
+	extra="--skip-empty --cherry-pick"
+fi
+test -n "$GIT_QUIET" && extra="$extra -q"
+test -z "$force_rebase" && extra="$extra --ff"
+git cherry-pick --no-merges --right-only --topo-order --do-walk --copy-notes $extra "$revisions"
 ret=$?
 
 if test 0 != $ret
-- 
1.8.3.rc3.312.g47657de

Re: [PATCH 5/5] rebase: fix cherry-pick invocations

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> So that all the tests pass.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  git-rebase--cherry.sh | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/git-rebase--cherry.sh b/git-rebase--cherry.sh
> index ca78b1b..c3a2ac9 100644
> --- a/git-rebase--cherry.sh
> +++ b/git-rebase--cherry.sh
> @@ -23,11 +23,26 @@ test -n "$rebase_root" && root_flag=--root
>  mkdir "$state_dir" || die "Could not create temporary $state_dir"
>  : > "$state_dir"/cherry || die "Could not mark as cherry"
>  
> +if test -n "$rebase_root"
> +then
> +	revisions="$onto...$orig_head"
> +else
> +	revisions="$upstream...$orig_head"
> +fi

"So that all the tests pass" needs a bit more explanation to say for
cherry-pick codepath why and how two-dot range fails and why and how
three-dot variant with --right-only fixes it.  What are the problematic
cases?

>  # we have to do this the hard way.  git format-patch completely squashes
>  # empty commits and even if it didn't the format doesn't really lend
>  # itself well to recording empty patches.  fortunately, cherry-pick
>  # makes this easy
> -git cherry-pick --allow-empty "$revisions"
> +if test -n "$keep_empty"
> +then
> +	extra="--allow-empty"
> +else
> +	extra="--skip-empty --cherry-pick"
> +fi
> +test -n "$GIT_QUIET" && extra="$extra -q"
> +test -z "$force_rebase" && extra="$extra --ff"
> +git cherry-pick --no-merges --right-only --topo-order --do-walk --copy-notes $extra "$revisions"
>  ret=$?
>  
>  if test 0 != $ret

Re: [PATCH 5/5] rebase: fix cherry-pick invocations

[Junio C Hamano]

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

> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> So that all the tests pass.
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  git-rebase--cherry.sh | 17 ++++++++++++++++-
>>  1 file changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/git-rebase--cherry.sh b/git-rebase--cherry.sh
>> index ca78b1b..c3a2ac9 100644
>> --- a/git-rebase--cherry.sh
>> +++ b/git-rebase--cherry.sh
>> @@ -23,11 +23,26 @@ test -n "$rebase_root" && root_flag=--root
>>  mkdir "$state_dir" || die "Could not create temporary $state_dir"
>>  : > "$state_dir"/cherry || die "Could not mark as cherry"
>>  
>> +if test -n "$rebase_root"
>> +then
>> +	revisions="$onto...$orig_head"
>> +else
>> +	revisions="$upstream...$orig_head"
>> +fi
>
> "So that all the tests pass" needs a bit more explanation to say for
> cherry-pick codepath why and how two-dot range fails and why and how
> three-dot variant with --right-only fixes it.  What are the problematic
> cases?

Yikes, sorry, this was me being slow.  Walking A...B range with
right-only and --cherry applied will filter the duplicates, which is
wat you want, I think, and walking A..B range will not do the
filtering for you.


>>  # we have to do this the hard way.  git format-patch completely squashes
>>  # empty commits and even if it didn't the format doesn't really lend
>>  # itself well to recording empty patches.  fortunately, cherry-pick
>>  # makes this easy
>> -git cherry-pick --allow-empty "$revisions"
>> +if test -n "$keep_empty"
>> +then
>> +	extra="--allow-empty"
>> +else
>> +	extra="--skip-empty --cherry-pick"
>> +fi
>> +test -n "$GIT_QUIET" && extra="$extra -q"
>> +test -z "$force_rebase" && extra="$extra --ff"
>> +git cherry-pick --no-merges --right-only --topo-order --do-walk --copy-notes $extra "$revisions"
>>  ret=$?
>>  
>>  if test 0 != $ret

Re: [PATCH 5/5] rebase: fix cherry-pick invocations

[Felipe Contreras]

Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Felipe Contreras <felipe.contreras@gmail.com> writes:
> >
> >> So that all the tests pass.
> >>
> >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> >> ---
> >>  git-rebase--cherry.sh | 17 ++++++++++++++++-
> >>  1 file changed, 16 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/git-rebase--cherry.sh b/git-rebase--cherry.sh
> >> index ca78b1b..c3a2ac9 100644
> >> --- a/git-rebase--cherry.sh
> >> +++ b/git-rebase--cherry.sh
> >> @@ -23,11 +23,26 @@ test -n "$rebase_root" && root_flag=--root
> >>  mkdir "$state_dir" || die "Could not create temporary $state_dir"
> >>  : > "$state_dir"/cherry || die "Could not mark as cherry"
> >>  
> >> +if test -n "$rebase_root"
> >> +then
> >> +	revisions="$onto...$orig_head"
> >> +else
> >> +	revisions="$upstream...$orig_head"
> >> +fi
> >
> > "So that all the tests pass" needs a bit more explanation to say for
> > cherry-pick codepath why and how two-dot range fails and why and how
> > three-dot variant with --right-only fixes it.  What are the problematic
> > cases?
> 
> Yikes, sorry, this was me being slow.  Walking A...B range with
> right-only and --cherry applied will filter the duplicates, which is
> wat you want, I think, and walking A..B range will not do the
> filtering for you.

That's right.

-- 
Felipe Contreras

Re: [PATCH 5/5] rebase: fix cherry-pick invocations

[Felipe Contreras]

Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > So that all the tests pass.
> >
> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> > ---
> >  git-rebase--cherry.sh | 17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/git-rebase--cherry.sh b/git-rebase--cherry.sh
> > index ca78b1b..c3a2ac9 100644
> > --- a/git-rebase--cherry.sh
> > +++ b/git-rebase--cherry.sh
> > @@ -23,11 +23,26 @@ test -n "$rebase_root" && root_flag=--root
> >  mkdir "$state_dir" || die "Could not create temporary $state_dir"
> >  : > "$state_dir"/cherry || die "Could not mark as cherry"
> >  
> > +if test -n "$rebase_root"
> > +then
> > +	revisions="$onto...$orig_head"
> > +else
> > +	revisions="$upstream...$orig_head"
> > +fi
> 
> "So that all the tests pass" needs a bit more explanation to say for
> cherry-pick codepath why and how two-dot range fails and why and how
> three-dot variant with --right-only fixes it.  What are the problematic
> cases?

There's too many failures to count. We are blindingly applying a series of
commits on top of another without checking if they are merges or if they
already exist in the destination branch.

-- 
Felipe Contreras

Re: [PATCH 0/5] rebase: improve the keep-empty

[Felipe Contreras]

On Tue, May 28, 2013 at 8:29 AM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> Hi,
>
> I've been analyzing 'git rebase' and found that the --keep-empty option
> triggers a very very different behavior. Here's a bunch of patches that make it
> behave like the 'am' does does for the most part.
>
> There's only a few minor changes, after which it might be possible to replace
> the whole 'am' mode to use cherr-pick instead.
>
> Felipe Contreras (5):
>   rebase: split the cherry-pick stuff
>   rebase: fix 'cherry' mode storage
>   rebase: fix sequence continuation
>   rebase: fix abort of cherry mode
>   rebase: fix cherry-pick invocations

To see the issue, simply enable keep_mode by default, and run the tests:

    Test Summary Report
    -------------------
    ./t3401-rebase-partial.sh                          (Wstat: 256
Tests: 7 Failed: 4)
      Failed tests:  3, 5-7
      Non-zero exit status: 1
    ./t3403-rebase-skip.sh                             (Wstat: 256
Tests: 10 Failed: 3)
      Failed tests:  4-5, 9
      Non-zero exit status: 1
    ./t3406-rebase-message.sh                          (Wstat: 256
Tests: 6 Failed: 3)
      Failed tests:  3-5
      Non-zero exit status: 1
    ./t3400-rebase.sh                                  (Wstat: 256
Tests: 27 Failed: 10)
      Failed tests:  13-17, 22-25, 27
      Non-zero exit status: 1
    ./t3407-rebase-abort.sh                            (Wstat: 256
Tests: 11 Failed: 5)
      Failed tests:  2-6
      Non-zero exit status: 1
    ./t3412-rebase-root.sh                             (Wstat: 256
Tests: 31 Failed: 17)
      Failed tests:  4, 6-8, 10-12, 14-17, 23, 25-26, 28-29
                    31
      Non-zero exit status: 1
    ./t3417-rebase-whitespace-fix.sh                   (Wstat: 256
Tests: 4 Failed: 4)
      Failed tests:  1-4
      Non-zero exit status: 1
    ./t3419-rebase-patch-id.sh                         (Wstat: 256
Tests: 9 Failed: 1)
      Failed test:  4
      Non-zero exit status: 1
    ./t3418-rebase-continue.sh                         (Wstat: 256
Tests: 6 Failed: 2)
      Failed tests:  3, 5
      Non-zero exit status: 1
    ./t5407-post-rewrite-hook.sh                       (Wstat: 256
Tests: 13 Failed: 4)
      Failed tests:  4-6, 8
      Non-zero exit status: 1
    ./t5520-pull.sh                                    (Wstat: 256
Tests: 21 Failed: 8)
      Failed tests:  12-15, 18-21
      Non-zero exit status: 1
    ./t7512-status-help.sh                             (Wstat: 256
Tests: 35 Failed: 3)
      Failed tests:  5-6, 28
      Non-zero exit status: 1
    ./t9106-git-svn-commit-diff-clobber.sh             (Wstat: 256
Tests: 10 Failed: 3)
      Failed tests:  6, 9-10
      Non-zero exit status: 1
    ./t3404-rebase-interactive.sh                      (Wstat: 256
Tests: 71 Failed: 2)
      Failed tests:  50, 69
      Non-zero exit status: 1
    ./t9137-git-svn-dcommit-clobber-series.sh          (Wstat: 256
Tests: 5 Failed: 1)
      Failed test:  4
      Non-zero exit status: 1
    ./t9140-git-svn-reset.sh                           (Wstat: 256
Tests: 6 Failed: 1)
      Failed test:  6
      Non-zero exit status: 1
    ./t9903-bash-prompt.sh                             (Wstat: 256
Tests: 51 Failed: 20)
      Failed tests:  27-34, 36-39, 41-42, 45-49, 51
      Non-zero exit status: 1
    ./t9164-git-svn-dcommit-concurrent.sh              (Wstat: 256
Tests: 8 Failed: 4)
      Failed tests:  4-6, 8
      Non-zero exit status: 1
    Files=629, Tests=10036, 308 wallclock secs ( 7.60 usr  1.39 sys +
556.35 cusr 382.46 csys = 947.80 CPU)
    Result: FAIL

-- 
Felipe Contreras

Re: [PATCH 0/5] rebase: improve the keep-empty

[Martin von Zweigbergk]

Hi,

I think I have some patches at home that instead teach 'git am' the
--keep-empty flag. Does that make sense? It's been a while since I
looked at it, but I'll try to take a look tonight (PST).

Martin

On Tue, May 28, 2013 at 6:29 AM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> Hi,
>
> I've been analyzing 'git rebase' and found that the --keep-empty option
> triggers a very very different behavior. Here's a bunch of patches that make it
> behave like the 'am' does does for the most part.
>
> There's only a few minor changes, after which it might be possible to replace
> the whole 'am' mode to use cherr-pick instead.
>
> Felipe Contreras (5):
>   rebase: split the cherry-pick stuff
>   rebase: fix 'cherry' mode storage
>   rebase: fix sequence continuation
>   rebase: fix abort of cherry mode
>   rebase: fix cherry-pick invocations
>
>  .gitignore            |  1 +
>  Makefile              |  1 +
>  git-rebase--am.sh     | 65 ++++++++++++++++++++++-----------------------------
>  git-rebase--cherry.sh | 55 +++++++++++++++++++++++++++++++++++++++++++
>  git-rebase.sh         |  8 +++++++
>  5 files changed, 93 insertions(+), 37 deletions(-)
>  create mode 100644 git-rebase--cherry.sh
>
> --
> 1.8.3.rc3.312.g47657de
>

Re: [PATCH 0/5] rebase: improve the keep-empty

[Felipe Contreras]

Martin von Zweigbergk wrote:
> I think I have some patches at home that instead teach 'git am' the
> --keep-empty flag. Does that make sense? It's been a while since I
> looked at it, but I'll try to take a look tonight (PST).

I think it does make sense. But I still would prefer 'git rebase' to rely on
'git cherry-pick' primariliy.

-- 
Felipe Contreras