threads / patch / 33534

patcht3400 (rebase): add failing test for a peculiar rev spec

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

## tl;dr

6 messages between Apr 18, 2013 and Apr 19, 2013. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

Ramkumar Ramachandra· Apr 18, 2013, 08:52 UTC · lore

'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(+)
Show changes to t/t3400-rebase.sh +8 −0
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
Junio C Hamano· Apr 18, 2013, 16:39 UTC · re: Ramkumar Ramachandra · lore

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

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.
Show 5 quoted lines
>  - 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
	}
Ramkumar Ramachandra· Apr 19, 2013, 07:43 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
Show 11 quoted lines
>         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?

Junio C Hamano· Apr 19, 2013, 16:21 UTC · re: Ramkumar Ramachandra · lore

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

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.

Junio C Hamano· Apr 19, 2013, 17:28 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 5 quoted lines
> 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.

Junio C Hamano· Apr 19, 2013, 17:53 UTC · re: Ramkumar Ramachandra · lore

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

Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 15 quoted lines
> 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.

← back to recent threads