Re: [PATCH v7 03/11] ref-filter: add option to pad atoms to the right

[Eric Sunshine <sunshine@sunshineco.com>]

On Thu, Jul 30, 2015 at 11:48 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> Add a new atom "padright" and support %(padright:X) where X is a
> number.  This will align the succeeding atom or string to the left
> followed by spaces for a total length of X characters. If X is less
> than the atom or string length then no padding is done.
>
> Add tests and documentation for the same.
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
> index 505a360..48caac9 100755
> --- a/t/t6302-for-each-ref-filter.sh
> +++ b/t/t6302-for-each-ref-filter.sh
> @@ -81,4 +81,36 @@ test_expect_success 'filtering with --contains' '
>         test_cmp expect actual
>  '
>
> +test_expect_success 'padding to the right using `padright`' '
> +       cat >expect <<-\EOF &&
> +       master|    master    |
> +       side|    side      |
> +       odd/spot|    odd/spot  |
> +       double-tag|    double-tag|
> +       four|    four      |
> +       one|    one       |
> +       signed-tag|    signed-tag|
> +       three|    three     |
> +       two|    two       |
> +       EOF
> +       git for-each-ref --format="%(refname:short)%(padright:5)|%(padright:10)%(refname:short)|" >actual &&
> +       test_cmp expect actual
> +'

In an earlier review, Matthieu pointed out that this test failed to
ensure that the 'padright' value did not leak into the next atom. In a
subsequent version, you fixed the test to check that condition, but
now you've somewhat lost it again, at least visually. That is, because
whitespace is "invisible" and because 'padright' now also affects
literal strings, someone reading this test can't tell if those
trailing |'s in the expected output are padded or not. You could use a
different format string to prove that the 'padright' value doesn't
leak. For instance:

    %(padright:10)%(refname:short)|%(padright:5)|%(refname:short)

This way, as long as the two |'s are side-by-side, then you've proved
that the first one wasn't affected by the preceding 'padright:10'. You
could also add back the %(refname:short) at the front of the pattern,
as you currently have it, if you want to prove that padding is not
enabled at the start of format.

> +test_expect_success 'no padding when `padright` length is smaller than atom length' '
> +       cat >expect <<-\EOF &&
> +       refs/heads/master|
> +       refs/heads/side|
> +       refs/odd/spot|
> +       refs/tags/double-tag|
> +       refs/tags/four|
> +       refs/tags/one|
> +       refs/tags/signed-tag|
> +       refs/tags/three|
> +       refs/tags/two|
> +       EOF
> +       git for-each-ref --format="%(padright:5)%(refname)|" >actual &&
> +       test_cmp expect actual
> +'
> +
>  test_done
> --
> 2.4.6