On Mon, Jun 8, 2015 at 5:39 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Will Palmer <wmpalmer@xxxxxxxxx> writes: >> diff --git a/t/t1511-rev-parse-caret.sh b/t/t1511-rev-parse-caret.sh >> index e0fe102..8a5983f 100755 >> --- a/t/t1511-rev-parse-caret.sh >> +++ b/t/t1511-rev-parse-caret.sh >> @@ -19,13 +19,17 @@ test_expect_success 'setup' ' >> echo modified >>a-blob && >> git add -u && >> git commit -m Modified && >> + git branch modref && > > This probably belongs to the previous step, no? As it isn't referenced until the "negative" tests, I didn't bother adding it in the "verify the way things are" tests. Funny that it was mentioned, as I *did* originally have it in the first commit, but I moved it to the commit in which it was first used, so that it would be easier to notice. > >> +test_expect_success 'ref^{/!-}' ' >> + test_must_fail git rev-parse master^{/!-} >> +' > > Hmmmm, we must fail because...? We are looking for something that > does not contain an empty string, which by definition does not > exist. > > Funny, but is correct ;-). This is left-over from the original patch's logic, which included a short-circuit to avoid an empty regex (as per 4322842 "get_sha1: handle special case $commit^{/}")... which I now realise perhaps should have been simply rephrased, rather than ommitted entirely. I feel like adding something like: 8<---------------------------------------------------------------------- --- a/sha1_name.c +++ b/sha1_name.c @@ -737,11 +737,15 @@ static int peel_onion(const char *name, int len, unsigned char *sha1) /* * $commit^{/}. Some regex implementation may reject. - * We don't need regex anyway. '' pattern always matches. + * We don't need regex anyway. '' pattern always matches, + * and '!' pattern never matches. */ if (sp[1] == '}') return 0; + if (sp[1] == '!' && sp[2] == '-' && sp[3] == '}') + return -1; + prefix = xstrndup(sp + 1, name + len - 1 - (sp + 1)); commit_list_insert((struct commit *)o, &list); ret = get_sha1_oneline(prefix, sha1, list); ---------------------------------------------------------------------->8 ...would be the wrong place for this short-circuit check, in light of discussion around extensibility; so, I'll see how it looks moving that into get_sha1_oneline(...) > > >> +test_expect_success 'ref^{/!-.}' ' >> + test_must_fail git rev-parse master^{/!-.} >> +' > > Likewise. I however wonder if we catch a commit without any message > (which you probably have to craft with either commit-tree or > hash-object), but that falls into the "curiosity" not the > "practicality" category. A commit with "no message" should indeed by returned by 'master^{/!-.}', or at least, that is the intent. This test is only meant to cover the result of there being "no matching commit", however. In summary: it looks like I'll be sending another one. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html