[PATCH 0/3] Get rebase to work with :/foomery committish

[PATCH 0/3] Get rebase to work with :/foomery committish

[Ramkumar Ramachandra]

Since the early preview, I realized that peel_committish() is required
in exactly two places: the <onto> and <upstream> parsers facing
end-user data.  Updated appropriately.

Thanks.

Ramkumar Ramachandra (3):
  t/rebase: add failing tests for a peculiar revision
  sh-setup: add new peel_committish() helper
  rebase: use peel_committish() where appropriate

 git-rebase.sh                 |  4 ++--
 git-sh-setup.sh               | 12 ++++++++++++
 t/t3400-rebase.sh             | 11 +++++++++++
 t/t3404-rebase-interactive.sh | 11 +++++++++++
 4 files changed, 36 insertions(+), 2 deletions(-)

-- 
1.8.3.1.381.g12ca056.dirty

[PATCH 1/3] t/rebase: add failing tests for a peculiar revision

[Ramkumar Ramachandra]

The following commands fail, even if :/quuxery and :/foomery resolve to
perfectly valid commits:

  $ git rebase [-i] --onto :/quuxery :/foomery

This is because rebase [-i] attempts to rev-parse ${REV}^0 to verify
that the given revision resolves to a commit.  Add tests to document
these failures.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t3400-rebase.sh             | 11 +++++++++++
 t/t3404-rebase-interactive.sh | 11 +++++++++++
 2 files changed, 22 insertions(+)

diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index b58fa1a..81ec517 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -88,6 +88,17 @@ 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_when_finished "git branch -D torebase" &&
+	git checkout -b torebase my-topic-branch^ &&
+	upstream=$(git rev-parse ":/Add B") &&
+	onto=$(git rev-parse ":/Add A") &&
+	git rebase --onto $onto $upstream &&
+	git reset --hard my-topic-branch^ &&
+	git rebase --onto ":/Add A" ":/Add B" &&
+	git checkout my-topic-branch
+'
+
 test_expect_success 'the rebase operation should not have destroyed author information' '
 	! (git log | grep "Author:" | grep "<>")
 '
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 79e8d3c..eb241f5 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -947,4 +947,15 @@ 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_when_finished "git branch -D torebase" &&
+	git checkout -b torebase branch1 &&
+	upstream=$(git rev-parse ":/J") &&
+	onto=$(git rev-parse ":/A") &&
+	git rebase --onto $onto $upstream &&
+	git reset --hard branch1 &&
+	git rebase --onto ":/A" ":/J" &&
+	git checkout branch1
+'
+
 test_done
-- 
1.8.3.1.381.g12ca056.dirty

[PATCH 2/3] sh-setup: add new peel_committish() helper

[Ramkumar Ramachandra]

The normal way to check whether a certain revision resolves to a valid
commit is:

  $ git rev-parse --verify $REV^0

Unfortunately, this does not work when $REV is of the type :/quuxery.
Write a helper to work around this limitation.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 git-sh-setup.sh | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 2f78359..7a964ad 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -313,3 +313,15 @@ then
 	}
 	: ${GIT_OBJECT_DIRECTORY="$GIT_DIR/objects"}
 fi
+
+peel_committish () {
+	case "$1" in
+	:/*)
+		peeltmp=$(git rev-parse --verify "$1") &&
+		git rev-parse --verify "${peeltmp}^0"
+		;;
+	*)
+		git rev-parse --verify "${1}^0"
+		;;
+	esac
+}
-- 
1.8.3.1.381.g12ca056.dirty

Re: [PATCH 2/3] sh-setup: add new peel_committish() helper

[Philip Oakley]

From: "Ramkumar Ramachandra" <artagnon@gmail.com>
Sent: Friday, June 14, 2013 2:17 PM
> The normal way to check whether a certain revision resolves to a valid
> commit is:
>
>  $ git rev-parse --verify $REV^0
>
> Unfortunately, this does not work when $REV is of the type :/quuxery.

Is there a proper name for this style of revision specification? I've 
been letting this 'style' wash over me in the hope that I'd understand 
eventually, but it hasn't.

Loking at git-rev-parse I now see that it might be the 'Commit Message 
Regex' rev specifier.

If re-rolled, can this elucidation be included in the commit message?

> Write a helper to work around this limitation.
>
> Suggested-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
> git-sh-setup.sh | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
> index 2f78359..7a964ad 100644
> --- a/git-sh-setup.sh
> +++ b/git-sh-setup.sh
> @@ -313,3 +313,15 @@ then
>  }
>  : ${GIT_OBJECT_DIRECTORY="$GIT_DIR/objects"}
> fi
> +
> +peel_committish () {
> + case "$1" in
> + :/*)
> + peeltmp=$(git rev-parse --verify "$1") &&
> + git rev-parse --verify "${peeltmp}^0"
> + ;;
> + *)
> + git rev-parse --verify "${1}^0"
> + ;;
> + esac
> +}
> -- 
> 1.8.3.1.381.g12ca056.dirty
>
> --
Philip 

Re: [PATCH 2/3] sh-setup: add new peel_committish() helper

[Ramkumar Ramachandra]

Philip Oakley wrote:
> Is there a proper name for this style of revision specification? I've been
> letting this 'style' wash over me in the hope that I'd understand
> eventually, but it hasn't.

See gitrevisions(7).  None of them have any names.

> Loking at git-rev-parse I now see that it might be the 'Commit Message
> Regex' rev specifier.

Did you just invent that term?  I couldn't find any mentions of it.

Re: [PATCH 2/3] sh-setup: add new peel_committish() helper

[Philip Oakley]

From: "Ramkumar Ramachandra" <artagnon@gmail.com>
Sent: Friday, June 14, 2013 5:18 PM
> Philip Oakley wrote:
>> Is there a proper name for this style of revision specification? I've 
>> been
>> letting this 'style' wash over me in the hope that I'd understand
>> eventually, but it hasn't.
>
> See gitrevisions(7).  None of them have any names.
... which is a shame...

>
>> Loking at git-rev-parse I now see that it might be the 'Commit 
>> Message
>> Regex' rev specifier.
>
> Did you just invent that term?  I couldn't find any mentions of it.

Yep, hence the 'it might be'.   Maybe I missed a word and should have 
written 'it might be called' ;-)

It would be useful to have some descriptive names for these unusual 
(uncommon) methods.
There's already the "pathspec :(glob) syntax".

Philip 

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

[Ramkumar Ramachandra]

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.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 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") &&
-- 
1.8.3.1.381.g12ca056.dirty

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> 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" ;-)

> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  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

[Ramkumar Ramachandra]

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).

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> 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?

It often is necessary, after applying a patch series that was
prepared against commit that is unnecessarily new (e.g. a bugfix
that should apply to 'maint' prepared against 'next'), to see if the
result rebases on older codebase.  Giving a commit (not branch) to
the command to force rebasing the commit on a detached HEAD is a
very handy technique to do so without damaging the original branch.

    $ git checkout mater^0
    $ git am -s mbox
    Applying A
    Applying B
    Applying C
    $ git rebase --onto maint master ":/B"

would see if the earlier two commits that are pure bugfix cleanly
applies to 'maint' (and then I can rebuild the topic by forking a
branch from 'maint', queue A and B, and if C is not needed for that
fix, fork another from that point, possibly merge 'master' to it and
then queue C).

>> 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).

We are saying the same thing: "don't do it then".