[RFC/PATCH v2 0/8] rebase: new cherry-pick mode

[RFC/PATCH v2 0/8] rebase: new cherry-pick mode

[Felipe Contreras]

Hi,

After the fixes I did to cherry-pick, it's now fully usable for 'git rebase'
and can be used to replace 'git am' for most cases.

We already rely on cherry-pick for the 'am' mode, but only when using the
--keep-empty option, and when in such mode the behavior of 'git rebase' changes
completely; more specifically; it's completely broken. Manually enabling
--keep-empty to be the default and running the test-suite shows a huge lot of
failures.

After fixing the --keep-empty option by creating a new cherry-pick mode, this
patch series uses this new mode instead of the 'am' mode, and everything works.

There's only two tests that fail, one because the output of the shell prompt
changes a bit, and the other I have not yet investigated.

This brings us one step closer to replace scripts with C code.

Felipe Contreras (8):
  rebase: split the cherry-pick stuff
  rebase: cherry-pick: fix mode storage
  rebase: cherry-pick: fix sequence continuation
  rebase: cherry-pick: fix abort of cherry mode
  rebase: cherry-pick: fix command invocations
  rebase: cherry-pick: fix status messages
  rebase: cherry-pick: automatically commit stage
  rebase: use 'cherrypick' mode instead of 'am'

 .gitignore                             |  1 +
 Makefile                               |  1 +
 contrib/completion/git-prompt.sh       |  2 ++
 git-rebase--am.sh                      | 12 ++-----
 git-rebase--cherrypick.sh              | 64 ++++++++++++++++++++++++++++++++++
 git-rebase.sh                          | 11 ++++--
 t/t3407-rebase-abort.sh                |  2 +-
 t/t5520-pull.sh                        |  2 +-
 t/t9106-git-svn-commit-diff-clobber.sh |  2 +-
 9 files changed, 82 insertions(+), 15 deletions(-)
 create mode 100644 git-rebase--cherrypick.sh

-- 
1.8.3.rc3.312.g47657de

[RFC/PATCH v2 1/8] 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         | 11 +----------
 git-rebase--cherrypick.sh | 34 ++++++++++++++++++++++++++++++++++
 git-rebase.sh             |  4 ++++
 5 files changed, 41 insertions(+), 10 deletions(-)
 create mode 100644 git-rebase--cherrypick.sh

diff --git a/.gitignore b/.gitignore
index 6669bf0..a171533 100644
--- a/.gitignore
+++ b/.gitignore
@@ -113,6 +113,7 @@
 /git-read-tree
 /git-rebase
 /git-rebase--am
+/git-rebase--cherrypick
 /git-rebase--interactive
 /git-rebase--merge
 /git-receive-pack
diff --git a/Makefile b/Makefile
index 0f931a2..800e42d 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--cherrypick
 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,15 +19,7 @@ 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 \
@@ -63,7 +55,6 @@ else
 ret=$?
 
 rm -f "$GIT_DIR/rebased-patches"
-fi
 
 if test 0 != $ret
 then
diff --git a/git-rebase--cherrypick.sh b/git-rebase--cherrypick.sh
new file mode 100644
index 0000000..cbf80f9
--- /dev/null
+++ b/git-rebase--cherrypick.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..f929ca3 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=cherrypick
+	state_dir="$apply_dir"
 else
 	type=am
 	state_dir="$apply_dir"
-- 
1.8.3.rc3.312.g47657de

[RFC/PATCH v2 2/8] rebase: cherry-pick: fix mode storage

[Felipe Contreras]

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

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

diff --git a/git-rebase--cherrypick.sh b/git-rebase--cherrypick.sh
index cbf80f9..51354af 100644
--- a/git-rebase--cherrypick.sh
+++ b/git-rebase--cherrypick.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"/cherrypick || die "Could not mark as cherrypick"
+
 # 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 f929ca3..76900a0 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"/cherrypick
+	then
+		type=cherrypick
 	else
 		type=merge
 	fi
@@ -382,7 +385,7 @@ then
 elif test -n "$keep_empty"
 then
 	type=cherrypick
-	state_dir="$apply_dir"
+	state_dir="$merge_dir"
 else
 	type=am
 	state_dir="$apply_dir"
-- 
1.8.3.rc3.312.g47657de

[RFC/PATCH v2 3/8] rebase: cherry-pick: fix sequence continuation

[Felipe Contreras]

We are not in am mode.

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

diff --git a/git-rebase--cherrypick.sh b/git-rebase--cherrypick.sh
index 51354af..2fa4993 100644
--- a/git-rebase--cherrypick.sh
+++ b/git-rebase--cherrypick.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

