[PATCH] t3400 (rebase): add failing test for a peculiar rev spec

[PATCH] t3400 (rebase): add failing test for a peculiar rev spec

[Ramkumar Ramachandra]

'git rebase' does not recognize revisions specified as :/text.  This
is because the attempts to rev-parse ${REV}^0, which fails in this
case.  Add a test to document this failure.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Okay, so I'm not sure what the right fix for this is.

 - The :/text syntax seems to be broken in the first place, as it
   can't be combined with ^ or @.  I'd like to be able to say
   {:/bardery}^1, or even {:/foomery}^{/communist mule}.  Why
   shouldn't I be allowed to do this?

 - The failure occurs in git-rebase.sh:403.  Is it using the ^0 only
   to make sure that the revision specified is a commit?  Surely,
   there'a a better way to do this?

 Can someone point me in the right direction?

 t/t3400-rebase.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index f6cc102..42ca1e0 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -88,6 +88,14 @@ test_expect_success 'rebase fast-forward to master' '
 	test_i18ngrep "Fast-forwarded HEAD to my-topic-branch" out
 '
 
+test_expect_failure 'rebase against revision specified as :/text' '
+	git checkout my-topic-branch^ &&
+	sha1=$(git rev-parse ":/Add B") &&
+	git rebase $sha1 &&
+	git checkout my-topic-branch^ &&
+	git rebase ":/Add B"
+'
+
 test_expect_success 'the rebase operation should not have destroyed author information' '
 	! (git log | grep "Author:" | grep "<>")
 '
-- 
1.8.2.1.423.g4fb5c0a.dirty

Re: [PATCH] t3400 (rebase): add failing test for a peculiar rev spec

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> 'git rebase' does not recognize revisions specified as :/text.  This
> is because the attempts to rev-parse ${REV}^0, which fails in this
> case.  Add a test to document this failure.

>  - The failure occurs in git-rebase.sh:403.  Is it using the ^0 only
>    to make sure that the revision specified is a commit?  Surely,
>    there'a a better way to do this?
>
>  Can someone point me in the right direction?

How about ${REV}^0 into

	nREV=$(git rev-parse "${REV}")^0

and use it where you need an object name that needs to be parsed by
get_sha1(), e.g.
	
	git checkout -q "$nREV^0"

I would suggest a helper function in git-sh-setup, something like:

	peel_committish () {
		case "$1" in
                :/*)	
			peeltmp=$(git rev-parse --verify "$1") &&
                	git rev-parse --verify "$peeltmp^0"
                        ;;
		*)
			git rev-parse --verify "$1^0"
                        ;;
		esac
	}

Re: [PATCH] t3400 (rebase): add failing test for a peculiar rev spec

[Ramkumar Ramachandra]

Junio C Hamano wrote:
>         peel_committish () {
>                 case "$1" in
>                 :/*)
>                         peeltmp=$(git rev-parse --verify "$1") &&
>                         git rev-parse --verify "$peeltmp^0"
>                         ;;
>                 *)
>                         git rev-parse --verify "$1^0"
>                         ;;
>                 esac
>         }

Thanks.  Yeah, that's the obvious "hack solution": special handling
for refspecs of the kind :/text.  Will write a patch for it soon
(currently struggling to complete rebase.autostash).  In the meantime,
please queue this patch.

I'm more interested in knowing what you think of my first point: is
:/text fundamentally broken, as it can't be combined with other
operators like the other rev specs can?  If so, how do you think we
should fix it?

Re: [PATCH] t3400 (rebase): add failing test for a peculiar rev spec

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> I'm more interested in knowing what you think of my first point: is
> :/text fundamentally broken, as it can't be combined with other
> operators like the other rev specs can?  If so, how do you think we
> should fix it?

The question :/string wants to ask is fundamentally ambiguous once
you start wanting to give arbitrary string to it.

Think what this asks:

    git show ":/Fix regression introduced by commit v1.5.1-rc1"~113^2

Is it looking for a commit that fixes v1.5.1-rc1 and then locate the
side branch that waas merged by the 113th-generation parent of that
fix?  Or is it looking for the commit that fixes v1.5.1-rc1~113^2?

When we introduced a postfix $rev^{/string} operator at 32574b68c57f
(get_sha1: support $commit^{/regex} syntax, 2010-12-13) to dig from
a given rev, we designed it so that we can disambiguate between the
two: HEAD^{/Fix v1.5.1~113^2} vs HEAD^{/Fix v1.5.1}~113^2.

But the tradeoff made with 28a4d9404438 (object name: introduce
':/<oneline prefix>' notation, 2007-02-24), which is much older than
the $rev^{/string} syntax, is to help casual interactive use by not
requiring sometimes-hard-to-type line noise characters.

Even back then, I did not want to paint us into a corner we cannot
escape from, and made sure the syntax has an escape hatch built-in.

cf.

  http://thread.gmane.org/gmane.comp.version-control.git/40460/focus=40477

If you cared deeply enough, you could activate it, to take something
like:

    git show ":/!(s=Fix regression by v1.5.1-rc1~113^2)~20"

to say "The 20th-generation parent of the commit whose subject says
it fixed regression at v1.5.1-rc1~113^2".

That would make this

	":/!(a=Ramkumar)(s=move diff.wordRegex)~4"

a way to find the fourth-generation parent of 2ead7a674d7f (t5516
(fetch-push): update test description, 2013-04-02) as a natural
extension, I guess.

Re: [PATCH] t3400 (rebase): add failing test for a peculiar rev spec

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> That would make this
>
> 	":/!(a=Ramkumar)(s=move diff.wordRegex)~4"
>
> a way to find the fourth-generation parent of ...

... 0b830ac52137 (Documentation: move diff.wordRegex from config.txt
to diff-config.txt, 2012-11-13)

I had a log output on another terminal and cut&paste a wrong one in
the original message.

Re: [PATCH] t3400 (rebase): add failing test for a peculiar rev spec

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>>         peel_committish () {
>>                 case "$1" in
>>                 :/*)
>>                         peeltmp=$(git rev-parse --verify "$1") &&
>>                         git rev-parse --verify "$peeltmp^0"
>>                         ;;
>>                 *)
>>                         git rev-parse --verify "$1^0"
>>                         ;;
>>                 esac
>>         }
>
> Thanks.  Yeah, that's the obvious "hack solution": special handling
> for refspecs of the kind :/text.

By the way, that is not a "hack", but is merely "working within the
given constraints".  Welcome to the real life.

Unlike "rebase" that has the luxury of possibly update the
underlying plumbing, third-party tools have to work within the
constraints of existing versions of Git.  It is better to learn that
you sometimes need to do so, and how to do so when you need to.