Re: [PATCH v2 2/2] object name: introduce '^{/!-<negative pattern>}' notation

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]