[PATCH] rebase -m: skip cherry-picked commits more reliably

[PATCH] rebase -m: skip cherry-picked commits more reliably

[Knut Franke]

Unlike ordinary and interactive rebase, rebase -m halts with a conflict
if two conflicting patches have been applied to both source and target
branch (e.g. by cherry-picking). Fix this by using git rev-list
--cherry-pick to generate the list of patches to apply.

Also adapt t3406 a) to catch this case and b) not to expect
"Already applied" messages which can't be emitted easily if duplicates
are removed already when storing the patches.

Signed-off-by: Knut Franke <Knut.Franke@gmx.de>
---
 git-rebase.sh             |   29 +++++++++++++++++++----------
 t/t3406-rebase-message.sh |   14 +++++++-------
 2 files changed, 26 insertions(+), 17 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index 10a238a..aa42744 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -552,15 +552,14 @@ then
 	exit 0
 fi
 
-if test -n "$rebase_root"
-then
-	revisions="$onto..$orig_head"
-else
-	revisions="$upstream..$orig_head"
-fi
-
 if test -z "$do_merge"
 then
+	if test -n "$rebase_root"
+	then
+		revisions="$onto..$orig_head"
+	else
+		revisions="$upstream..$orig_head"
+	fi
 	git format-patch -k --stdout --full-index --ignore-if-in-upstream \
 		--src-prefix=a/ --dst-prefix=b/ \
 		--no-renames $root_flag "$revisions" |
@@ -587,11 +586,21 @@ echo "$orig_head" > "$dotest/orig-head"
 echo "$head_name" > "$dotest/head-name"
 echo "$GIT_QUIET" > "$dotest/quiet"
 
+if test -n "$rebase_root"
+then
+	revisions="$onto...$orig_head"
+else
+	revisions="$upstream...$orig_head"
+fi
+
 msgnum=0
-for cmt in `git rev-list --reverse --no-merges "$revisions"`
+for cmt in `git rev-list --reverse --no-merges --cherry-pick "$revisions"`
 do
-	msgnum=$(($msgnum + 1))
-	echo "$cmt" > "$dotest/cmt.$msgnum"
+	if test $(git merge-base "$cmt" "$orig_head") = "$cmt"
+	then
+		msgnum=$(($msgnum + 1))
+		echo "$cmt" > "$dotest/cmt.$msgnum"
+	fi
 done
 
 echo 1 >"$dotest/msgnum"
diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index 85fc7c4..41cb039 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -5,8 +5,10 @@ test_description='messages from rebase operation'
 . ./test-lib.sh
 
 quick_one () {
-	echo "$1" >"file$1" &&
-	git add "file$1" &&
+	fileno=$2
+	test -z "$fileno" && fileno=$1
+	echo "$1" >"file$fileno" &&
+	git add "file$fileno" &&
 	test_tick &&
 	git commit -m "$1"
 }
@@ -16,21 +18,19 @@ test_expect_success setup '
 	git branch topic &&
 	quick_one X &&
 	quick_one A &&
-	quick_one B &&
+	quick_one B A &&
 	quick_one Y &&
 
 	git checkout topic &&
 	quick_one A &&
-	quick_one B &&
+	quick_one B A &&
 	quick_one Z &&
 	git tag start
 
 '
 
 cat >expect <<\EOF
-Already applied: 0001 A
-Already applied: 0002 B
-Committed: 0003 Z
+Committed: 0001 Z
 EOF
 
 test_expect_success 'rebase -m' '
-- 
1.7.3.2

Re: [PATCH] rebase -m: skip cherry-picked commits more reliably

[Martin von Zweigbergk]

On Tue, 23 Nov 2010, Knut Franke wrote:

> Unlike ordinary and interactive rebase, rebase -m halts with a conflict
> if two conflicting patches have been applied to both source and target
> branch (e.g. by cherry-picking). Fix this by using git rev-list
> --cherry-pick to generate the list of patches to apply.

I think the idea is sound, but...

Until I saw your patch I had not thought about the impact of the
--cherry-pick option (thanks!). I think there is a bug in the existing
rebase code related to it when rebase is used with the --onto option.

I will try to explain how I think it currently works.

Let's say we have the below history, where E is a cherry-pick of e.

      .-c
     /
a---b---d---e---f
         \
          .-g---E


If we now run 'git rebase --onto c f E', the revisions that will be
applied onto 'c' are given by 'git format-patch
--ignore-if-in-upstream f..E'. In this case that would be only 'g' and
NOT 'E'.

What I think we really want to do is to remove any patches in
upstream..branch that also exist in branch..onto. I don't know how to
do that efficiently, though. (And am I even right about it?)