[RFC/PATCH v2 4/8] rebase: cherry-pick: 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 76900a0..9b5d78b 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -335,6 +335,7 @@ skip)
 	run_specific_rebase
 	;;
 abort)
+	test "$type" == "cherrypick" && git cherry-pick --abort
 	git rerere clear
 	read_basic_state
 	case "$head_name" in
-- 
1.8.3.rc3.312.g47657de

[RFC/PATCH v2 5/8] rebase: cherry-pick: fix command invocations

[Felipe Contreras]

So that all the tests pass.

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

diff --git a/git-rebase--cherrypick.sh b/git-rebase--cherrypick.sh
index 2fa4993..ab892e6 100644
--- a/git-rebase--cherrypick.sh
+++ b/git-rebase--cherrypick.sh
@@ -23,11 +23,26 @@ test -n "$rebase_root" && root_flag=--root
 mkdir "$state_dir" || die "Could not create temporary $state_dir"
 : > "$state_dir"/cherrypick || die "Could not mark as cherrypick"
 
+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

[RFC/PATCH v2 6/8] rebase: cherry-pick: fix status messages

[Felipe Contreras]

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

diff --git a/git-rebase--cherrypick.sh b/git-rebase--cherrypick.sh
index ab892e6..ef3224d 100644
--- a/git-rebase--cherrypick.sh
+++ b/git-rebase--cherrypick.sh
@@ -3,6 +3,9 @@
 # Copyright (c) 2010 Junio C Hamano.
 #
 
+GIT_CHERRY_PICK_HELP="$resolvemsg"
+export GIT_CHERRY_PICK_HELP
+
 case "$action" in
 continue)
 	git cherry-pick --continue &&
-- 
1.8.3.rc3.312.g47657de

[RFC/PATCH v2 7/8] rebase: cherry-pick: automatically commit stage

[Felipe Contreras]

When there's changes in the staging area. Just like the other rebase
modes.

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

diff --git a/git-rebase--cherrypick.sh b/git-rebase--cherrypick.sh
index ef3224d..0fcf2e1 100644
--- a/git-rebase--cherrypick.sh
+++ b/git-rebase--cherrypick.sh
@@ -8,6 +8,12 @@ export GIT_CHERRY_PICK_HELP
 
 case "$action" in
 continue)
+	# do we have anything to commit?
+	if ! git diff-index --cached --quiet HEAD --
+	then
+		git commit --no-verify -e ||
+			die "Could not commit staged changes."
+	fi
 	git cherry-pick --continue &&
 	move_to_original_branch &&
 	rm -rf "$state_dir"
-- 
1.8.3.rc3.312.g47657de

[RFC/PATCH v2 8/8] rebase: use 'cherrypick' mode instead of 'am'

[Felipe Contreras]

Unless any specific 'git am' options are used.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/completion/git-prompt.sh       | 2 ++
 git-rebase--am.sh                      | 1 +
 git-rebase.sh                          | 9 ++++-----
 t/t3407-rebase-abort.sh                | 2 +-
 t/t5520-pull.sh                        | 2 +-
 t/t9106-git-svn-commit-diff-clobber.sh | 2 +-
 6 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index eaf5c36..f001463 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -271,6 +271,8 @@ __git_ps1 ()
 			total=$(cat "$g/rebase-merge/end")
 			if [ -f "$g/rebase-merge/interactive" ]; then
 				r="|REBASE-i"
+			elif [ -f "$g/rebase-merge/cherrypick" ]; then
+				r="|REBASE"
 			else
 				r="|REBASE-m"
 			fi
diff --git a/git-rebase--am.sh b/git-rebase--am.sh
index ee1b1b9..7a978fd 100644
--- a/git-rebase--am.sh
+++ b/git-rebase--am.sh
@@ -51,6 +51,7 @@ then
 	exit $?
 fi
 
+test -n "$GIT_QUIET" && git_am_opt="$git_am_opt -q"
 git am $git_am_opt --rebasing --resolvemsg="$resolvemsg" <"$GIT_DIR/rebased-patches"
 ret=$?
 
diff --git a/git-rebase.sh b/git-rebase.sh
index 9b5d78b..16cc91b 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -251,7 +251,6 @@ do
 		;;
 	-q)
 		GIT_QUIET=t
-		git_am_opt="$git_am_opt -q"
 		verbose=
 		diffstat=
 		;;
@@ -383,13 +382,13 @@ elif test -n "$do_merge"
 then
 	type=merge
 	state_dir="$merge_dir"
-elif test -n "$keep_empty"
+elif test -n "$git_am_opt"
 then
