Re: [PATCH 3/3] rebase: use peel_committish() where appropriate

2 messages, 2 authors, 2016-06-15 · open the first message on its own page

Re: [PATCH 3/3] rebase: use peel_committish() where appropriate

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:57:45

Ramkumar Ramachandra [off-list ref] writes:
The revisions specified on the command-line as <onto> and <upstream>
arguments could be of the form :/quuxery; so, use peel_committish() to
resolve them.  The failing tests in t/rebase and t/rebase-interactive
now pass.
You can also specify the commit at the end of the history to be
rebased (very useful while trial runs to see where a series should
apply):

    git rebase foo ":/Add B"

This is already handled properly because it first gets turned into
an object name $orig_head and then we use it (without ^0) to update
the ORIG_HEAD.

Even after this patch, there is

	git checkout -q "$onto^0"

when detaching the HEAD to that commit.  Can that peeling be dropped
now (I am not suggesting to drop it in this patch)?

What would happen when you are given "--onto :/f...o" is somewhat
interesting, but that may be a separate topic, I think.  At that
point, it is probably in the realm of "don't do it then" ;-)
quoted hunk
Signed-off-by: Ramkumar Ramachandra <redacted>
---
 git-rebase.sh                 | 4 ++--
 t/t3400-rebase.sh             | 2 +-
 t/t3404-rebase-interactive.sh | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/git-rebase.sh b/git-rebase.sh
index d0c11a9..6987b9b 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -434,7 +434,7 @@ then
 		shift
 		;;
 	esac
-	upstream=`git rev-parse --verify "${upstream_name}^0"` ||
+	upstream=$(peel_committish "${upstream_name}") ||
 	die "$(eval_gettext "invalid upstream \$upstream_name")"
 	upstream_arg="$upstream_name"
 else
@@ -470,7 +470,7 @@ case "$onto_name" in
 	fi
 	;;
 *)
-	onto=$(git rev-parse --verify "${onto_name}^0") ||
+	onto=$(peel_committish "$onto_name") ||
 	die "$(eval_gettext "Does not point to a valid commit: \$onto_name")"
 	;;
 esac
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 81ec517..cbca71e 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -88,7 +88,7 @@ test_expect_success 'rebase fast-forward to master' '
 	test_i18ngrep "Fast-forwarded HEAD to my-topic-branch" out
 '
 
-test_expect_failure 'rebase, with <onto> and <upstream> specified as :/quuxery' '
+test_expect_success 'rebase, with <onto> and <upstream> specified as :/quuxery' '
 	test_when_finished "git branch -D torebase" &&
 	git checkout -b torebase my-topic-branch^ &&
 	upstream=$(git rev-parse ":/Add B") &&
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index eb241f5..86917d1 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -947,7 +947,7 @@ test_expect_success 'rebase -i respects core.commentchar' '
 	test B = $(git cat-file commit HEAD^ | sed -ne \$p)
 '
 
-test_expect_failure 'rebase -i, with <onto> and <upstream> specified as :/quuxery' '
+test_expect_success 'rebase -i, with <onto> and <upstream> specified as :/quuxery' '
 	test_when_finished "git branch -D torebase" &&
 	git checkout -b torebase branch1 &&
 	upstream=$(git rev-parse ":/J") &&

Re: [PATCH 3/3] rebase: use peel_committish() where appropriate

From: Ramkumar Ramachandra <hidden>
Date: 2016-06-15 22:57:45

Junio C Hamano wrote:
You can also specify the commit at the end of the history to be
rebased (very useful while trial runs to see where a series should
apply):

    git rebase foo ":/Add B"

This is already handled properly because it first gets turned into
an object name $orig_head and then we use it (without ^0) to update
the ORIG_HEAD.
Correct, but what sense does it make unless <branch> is a ref to update?

  $ git rebase master :/v1.8.1
  First, rewinding head to replay your work on top of it...
  Fast-forwarded :/v1.8.1 to master.

Huh?  The message is wrong, and no end-user can figure out what happened.
Even after this patch, there is

        git checkout -q "$onto^0"

when detaching the HEAD to that commit.  Can that peeling be dropped
now (I am not suggesting to drop it in this patch)?
Yeah, that can be dropped.
What would happen when you are given "--onto :/f...o" is somewhat
interesting, but that may be a separate topic, I think.  At that
point, it is probably in the realm of "don't do it then" ;-)
The utility of this very series can be questioned.  I've rarely wanted
to use the :/fommery with rebase, so this mostly an exercise in
"theoretical correctness" (something I usually stay away from).
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help