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

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Oct 3, 2015, 10:02 UTC
Message-ID: <vpqwpv49so7.fsf@grenoble-inp.fr>
In-Reply-To: <1443807546-5985-4-git-send-email-Karthik.188@gmail.com>

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.

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

Show 10 quoted lines

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

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