-	type=cherrypick
-	state_dir="$merge_dir"
-else
 	type=am
 	state_dir="$apply_dir"
+else
+	type=cherrypick
+	state_dir="$merge_dir"
 fi
 
 if test -z "$rebase_root"
diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index a6a6c40..2699b08 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -96,7 +96,7 @@ testrebase() {
 	'
 }
 
-testrebase "" .git/rebase-apply
+testrebase "" .git/rebase-merge
 testrebase " --merge" .git/rebase-merge
 
 test_done
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 6af6c63..ec2373b 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -244,7 +244,7 @@ test_expect_success 'setup for avoiding reapplying old patches' '
 test_expect_success 'git pull --rebase does not reapply old patches' '
 	(cd dst &&
 	 test_must_fail git pull --rebase &&
-	 test 1 = $(find .git/rebase-apply -name "000*" | wc -l)
+	 test 1 = $(cat .git/sequencer/todo | wc -l)
 	)
 '
 
diff --git a/t/t9106-git-svn-commit-diff-clobber.sh b/t/t9106-git-svn-commit-diff-clobber.sh
index f6d7ac7..b9cec33 100755
--- a/t/t9106-git-svn-commit-diff-clobber.sh
+++ b/t/t9106-git-svn-commit-diff-clobber.sh
@@ -92,7 +92,7 @@ test_expect_success 'multiple dcommit from git svn will not clobber svn' "
 
 
 test_expect_success 'check that rebase really failed' '
-	test -d .git/rebase-apply
+	test -d .git/rebase-merge
 '
 
 test_expect_success 'resolve, continue the rebase and dcommit' "
-- 
1.8.3.rc3.312.g47657de

Re: [RFC/PATCH v2 3/8] rebase: cherry-pick: fix sequence continuation

[Martin von Zweigbergk]

 As Junio asked in the previous iteration, shouldn't this have been in
the first patch?

On Tue, May 28, 2013 at 9:16 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> We are not in am mode.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  git-rebase--cherrypick.sh | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/git-rebase--cherrypick.sh b/git-rebase--cherrypick.sh
> index 51354af..2fa4993 100644
> --- a/git-rebase--cherrypick.sh
> +++ b/git-rebase--cherrypick.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: [RFC/PATCH v2 4/8] rebase: cherry-pick: fix abort of cherry mode

[Martin von Zweigbergk]

Same here: should this have been in the first patch? If not, do you
know for how long it has been broken (since which commit)?

On Tue, May 28, 2013 at 9:16 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> 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 76900a0..9b5d78b 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -335,6 +335,7 @@ skip)
>         run_specific_rebase
>         ;;
>  abort)
> +       test "$type" == "cherrypick" && git cherry-pick --abort
>         git rerere clear
>         read_basic_state
>         case "$head_name" in
> --
> 1.8.3.rc3.312.g47657de
>

Re: [RFC/PATCH v2 2/8] rebase: cherry-pick: fix mode storage

[Martin von Zweigbergk]

Actually, are all of 2/8 - 7/8 fixes for things that broke in patch 1/8?

On Tue, May 28, 2013 at 9:16 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> We don't use the 'rebase-apply'.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  git-rebase--cherrypick.sh | 4 ++++
>  git-rebase.sh             | 5 ++++-
>  2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/git-rebase--cherrypick.sh b/git-rebase--cherrypick.sh
> index cbf80f9..51354af 100644
> --- a/git-rebase--cherrypick.sh
> +++ b/git-rebase--cherrypick.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"/cherrypick || die "Could not mark as cherrypick"
> +
>  # 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 f929ca3..76900a0 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"/cherrypick
> +       then
> +               type=cherrypick
>         else
>                 type=merge
>         fi
> @@ -382,7 +385,7 @@ then
>  elif test -n "$keep_empty"
>  then
>         type=cherrypick
> -       state_dir="$apply_dir"
> +       state_dir="$merge_dir"
>  else
>         type=am
>         state_dir="$apply_dir"
> --
> 1.8.3.rc3.312.g47657de
>

Re: [RFC/PATCH v2 3/8] rebase: cherry-pick: fix sequence continuation

[Felipe Contreras]

On Wed, May 29, 2013 at 12:33 AM, Martin von Zweigbergk
<martinvonz@gmail.com> wrote:
>  As Junio asked in the previous iteration, shouldn't this have been in
> the first patch?

No, the first patch is splitting the code without introducing any
functional changes.

This is fixing a bug that already exists, we could fix it before the
split, or after, but mixing the split and the fix at the same time is
a no-no.

One change splits, the other change fixes, what's wrong with that?

This way it's easy to see what each patch does.

