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

[Matthieu Moy]

Junio C Hamano  writes:

> Matthieu Moy  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.
>
> 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?

Yes, indeed much better. It's still about the refnames, so a specialized
version of %(refname) makes much more sense than a new atom.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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

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

[Karthik Nayak]

On Sat, Oct 3, 2015 at 3:32 PM, Matthieu Moy
 wrote:
> Karthik Nayak  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", )) {
>> + 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=)' '
>>   test_cmp expect actual
>>  '
>>
>> +
>>  test_expect_success 'check %(if:notequals=)' '
>
> 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Matthieu Moy]

Karthik Nayak  writes:

> On Sat, Oct 3, 2015 at 3:32 PM, Matthieu Moy
>  wrote:
>> Karthik Nayak  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", )) {
>>> + 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);

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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

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

[Junio C Hamano]

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

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

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

[Karthik Nayak]

On Mon, Oct 5, 2015 at 12:14 AM, Junio C Hamano  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
 wrote:
> Karthik Nayak  writes:
>
>> On Sat, Oct 3, 2015 at 3:32 PM, Matthieu Moy
>>  wrote:
>>> Karthik Nayak  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", )) {
 + 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

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

[Matthieu Moy]

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

> + } else if (match_atom_name(name, "path", )) {
> + 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.

> 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=)' '
>   test_cmp expect actual
>  '
>  
> +
>  test_expect_success 'check %(if:notequals=)' '

Useless new blank line.

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

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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

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

[Karthik Nayak]

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.

Add tests and documentation for the same.
---
 Documentation/git-for-each-ref.txt |  5 +
 ref-filter.c   | 17 +
 t/t6302-for-each-ref-filter.sh | 39 ++
 3 files changed, 61 insertions(+)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index 5c12c2f..6a476ba 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -145,6 +145,11 @@ if::
compare the value between the %(if:...) and %(then) atoms with the
given string.
 
+path::
+   The path of the given ref. For a shortened version listing
+   only the name of the directory the ref is under append
+   `:short`.
+
 In addition to the above, for commit and tag objects, the header
 field names (`tree`, `parent`, `object`, `type`, and `tag`) can
 be used to specify the value in the header field.
diff --git a/ref-filter.c b/ref-filter.c
index da7723b..b0e86ae 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -58,6 +58,7 @@ static struct {
{ "if" },
{ "then" },
{ "else" },
+   { "path" },
 };
 
 #define REF_FORMATTING_STATE_INIT  { 0, NULL }
@@ -1042,6 +1043,22 @@ static void populate_value(struct ref_array_item *ref)
} else if (!strcmp(name, "else")) {
v->handler = else_atom_handler;
continue;
+   } else if (match_atom_name(name, "path", )) {
+   const char *sp, *ep;
+
+   if (ref->kind & FILTER_REFS_DETACHED_HEAD)
+   continue;
+
+   sp = strchr(ref->refname, '/');
+   ep = strchr(++sp, '/');
+
+   if (!valp)
+   sp = ref->refname;
+   else if (strcmp(valp, "short"))
+   die(_("format: invalid value given path:%s"), 
valp);
+
+   v->s = xstrndup(sp, ep - sp);
+   continue;
} else
continue;
 
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=)' '
test_cmp expect actual
 '
 
+
 test_expect_success 'check %(if:notequals=)' '
git for-each-ref 
--format="%(if:notequals=master)%(refname:short)%(then)Not master%(else)Found 
master%(end)" refs/heads/ >actual &&
cat >expect <<-\EOF &&
@@ -321,4 +322,42 @@ test_expect_success 'check %(if:notequals=)' '
test_cmp expect actual
 '
 
+test_expect_success 'check %(path)' '
+   git for-each-ref --format="%(path)" >actual &&
+   cat >expect <<-\EOF &&
+   refs/heads
+   refs/heads
+   refs/odd
+   refs/tags
+   refs/tags
+   refs/tags
+   refs/tags
+   refs/tags
+   refs/tags
+   refs/tags
+   refs/tags
+   refs/tags
+   EOF
+   test_cmp expect actual
+'
+
+test_expect_success 'check %(path:short)' '
+   git for-each-ref --format="%(path:short)" >actual &&
+   cat >expect <<-\EOF &&
+   heads
+   heads
+   odd
+   tags
+   tags
+   tags
+   tags
+   tags
+   tags
+   tags
+   tags
+   tags
+   EOF
+   test_cmp expect actual
+'
+
 test_done
-- 
2.6.0

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