Re: [PATCH v2 08/11] ref-filter: introduce prefixes for the align atom

[Eric Sunshine <sunshine@sunshineco.com>]

On Wed, Dec 16, 2015 at 10:29 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> ref-filter: introduce prefixes for the align atom

The prefixes are actually for the arguments to the 'align' atom, not
for the atom itself. However, it might be better to describe this at a
bit higher level. Perhaps:

    ref-filter: align: introduce long-form syntax

or something.

> Introduce optional prefixes "width=" and "position=" for the align atom
> so that the atom can be used as "%(align:width=<width>,position=<position>)".
>
> Add Documetation and tests for the same.
>
> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -96,10 +96,19 @@ static void align_atom_parser(struct used_atom *atom)
>
>         while (*s) {
>                 int position;
> +               buf = s[0]->buf;

It probably would be better to do this assignment in the previous
patch (7/11) since its presence here introduces unwanted noise
(textual replacement of "s[0]->buf" with "buf") in several locations
below which slightly obscure the real changes of this patch.

> -               if (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))
> +               if (skip_prefix(buf, "position=", &buf)) {
> +                       position = parse_align_position(buf);
> +                       if (position == -1)

It may be more idiomatic in this codebase to detect errors via '< 0'
rather than explicit '== -1'. Ditto below.

> +                               die(_("unrecognized position:%s"), buf);
> +                       align->position = position;
> +               } else if (skip_prefix(buf, "width=", &buf)) {
> +                       if (strtoul_ui(buf, 10, (unsigned int *)&width))
> +                               die(_("unrecognized width:%s"), buf);
> +               } else if (!strtoul_ui(buf, 10, (unsigned int *)&width))
>                         ;
> -               else if ((position = parse_align_position(s[0]->buf)) > 0)
> +               else if ((position = parse_align_position(buf)) != -1)
>                         align->position = position;
>                 else
>                         die(_("unrecognized %%(align) argument: %s"), s[0]->buf);

s/s[0]->//

More below...

> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
> @@ -133,6 +133,168 @@ test_expect_success 'right alignment' '
>         test_cmp expect actual
>  '
>
> +test_expect_success 'alignment with "position" prefix' '
> +       cat >expect <<-\EOF &&
> +       |  refname is refs/heads/master|refs/heads/master
> +       |    refname is refs/heads/side|refs/heads/side
> +       |      refname is refs/odd/spot|refs/odd/spot
> +       |refname is refs/tags/double-tag|refs/tags/double-tag
> +       |     refname is refs/tags/four|refs/tags/four
> +       |      refname is refs/tags/one|refs/tags/one
> +       |refname is refs/tags/signed-tag|refs/tags/signed-tag
> +       |    refname is refs/tags/three|refs/tags/three
> +       |      refname is refs/tags/two|refs/tags/two
> +       EOF
> +       git for-each-ref --format="|%(align:30,position=right)refname is %(refname)%(end)|%(refname)" >actual &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success 'alignment with "position" prefix' '
> +       cat >expect <<-\EOF &&
> +       |  refname is refs/heads/master|refs/heads/master
> +       |    refname is refs/heads/side|refs/heads/side
> +       |      refname is refs/odd/spot|refs/odd/spot
> +       |refname is refs/tags/double-tag|refs/tags/double-tag
> +       |     refname is refs/tags/four|refs/tags/four
> +       |      refname is refs/tags/one|refs/tags/one
> +       |refname is refs/tags/signed-tag|refs/tags/signed-tag
> +       |    refname is refs/tags/three|refs/tags/three
> +       |      refname is refs/tags/two|refs/tags/two
> +       EOF
> +       git for-each-ref --format="|%(align:position=right,30)refname is %(refname)%(end)|%(refname)" >actual &&
> +       test_cmp expect actual
> +'

