Re: [PATCH 3/9] ref-filter: add support for %(path) atom

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

 



On Sat, Oct 3, 2015 at 3:32 PM, Matthieu Moy
<Matthieu.Moy@xxxxxxxxxxxxxxx> wrote:
> Karthik Nayak <karthik.188@xxxxxxxxx> writes:
>
>> This adds %(path) and %(path:short) atoms. The %(path) atom will print
>> the path of the given ref, while %(path:short) will only print the
>> subdirectory of the given ref.
>
> What does "path" mean in this context? How is it different from
> %(refname)?
>
> I found the answer below, but I could not guess from the doc and commit
> message. Actually, I'm not sure %(path) is the right name. If you want
> the "file/directory" analogy, then %(dirname) would be better.
>

Noted will change.

>> +             } else if (match_atom_name(name, "path", &valp)) {
>> +                     const char *sp, *ep;
>> +
>> +                     if (ref->kind & FILTER_REFS_DETACHED_HEAD)
>> +                             continue;
>> +
>> +                     sp = strchr(ref->refname, '/');
>> +                     ep = strchr(++sp, '/');
>
> This assumes you have two / in the fullrefname. It is normally the case,
> but one can also create eg. refs/foo references. AFAIK, in this case sp
> will be NULL, and you're then calling strchr(++NULL) which may segfault.
>

Not really, but you made me think of another possible issue.

Assume refs/foo "sp = strchr(ref->refname, '/');" would set sp to point at
'/' and ++sp will now point at 'f'.

The problem now is refs/foo is supposed to print just "refs" whereas it'll
print "refs/foo". and %(dirname:short) is supposed to print "" whereas it'll
print "foo". Will look into this :)

>> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
>> index d7f7a18..5557657 100755
>> --- a/t/t6302-for-each-ref-filter.sh
>> +++ b/t/t6302-for-each-ref-filter.sh
>> @@ -312,6 +312,7 @@ test_expect_success 'check %(if:equals=<string>)' '
>>       test_cmp expect actual
>>  '
>>
>> +
>>  test_expect_success 'check %(if:notequals=<string>)' '
>
> Useless new blank line.
>

Will remove.

>> +test_expect_success 'check %(path)' '
>> +     git for-each-ref --format="%(path)" >actual &&
>> +     cat >expect <<-\EOF &&
>> +     refs/heads
>
> You should add eg.
>
> git update-ref refs/foo HEAD
> git update-ref refs/foodir/bar/boz HEAD
>
> before the test to check and document the behavior for such refnames.

Yeah makes sense.

-- 
Regards,
Karthik Nayak
--
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]