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

>
> Is that a derived form of the refname, just like %(refname:short)
> that is 'master' for a ref whose %(refname) is 'refs/heads/master'
> is a derived form of %(refname), and ":short" is what tells the
> formatting machinery what kind of derivation is desired?
>
> If so would %(refname:dir) & %(refname:base) be more in line with
> the overall structure?

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> On Sat, Oct 3, 2015 at 3:32 PM, Matthieu Moy
>> <Matthieu.Moy@grenoble-inp.fr> wrote:
>>> Karthik Nayak <karthik.188@gmail.com> 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.
>
> Note: I don't completely like %(dirname) either. I'm convinced it's
> better than %(path), but there may be a better option.
>

>>>> +             } 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,
>
> Oops, indeed, for refs/foo, sp points to / and ep is NULL. It would
> segfault for any ref without a /. Currently, the only such ref is HEAD
> and it is dealt with by the 'if' above, but that still sounds a bit
> fragile to me. Ifever we allow listing refs like FETCH_HEAD, I'm not
> sure we'll remember that and test %(path) on it.
>
> Adding something like
>
> if (!sp)
>         continue;
>
> after the first strchr would make me feel safer.
>

>> 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 :)
>
> I think it's worse than that because ep will be NULL, and then you call
>
>         v->s = xstrndup(sp, ep - sp);
>