{"thread":{"id":"33534","subject":"[PATCH] t3400 (rebase): add failing test for a peculiar rev spec","startedAt":"2013-04-18T08:52:35Z","lastAt":"2013-04-19T17:53:37Z","messageCount":6,"participants":["Ramkumar Ramachandra","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"214696","messageId":"1366275155-26244-1-git-send-email-artagnon@gmail.com","threadId":"33534","inReplyTo":null,"subject":"[PATCH] t3400 (rebase): add failing test for a peculiar rev spec","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-04-18T08:52:35Z","receivedAt":"2013-04-18T08:52:35Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"'git rebase' does not recognize revisions specified as :/text. This\nis because the attempts to rev-parse ${REV}^0, which fails in this\ncase. Add a test to document this failure.\n\nSigned-off-by: Ramkumar Ramachandra \n---\n Okay, so I'm not sure what the right fix for this is.\n\n - The :/text syntax seems to be broken in the first place, as it\n can't be combined with ^ or @. I'd like to be able to say\n {:/bardery}^1, or even {:/foomery}^{/communist mule}. Why\n shouldn't I be allowed to do this?\n\n - The failure occurs in git-rebase.sh:403. Is it using the ^0 only\n to make sure that the revision specified is a commit? Surely,\n there'a a better way to do this?\n\n Can someone point me in the right direction?\n\n t/t3400-rebase.sh | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex f6cc102..42ca1e0 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -88,6 +88,14 @@ test_expect_success 'rebase fast-forward to master' '\n \ttest_i18ngrep \"Fast-forwarded HEAD to my-topic-branch\" out\n '\n \n+test_expect_failure 'rebase against revision specified as :/text' '\n+\tgit checkout my-topic-branch^ &&\n+\tsha1=$(git rev-parse \":/Add B\") &&\n+\tgit rebase $sha1 &&\n+\tgit checkout my-topic-branch^ &&\n+\tgit rebase \":/Add B\"\n+'\n+\n test_expect_success 'the rebase operation should not have destroyed author information' '\n \t! (git log | grep \"Author:\" | grep \"<>\")\n '\n-- \n1.8.2.1.423.g4fb5c0a.dirty\n"},{"id":"214733","messageId":"7vmwsv94jo.fsf@alter.siamese.dyndns.org","threadId":"33534","inReplyTo":"1366275155-26244-1-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH] t3400 (rebase): add failing test for a peculiar rev spec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-18T16:39:23Z","receivedAt":"2013-04-18T16:39:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> 'git rebase' does not recognize revisions specified as :/text. This\n> is because the attempts to rev-parse ${REV}^0, which fails in this\n> case. Add a test to document this failure.\n\n> - The failure occurs in git-rebase.sh:403. Is it using the ^0 only\n> to make sure that the revision specified is a commit? Surely,\n> there'a a better way to do this?\n>\n> Can someone point me in the right direction?\n\nHow about ${REV}^0 into\n\n\tnREV=$(git rev-parse \"${REV}\")^0\n\nand use it where you need an object name that needs to be parsed by\nget_sha1(), e.g.\n\t\n\tgit checkout -q \"$nREV^0\"\n\nI would suggest a helper function in git-sh-setup, something like:\n\n\tpeel_committish () {\n\t\tcase \"$1\" in\n :/*)\t\n\t\t\tpeeltmp=$(git rev-parse --verify \"$1\") &&\n \tgit rev-parse --verify \"$peeltmp^0\"\n ;;\n\t\t*)\n\t\t\tgit rev-parse --verify \"$1^0\"\n ;;\n\t\tesac\n\t}\n"},{"id":"214817","messageId":"CALkWK0=RDCCSxOJ2TU4=tniSRTg9VJohJ8Pf6uN9E=fd3jg=_Q@mail.gmail.com","threadId":"33534","inReplyTo":"7vmwsv94jo.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] t3400 (rebase): add failing test for a peculiar rev spec","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-04-19T07:43:46Z","receivedAt":"2013-04-19T07:43:46Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> peel_committish () {\n> case \"$1\" in\n> :/*)\n> peeltmp=$(git rev-parse --verify \"$1\") &&\n> git rev-parse --verify \"$peeltmp^0\"\n> ;;\n> *)\n> git rev-parse --verify \"$1^0\"\n> ;;\n> esac\n> }\n\nThanks. Yeah, that's the obvious \"hack solution\": special handling\nfor refspecs of the kind :/text. Will write a patch for it soon\n(currently struggling to complete rebase.autostash). In the meantime,\nplease queue this patch.\n\nI'm more interested in knowing what you think of my first point: is\n:/text fundamentally broken, as it can't be combined with other\noperators like the other rev specs can? If so, how do you think we\nshould fix it?\n"},{"id":"214835","messageId":"7vvc7i330b.fsf@alter.siamese.dyndns.org","threadId":"33534","inReplyTo":"CALkWK0=RDCCSxOJ2TU4=tniSRTg9VJohJ8Pf6uN9E=fd3jg=_Q@mail.gmail.com","subject":"Re: [PATCH] t3400 (rebase): add failing test for a peculiar rev spec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-19T16:21:24Z","receivedAt":"2013-04-19T16:21:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> I'm more interested in knowing what you think of my first point: is\n> :/text fundamentally broken, as it can't be combined with other\n> operators like the other rev specs can? If so, how do you think we\n> should fix it?\n\nThe question :/string wants to ask is fundamentally ambiguous once\nyou start wanting to give arbitrary string to it.\n\nThink what this asks:\n\n git show \":/Fix regression introduced by commit v1.5.1-rc1\"~113^2\n\nIs it looking for a commit that fixes v1.5.1-rc1 and then locate the\nside branch that waas merged by the 113th-generation parent of that\nfix? Or is it looking for the commit that fixes v1.5.1-rc1~113^2?\n\nWhen we introduced a postfix $rev^{/string} operator at 32574b68c57f\n(get_sha1: support $commit^{/regex} syntax, 2010-12-13) to dig from\na given rev, we designed it so that we can disambiguate between the\ntwo: HEAD^{/Fix v1.5.1~113^2} vs HEAD^{/Fix v1.5.1}~113^2.\n\nBut the tradeoff made with 28a4d9404438 (object name: introduce\n':/' notation, 2007-02-24), which is much older than\nthe $rev^{/string} syntax, is to help casual interactive use by not\nrequiring sometimes-hard-to-type line noise characters.\n\nEven back then, I did not want to paint us into a corner we cannot\nescape from, and made sure the syntax has an escape hatch built-in.\n\ncf.\n\n http://thread.gmane.org/gmane.comp.version-control.git/40460/focus=40477\n\nIf you cared deeply enough, you could activate it, to take something\nlike:\n\n git show \":/!(s=Fix regression by v1.5.1-rc1~113^2)~20\"\n\nto say \"The 20th-generation parent of the commit whose subject says\nit fixed regression at v1.5.1-rc1~113^2\".\n\nThat would make this\n\n\t\":/!(a=Ramkumar)(s=move diff.wordRegex)~4\"\n\na way to find the fourth-generation parent of 2ead7a674d7f (t5516\n(fetch-push): update test description, 2013-04-02) as a natural\nextension, I guess.\n"},{"id":"214850","messageId":"7v7gjy2zx3.fsf@alter.siamese.dyndns.org","threadId":"33534","inReplyTo":"7vvc7i330b.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] t3400 (rebase): add failing test for a peculiar rev spec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-19T17:28:08Z","receivedAt":"2013-04-19T17:28:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> That would make this\n>\n> \t\":/!(a=Ramkumar)(s=move diff.wordRegex)~4\"\n>\n> a way to find the fourth-generation parent of ...\n\n... 0b830ac52137 (Documentation: move diff.wordRegex from config.txt\nto diff-config.txt, 2012-11-13)\n\nI had a log output on another terminal and cut&paste a wrong one in\nthe original message.\n"},{"id":"214855","messageId":"7vy5ce1k66.fsf@alter.siamese.dyndns.org","threadId":"33534","inReplyTo":"CALkWK0=RDCCSxOJ2TU4=tniSRTg9VJohJ8Pf6uN9E=fd3jg=_Q@mail.gmail.com","subject":"Re: [PATCH] t3400 (rebase): add failing test for a peculiar rev spec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-19T17:53:37Z","receivedAt":"2013-04-19T17:53:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> Junio C Hamano wrote:\n>> peel_committish () {\n>> case \"$1\" in\n>> :/*)\n>> peeltmp=$(git rev-parse --verify \"$1\") &&\n>> git rev-parse --verify \"$peeltmp^0\"\n>> ;;\n>> *)\n>> git rev-parse --verify \"$1^0\"\n>> ;;\n>> esac\n>> }\n>\n> Thanks. Yeah, that's the obvious \"hack solution\": special handling\n> for refspecs of the kind :/text.\n\nBy the way, that is not a \"hack\", but is merely \"working within the\ngiven constraints\". Welcome to the real life.\n\nUnlike \"rebase\" that has the luxury of possibly update the\nunderlying plumbing, third-party tools have to work within the\nconstraints of existing versions of Git. It is better to learn that\nyou sometimes need to do so, and how to do so when you need to.\n"}]}