I think this may be quite a serious bug, since it might go unnoticed
that a patch was dropped. If the <upstream> ref is later removed, the
patch might be completely gone from history without the user noticing.


If the above is correct, is it better to fix that problem first before
applying your patch? On the other hand, your patch does not really
make things worse (except for duplicating the buggy code)...

With the current implementation, my patch in
http://thread.gmane.org/gmane.comp.version-control.git/161381/ would
also make things worse when there are cherry-picks between <upstream>
and <branch>. The above should be fixed before I re-submit.


> 
> Also adapt t3406 a) to catch this case and b) not to expect
> "Already applied" messages which can't be emitted easily if duplicates
> are removed already when storing the patches.
> 
> Signed-off-by: Knut Franke <Knut.Franke@gmx.de>
> ---
>  git-rebase.sh             |   29 +++++++++++++++++++----------
>  t/t3406-rebase-message.sh |   14 +++++++-------
>  2 files changed, 26 insertions(+), 17 deletions(-)
> 
> diff --git a/git-rebase.sh b/git-rebase.sh
> index 10a238a..aa42744 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -552,15 +552,14 @@ then
>  	exit 0
>  fi
>  
> -if test -n "$rebase_root"
> -then
> -	revisions="$onto..$orig_head"
> -else
> -	revisions="$upstream..$orig_head"
> -fi
> -
>  if test -z "$do_merge"
>  then
> +	if test -n "$rebase_root"
> +	then
> +		revisions="$onto..$orig_head"
> +	else
> +		revisions="$upstream..$orig_head"
> +	fi
>  	git format-patch -k --stdout --full-index --ignore-if-in-upstream \
>  		--src-prefix=a/ --dst-prefix=b/ \
>  		--no-renames $root_flag "$revisions" |
> @@ -587,11 +586,21 @@ echo "$orig_head" > "$dotest/orig-head"
>  echo "$head_name" > "$dotest/head-name"
>  echo "$GIT_QUIET" > "$dotest/quiet"
>  
> +if test -n "$rebase_root"
> +then
> +	revisions="$onto...$orig_head"
> +else
> +	revisions="$upstream...$orig_head"
> +fi
> +
>  msgnum=0
> -for cmt in `git rev-list --reverse --no-merges "$revisions"`
> +for cmt in `git rev-list --reverse --no-merges --cherry-pick "$revisions"`
>  do
> -	msgnum=$(($msgnum + 1))
> -	echo "$cmt" > "$dotest/cmt.$msgnum"
> +	if test $(git merge-base "$cmt" "$orig_head") = "$cmt"
> +	then
> +		msgnum=$(($msgnum + 1))
> +		echo "$cmt" > "$dotest/cmt.$msgnum"
> +	fi
>  done
>  
>  echo 1 >"$dotest/msgnum"
> diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
> index 85fc7c4..41cb039 100755
> --- a/t/t3406-rebase-message.sh
> +++ b/t/t3406-rebase-message.sh
> @@ -5,8 +5,10 @@ test_description='messages from rebase operation'
>  . ./test-lib.sh
>  
>  quick_one () {
> -	echo "$1" >"file$1" &&
> -	git add "file$1" &&
> +	fileno=$2
> +	test -z "$fileno" && fileno=$1
> +	echo "$1" >"file$fileno" &&
> +	git add "file$fileno" &&
>  	test_tick &&
>  	git commit -m "$1"
>  }
> @@ -16,21 +18,19 @@ test_expect_success setup '
>  	git branch topic &&
>  	quick_one X &&
>  	quick_one A &&
> -	quick_one B &&
> +	quick_one B A &&
>  	quick_one Y &&
>  
>  	git checkout topic &&
>  	quick_one A &&
> -	quick_one B &&
> +	quick_one B A &&
>  	quick_one Z &&
>  	git tag start
>  
>  '
>  
>  cat >expect <<\EOF
> -Already applied: 0001 A
> -Already applied: 0002 B
> -Committed: 0003 Z
> +Committed: 0001 Z
>  EOF
>  
>  test_expect_success 'rebase -m' '
> -- 
> 1.7.3.2
> 
> 

Re: [PATCH] rebase -m: skip cherry-picked commits more reliably

[Martin von Zweigbergk]

On Tue, 23 Nov 2010, Martin von Zweigbergk wrote:

> What I think we really want to do is to remove any patches in
> upstream..branch that also exist in branch..onto. I don't know how to
> do that efficiently, though. (And am I even right about it?)

To try to answer my own question, would the lines prefixed with a plus
in the output of 'git cherry $onto $branch $(git merge-base $upstream
$branch)' work?

/Martin