-- 
Felipe Contreras

Re: [RFC/PATCH v2 2/8] rebase: cherry-pick: fix mode storage

[Felipe Contreras]

On Wed, May 29, 2013 at 12:38 AM, Martin von Zweigbergk
<martinvonz@gmail.com> wrote:
> Actually, are all of 2/8 - 7/8 fixes for things that broke in patch 1/8?

No, everything is already broken.

Try this:

--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -78,7 +78,7 @@ state_dir=
 action=
 preserve_merges=
 autosquash=
-keep_empty=
+keep_empty=yes
 test "$(git config --bool rebase.autosquash)" = "true" && autosquash=t

 read_basic_state () {

And tell me what happens when you run the test suite:

Hint: This[1] is what would happen; everything breaks, not only the
tests that check for empty commits.

[1] http://article.gmane.org/gmane.comp.version-control.git/225652

-- 
Felipe Contreras

Re: [RFC/PATCH v2 4/8] rebase: cherry-pick: fix abort of cherry mode

[Felipe Contreras]

On Wed, May 29, 2013 at 12:35 AM, Martin von Zweigbergk
<martinvonz@gmail.com> wrote:
> Same here: should this have been in the first patch? If not, do you
> know for how long it has been broken (since which commit)?

Since day 1 of --keep-empty:

90e1818 git-rebase: add keep_empty flag

-- 
Felipe Contreras

Re: [RFC/PATCH v2 3/8] rebase: cherry-pick: fix sequence continuation

[Martin von Zweigbergk]

On Tue, May 28, 2013 at 10:41 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Wed, May 29, 2013 at 12:33 AM, Martin von Zweigbergk
> <martinvonz@gmail.com> wrote:
>>  As Junio asked in the previous iteration, shouldn't this have been in
>> the first patch?
>
> No, the first patch is splitting the code without introducing any
> functional changes.
>
> This is fixing a bug that already exists, we could fix it before the
> split, or after, but mixing the split and the fix at the same time is
> a no-no.

Oh, now I remember. I ran into that bug once.

> One change splits, the other change fixes, what's wrong with that?

I didn't say there was anything wrong. I was asking if the bug was
there before (and I didn't see an answer when Junio asked).

Thanks

Re: [RFC/PATCH v2 3/8] rebase: cherry-pick: fix sequence continuation

[Felipe Contreras]

On Wed, May 29, 2013 at 12:51 AM, Martin von Zweigbergk
<martinvonz@gmail.com> wrote:
> On Tue, May 28, 2013 at 10:41 PM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:

>> One change splits, the other change fixes, what's wrong with that?
>
> I didn't say there was anything wrong. I was asking if the bug was
> there before (and I didn't see an answer when Junio asked).

Why wouldn't it be before? Did I mention a commit that introduced a
problem? No. Did any patch in this series introduce a problem? No.

All we've done in this series is 1) reorganize the code without
introducing *ANY* functional changes, and 2) fix a bug.

If you see 1) introducing a problem, or 2) introducing a problem, then
mention that in *those* patches. If there is no problem with 1) or 2)
then it follows the problem already exists.

-- 
Felipe Contreras

Re: [RFC/PATCH v2 3/8] rebase: cherry-pick: fix sequence continuation

[Martin von Zweigbergk]

:-)

On Tue, May 28, 2013 at 11:05 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Wed, May 29, 2013 at 12:51 AM, Martin von Zweigbergk
> <martinvonz@gmail.com> wrote:
>> On Tue, May 28, 2013 at 10:41 PM, Felipe Contreras
>> <felipe.contreras@gmail.com> wrote:
>
>>> One change splits, the other change fixes, what's wrong with that?
>>
>> I didn't say there was anything wrong. I was asking if the bug was
>> there before (and I didn't see an answer when Junio asked).
>
> Why wouldn't it be before? Did I mention a commit that introduced a
> problem? No. Did any patch in this series introduce a problem? No.
>
> All we've done in this series is 1) reorganize the code without
> introducing *ANY* functional changes, and 2) fix a bug.
>
> If you see 1) introducing a problem, or 2) introducing a problem, then
> mention that in *those* patches. If there is no problem with 1) or 2)
> then it follows the problem already exists.
>
> --
> Felipe Contreras

Re: [RFC/PATCH v2 4/8] rebase: cherry-pick: fix abort of cherry mode

[Stefano Lattarini]

On 05/29/2013 06:16 AM, Felipe Contreras wrote:
> 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 76900a0..9b5d78b 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -335,6 +335,7 @@ skip)
>  	run_specific_rebase
>  	;;
>  abort)
> +	test "$type" == "cherrypick" && git cherry-pick --abort
>
Bashism; please use "=", not "==".

