threads / rfc / 33967

RFC v2, 8 partsrebase: new cherry-pick mode

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

## tl;dr

21 messages between May 29, 2013 and May 30, 2013. Diffs are folded; open one to read it.

replies: 20people: 4as markdown or json

Felipe Contreras· May 29, 2013, 04:16 UTC · lore
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
Felipe Contreras· May 29, 2013, 04:16 UTC · re: Felipe Contreras · lore

[RFC/PATCH v2 1/8] rebase: split the cherry-pick stuff

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
Show changes to 5 files +41 −10

.gitignore, Makefile, git-rebase--am.sh, git-rebase--cherrypick.sh, git-rebase.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
Felipe Contreras· May 29, 2013, 04:16 UTC · re: Felipe Contreras · lore

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

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(-)
Show changes to 2 files +8 −1

git-rebase--cherrypick.sh, git-rebase.sh

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
Martin von Zweigbergk· May 29, 2013, 05:38 UTC · re: Felipe Contreras · lore

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

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:

Show 53 quoted lines
> 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
>
Felipe Contreras· May 29, 2013, 05:45 UTC · re: Martin von Zweigbergk · lore

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

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:
Show changes to git-rebase.sh +1 −1
--- 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
Felipe Contreras· May 29, 2013, 04:16 UTC · re: Felipe Contreras · lore

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

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(-)
Show changes to git-rebase--cherrypick.sh +6 −4
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
Martin von Zweigbergk· May 29, 2013, 05:33 UTC · re: Felipe Contreras · lore

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

 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:

Show 34 quoted lines
> 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
>
Felipe Contreras· May 29, 2013, 05:41 UTC · re: Martin von Zweigbergk · lore

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

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
Martin von Zweigbergk· May 29, 2013, 05:51 UTC · re: Felipe Contreras · lore

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

On Tue, May 28, 2013 at 10:41 PM, Felipe Contreras <felipe.contreras@gmail.com> wrote:

Show 11 quoted lines
> 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
Felipe Contreras· May 29, 2013, 06:05 UTC · re: Martin von Zweigbergk · lore

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

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
Martin von Zweigbergk· May 29, 2013, 06:06 UTC · re: Felipe Contreras · lore

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

:-)

On Tue, May 28, 2013 at 11:05 PM, Felipe Contreras <felipe.contreras@gmail.com> wrote:

Show 22 quoted lines
> 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
Felipe Contreras· May 29, 2013, 04:16 UTC · re: Felipe Contreras · lore

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

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 git-rebase.sh | 1 +
 1 file changed, 1 insertion(+)
Show changes to git-rebase.sh +1 −0
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
Martin von Zweigbergk· May 29, 2013, 05:35 UTC · re: Felipe Contreras · lore

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

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:

Show 20 quoted lines
> 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
>
Felipe Contreras· May 29, 2013, 05:47 UTC · re: Martin von Zweigbergk · lore

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

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
Stefano Lattarini· May 29, 2013, 11:13 UTC · re: Felipe Contreras · lore

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

On 05/29/2013 06:16 AM, Felipe Contreras wrote:
Show 15 quoted lines
> 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
Felipe Contreras· May 29, 2013, 04:16 UTC · re: Felipe Contreras · lore

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

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(-)
Show changes to git-rebase--cherrypick.sh +16 −1
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
Felipe Contreras· May 29, 2013, 04:16 UTC · re: Felipe Contreras · lore

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

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 git-rebase--cherrypick.sh | 3 +++
 1 file changed, 3 insertions(+)
Show changes to git-rebase--cherrypick.sh +3 −0
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
Felipe Contreras· May 29, 2013, 04:16 UTC · re: Felipe Contreras · lore

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

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(+)
Show changes to git-rebase--cherrypick.sh +6 −0
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
Felipe Contreras· May 29, 2013, 04:16 UTC · re: Felipe Contreras · lore

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

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(-)
Show changes to 6 files +10 −8

contrib/completion/git-prompt.sh, git-rebase--am.sh, git-rebase.sh, t/t3407-rebase-abort.sh, t/t5520-pull.sh, t/t9106-git-svn-commit-diff-clobber.sh

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
Junio C Hamano· May 29, 2013, 23:23 UTC · re: Felipe Contreras · lore

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

Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 8 quoted lines
> 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?

Show 27 quoted lines
>
> 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
Felipe Contreras· May 30, 2013, 02:37 UTC · re: Junio C Hamano · lore

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

On Wed, May 29, 2013 at 6:23 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 21 quoted lines
> 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

← back to recent threads