This (and below) is a lot of copy/paste code which makes it difficult
to read the tests and maintain (change) them. Since 'expect' doesn't
appear to change from test to test, one way to eliminate some of this
noisiness would be to create 'expect' once outside of the tests.

However, even better, especially from a comprehension,
maintainability, and extensibility standpoints would be to make this
all table-driven. In particular, I'd expect to see a table with
exactly the list of test inputs mentioned in my earlier review[1], and
have that table passed to a shell function which performs the test for
each element of the table. For instance, something like:

    test_align_permutations <<-\EOF
        middle,42
        42,middle
        position=middle,42
        42,position=middle
        middle,width=42
        width=42,middle
        position=middle,width=42
        width=42,position=middle
        EOF

where test_align_permutations is the name of the shell function which
reads each line of it stdin and performs the "git for-each-ref
--format=..." test with the given %(align:) arguments.

Ditto regarding the below "last one wins (silently) tests".

[1]: http://article.gmane.org/gmane.comp.version-control.git/281916

> +
> +test_expect_success 'alignment with "width" prefix' '
> +       cat >expect <<-\EOF &&
> +       |  refname is refs/heads/master|refs/heads/master
> +       |    refname is refs/heads/side|refs/heads/side
> +       |      refname is refs/odd/spot|refs/odd/spot
> +       |refname is refs/tags/double-tag|refs/tags/double-tag
> +       |     refname is refs/tags/four|refs/tags/four
> +       |      refname is refs/tags/one|refs/tags/one
> +       |refname is refs/tags/signed-tag|refs/tags/signed-tag
> +       |    refname is refs/tags/three|refs/tags/three
> +       |      refname is refs/tags/two|refs/tags/two
> +       EOF
> +       git for-each-ref --format="|%(align:width=30,right)refname is %(refname)%(end)|%(refname)" >actual &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success 'alignment with "width" prefix' '
> +       cat >expect <<-\EOF &&
> +       |  refname is refs/heads/master|refs/heads/master
> +       |    refname is refs/heads/side|refs/heads/side
> +       |      refname is refs/odd/spot|refs/odd/spot
> +       |refname is refs/tags/double-tag|refs/tags/double-tag
> +       |     refname is refs/tags/four|refs/tags/four
> +       |      refname is refs/tags/one|refs/tags/one
> +       |refname is refs/tags/signed-tag|refs/tags/signed-tag
> +       |    refname is refs/tags/three|refs/tags/three
> +       |      refname is refs/tags/two|refs/tags/two
> +       EOF
> +       git for-each-ref --format="|%(align:right,width=30)refname is %(refname)%(end)|%(refname)" >actual &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success 'alignment with "position" and "width" prefix' '
> +       cat >expect <<-\EOF &&
> +       |  refname is refs/heads/master|refs/heads/master
> +       |    refname is refs/heads/side|refs/heads/side
> +       |      refname is refs/odd/spot|refs/odd/spot
> +       |refname is refs/tags/double-tag|refs/tags/double-tag
> +       |     refname is refs/tags/four|refs/tags/four
> +       |      refname is refs/tags/one|refs/tags/one
> +       |refname is refs/tags/signed-tag|refs/tags/signed-tag
> +       |    refname is refs/tags/three|refs/tags/three
> +       |      refname is refs/tags/two|refs/tags/two
> +       EOF
> +       git for-each-ref --format="|%(align:width=30,position=right)refname is %(refname)%(end)|%(refname)" >actual &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success 'alignment with "position" and "width" prefix' '
> +       cat >expect <<-\EOF &&
> +       |  refname is refs/heads/master|refs/heads/master
> +       |    refname is refs/heads/side|refs/heads/side
> +       |      refname is refs/odd/spot|refs/odd/spot
> +       |refname is refs/tags/double-tag|refs/tags/double-tag
> +       |     refname is refs/tags/four|refs/tags/four
> +       |      refname is refs/tags/one|refs/tags/one
> +       |refname is refs/tags/signed-tag|refs/tags/signed-tag
> +       |    refname is refs/tags/three|refs/tags/three
> +       |      refname is refs/tags/two|refs/tags/two
> +       EOF
> +       git for-each-ref --format="|%(align:position=right,width=30)refname is %(refname)%(end)|%(refname)" >actual &&
> +       test_cmp expect actual
> +'
> +
> +# Last one wins (silently) when multiple arguments of the same type are given
> +
> +test_expect_success 'alignment with multiple "<width>" values' '
> +       cat >expect <<-\EOF &&
> +       |refname is refs/heads/master  |refs/heads/master
> +       |refname is refs/heads/side    |refs/heads/side
> +       |refname is refs/odd/spot      |refs/odd/spot
> +       |refname is refs/tags/double-tag|refs/tags/double-tag
> +       |refname is refs/tags/four     |refs/tags/four
> +       |refname is refs/tags/one      |refs/tags/one
> +       |refname is refs/tags/signed-tag|refs/tags/signed-tag
> +       |refname is refs/tags/three    |refs/tags/three
> +       |refname is refs/tags/two      |refs/tags/two
> +       EOF
> +       git for-each-ref --format="|%(align:32,width=30)refname is %(refname)%(end)|%(refname)" >actual &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success 'alignment with multiple "<width>" values' '
> +       cat >expect <<-\EOF &&
> +       |refname is refs/heads/master    |refs/heads/master
> +       |refname is refs/heads/side      |refs/heads/side
> +       |refname is refs/odd/spot        |refs/odd/spot
> +       |refname is refs/tags/double-tag |refs/tags/double-tag
> +       |refname is refs/tags/four       |refs/tags/four
> +       |refname is refs/tags/one        |refs/tags/one
> +       |refname is refs/tags/signed-tag |refs/tags/signed-tag
> +       |refname is refs/tags/three      |refs/tags/three
> +       |refname is refs/tags/two        |refs/tags/two
> +       EOF
> +       git for-each-ref --format="|%(align:width=30,32)refname is %(refname)%(end)|%(refname)" >actual &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success 'alignment with multiple "<position>" values' '
> +       cat >expect <<-\EOF &&
> +       |  refname is refs/heads/master|refs/heads/master
> +       |    refname is refs/heads/side|refs/heads/side
> +       |      refname is refs/odd/spot|refs/odd/spot
> +       |refname is refs/tags/double-tag|refs/tags/double-tag
> +       |     refname is refs/tags/four|refs/tags/four
> +       |      refname is refs/tags/one|refs/tags/one
> +       |refname is refs/tags/signed-tag|refs/tags/signed-tag
> +       |    refname is refs/tags/three|refs/tags/three
> +       |      refname is refs/tags/two|refs/tags/two
> +       EOF
> +       git for-each-ref --format="|%(align:width=30,position=middle,right)refname is %(refname)%(end)|%(refname)" >actual &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success 'alignment with multiple "<position>" values' '
> +       cat >expect <<-\EOF &&
> +       | refname is refs/heads/master |refs/heads/master
> +       |  refname is refs/heads/side  |refs/heads/side
> +       |   refname is refs/odd/spot   |refs/odd/spot
> +       |refname is refs/tags/double-tag|refs/tags/double-tag
> +       |  refname is refs/tags/four   |refs/tags/four
> +       |   refname is refs/tags/one   |refs/tags/one
> +       |refname is refs/tags/signed-tag|refs/tags/signed-tag
> +       |  refname is refs/tags/three  |refs/tags/three
> +       |   refname is refs/tags/two   |refs/tags/two
> +       EOF
> +       git for-each-ref --format="|%(align:30,right,position=middle)refname is %(refname)%(end)|%(refname)" >actual &&
> +       test_cmp expect actual
> +'
> +
>  # Individual atoms inside %(align:...) and %(end) must not be quoted.
>
>  test_expect_success 'alignment with format quote' "
> --
> 2.6.4
>