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

On Thu, Dec 17, 2015 at 2:29 PM, Eric Sunshine [off-list ref] wrote:

On Wed, Dec 16, 2015 at 10:29 AM, Karthik Nayak [off-list ref] wrote:

quoted

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.

Makes sense, thanks.

quoted

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

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.

This makes sense from a reviewers perspective, will do.

quoted

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

I think Junio also mentioned this once. Thanks for reminding.

quoted

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

Thanks.

More below...

quoted

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

This seems like a good idea, I implemented both of those together.

test_align_permutations() {
while read -r option; do
test_expect_success 'align permutations' '
git for-each-ref --format="|%(align:$option)refname is
%(refname)%(end)|%(refname)" >actual &&
test_cmp expect actual
'
done;
}

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

# Last one wins (silently) when multiple arguments of the same type are given

test_align_permutations <<-\EOF
32,width=42,middle
width=30,42,middle
width=42,position=right,middle
42,right,position=middle
EOF

Thanks :)

--
Regards,
Karthik Nayak