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

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Oct 4, 2015, 17:51 UTC
Message-ID: <vpqfv1q5xop.fsf@grenoble-inp.fr>
In-Reply-To: <CAOLa=ZSdM34_J+gb_=K4-9fgNsMfseAYxUYGm_ncWZstcW+SYw@mail.gmail.com>

Karthik Nayak <karthik.188@gmail.com> writes:

Show 17 quoted lines

> 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.

Show 14 quoted lines

>>> +             } 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.

Show 8 quoted lines

> 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);

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/