>  	git rerere clear
>  	read_basic_state
>  	case "$head_name" in

Regards,
  Stefano

Re: [RFC/PATCH v2 0/8] rebase: new cherry-pick mode

[Junio C Hamano]

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

> We already rely on cherry-pick for the 'am' mode, but only when using the
> --keep-empty option, and when in such mode the behavior of 'git rebase' changes
> completely; more specifically; it's completely broken. Manually enabling
> --keep-empty to be the default and running the test-suite shows a huge lot of
> failures.
>
> After fixing the --keep-empty option by creating a new cherry-pick mode, this
> patch series uses this new mode instead of the 'am' mode, and everything works.

This may be a stupid question, but does --keep-empty only fail with
the "am" mode?

More specifically, how well does "rebase -i --keep-empty" work?

If the answer is "very well", then it might make sense not to
introduce yet another cherry-pick mode, but do exactly the same
thing as what -p mode does, namely, to internally delegate the
processing to "rebase -i" codepath.  After all, multi-pick mode of
cherry-pick uses the same sequencer machinery as rebase -i uses,
so if we are already producing a correct "rebase todo" sequencer
insn list for "rebase -i" anyway, it should be the matter of not
launching the editor to edit the initial insn sheet to make it
non-interactive, isn't it?

>
> There's only two tests that fail, one because the output of the shell prompt
> changes a bit, and the other I have not yet investigated.
>
> This brings us one step closer to replace scripts with C code.
>
> Felipe Contreras (8):
>   rebase: split the cherry-pick stuff
>   rebase: cherry-pick: fix mode storage
>   rebase: cherry-pick: fix sequence continuation
>   rebase: cherry-pick: fix abort of cherry mode
>   rebase: cherry-pick: fix command invocations
>   rebase: cherry-pick: fix status messages
>   rebase: cherry-pick: automatically commit stage
>   rebase: use 'cherrypick' mode instead of 'am'
>
>  .gitignore                             |  1 +
>  Makefile                               |  1 +
>  contrib/completion/git-prompt.sh       |  2 ++
>  git-rebase--am.sh                      | 12 ++-----
>  git-rebase--cherrypick.sh              | 64 ++++++++++++++++++++++++++++++++++
>  git-rebase.sh                          | 11 ++++--
>  t/t3407-rebase-abort.sh                |  2 +-
>  t/t5520-pull.sh                        |  2 +-
>  t/t9106-git-svn-commit-diff-clobber.sh |  2 +-
>  9 files changed, 82 insertions(+), 15 deletions(-)
>  create mode 100644 git-rebase--cherrypick.sh

Re: [RFC/PATCH v2 0/8] rebase: new cherry-pick mode

[Felipe Contreras]

On Wed, May 29, 2013 at 6:23 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> We already rely on cherry-pick for the 'am' mode, but only when using the
>> --keep-empty option, and when in such mode the behavior of 'git rebase' changes
>> completely; more specifically; it's completely broken. Manually enabling
>> --keep-empty to be the default and running the test-suite shows a huge lot of
>> failures.
>>
>> After fixing the --keep-empty option by creating a new cherry-pick mode, this
>> patch series uses this new mode instead of the 'am' mode, and everything works.
>
> This may be a stupid question, but does --keep-empty only fail with
> the "am" mode?
>
> More specifically, how well does "rebase -i --keep-empty" work?
>
> If the answer is "very well", then it might make sense not to
> introduce yet another cherry-pick mode, but do exactly the same
> thing as what -p mode does, namely, to internally delegate the
> processing to "rebase -i" codepath.  After all, multi-pick mode of
> cherry-pick uses the same sequencer machinery as rebase -i uses,

No, that's not true. 'rebase -i' implements sequencing completely on
it's own, and cherry picks commits one by one, it never uses the
sequencer.c code.

Also, there's the issue of the 'git am' options, that of course only
the am mode respects. I personally wouldn't mind getting rid of them
for consistency purposes, but I know you would disagree.

But the real issue is that we would be detracting from the goal even
more; to replace script code with C code.

> so if we are already producing a correct "rebase todo" sequencer
> insn list for "rebase -i" anyway, it should be the matter of not
> launching the editor to edit the initial insn sheet to make it
> non-interactive, isn't it?

Sure, and make 'git rebase' extremely much more complicated (and
probably inefficient) in the process.

I don't mind going that way, but I want to rewrite
'git-rebase--interactive.sh' to use 'git cherry-pick' more, and maybe
'git-rebase--merge.sh' too, and the first step was to replace the
simplest mode.

-- 
Felipe Contreras