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

Karthik Nayak [off-list ref] 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", &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.

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/

On Sat, Oct 3, 2015 at 3:32 PM, Matthieu Moy
[off-list ref] wrote:

Karthik Nayak [off-list ref] writes:

quoted

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.

quoted

+             } 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 :)

quoted

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.

quoted

+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