From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Tue, 28 Jul 2015 08:50:41 GMT
Subject: Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom
Message-ID: <vpq7fpk1x9a.fsf@anie.imag.fr>
In-Reply-To: <1438066594-5620-4-git-send-email-Karthik.188@gmail.com>

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

> --- a/t/t6302-for-each-ref-filter.sh
> +++ b/t/t6302-for-each-ref-filter.sh
> @@ -149,4 +149,25 @@ test_expect_success 'check `colornext` format option' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'check `ifexists` format option' '
> +	cat >expect <<-\EOF &&
> +	[foo1.10]
> +	[foo1.3]
> +	[foo1.6]
> +	EOF
> +	git for-each-ref --format="%(ifexists:[%s])%(refname:short)" | grep "foo" >actual &&
> +	test_cmp expect actual
> +'

You're testing only the positive case of ifexists, right? You need a
test for the negative case (i.e. the attribute does not exist) too.
Ideally, check that the ifexist actually applies to the right attribute,
like

--format '%(ifexist:<%s>)%(attribute1) %(ifexist:[%s])%(attribute2)'

and manage to get an output like

<foo>
 [bar]
<foobar> [barfoo]

> +cat >expect <<EOF &&
> +[$(get_color green)foo1.10$(get_color reset)]||foo1.10
> +[$(get_color green)foo1.3$(get_color reset)]||foo1.3
> +[$(get_color green)foo1.6$(get_color reset)]||foo1.6
> +EOF
> +
> +test_expect_success 'check `ifexists` with `colornext` format option' '
> +	git for-each-ref --format="%(ifexists:[%s])%(colornext:green)%(refname:short)||%(refname:short)" | grep "foo" >actual &&
> +	test_cmp expect actual
> +'

You have a double || that is not useful. Not really harmful either, but
it may distract the reader. You may want to s/||/|/.

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