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

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