On Mon, Oct 5, 2015 at 12:14 AM, Junio C Hamano <gits...@pobox.com> wrote:
>
> 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?
This seems way better, I like these names more.
On Sun, Oct 4, 2015 at 11:21 PM, Matthieu Moy
<matthieu....@grenoble-inp.fr> wrote:
> Karthik Nayak <karthik....@gmail.com> writes:
>
>> On Sat, Oct 3, 2015 at 3:32 PM, Matthieu Moy
>> <matthieu....@grenoble-inp.fr> wrote:
>>> Karthik Nayak <karthik....@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.
>
I like Junio's suggestion, stick with that?
>>>> + } 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.
>
Good point! I'll add that.
>> 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);
>
Ah yes, weirdly though my test didn't yield a problem.
~ git update-ref refs/foo master
~ git for-each-ref --format="%(refname) %(color:red)%(dirname:short)"
refs/foo foo
~ git for-each-ref --format="%(refname) %(color:red)%(dirname)"
refs/foo refs/foo
--
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
