git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Eric Sunshine <sunshine@sunshineco.com>
Date
Dec 17, 2015, 08:59 UTC
Message-ID
<CAPig+cRmoOoSscuLcVHWkpH4uVU2Gei791qqn0GjyZ4b9fA45w@mail.gmail.com>
In-Reply-To
<1450279802-29414-9-git-send-email-Karthik.188@gmail.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.
Show 13 quoted lines
> 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.

Show 12 quoted lines
> +                               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...
Show 36 quoted lines
> 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
Show 137 quoted lines
> +
> +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
>
Previous: Karthik NayakNext: Karthik Nayak
Message 22 of 34 in “ref-filter: use parsing functions”
  1. 00/11 ref-filter: use parsing functionsKarthik Nayak, Dec 16, 2015
  2. 01/11 strbuf: introduce strbuf_split_str_without_term()Karthik Nayak, Dec 16, 2015
  3. Eric SunshineDec 16, 2015
  4. Karthik NayakDec 17, 2015
  5. 02/11 ref-filter: use strbuf_split_str_omit_term()Karthik Nayak, Dec 16, 2015
  6. 03/11 ref-filter: introduce struct used_atomKarthik Nayak, Dec 16, 2015
  7. Eric SunshineDec 16, 2015
  8. Karthik NayakDec 19, 2015
  9. 04/11 ref-fitler: bump match_atom() name to the topKarthik Nayak, Dec 16, 2015
  10. 05/11 ref-filter: skip deref specifier in match_atom_name()Karthik Nayak, Dec 16, 2015
  11. Eric SunshineDec 16, 2015
  12. Karthik NayakDec 19, 2015
  13. 06/11 ref-filter: introduce color_atom_parser()Karthik Nayak, Dec 16, 2015
  14. Eric SunshineDec 16, 2015
  15. Karthik NayakDec 19, 2015
  16. 07/11 ref-filter: introduce align_atom_parser()Karthik Nayak, Dec 16, 2015
  17. Eric SunshineDec 16, 2015
  18. Karthik NayakDec 19, 2015
  19. Karthik NayakDec 19, 2015
  20. Eric SunshineDec 18, 2015
  21. 08/11 ref-filter: introduce prefixes for the align atomKarthik Nayak, Dec 16, 2015
  22. Eric SunshineDec 17, 2015
  23. Karthik NayakDec 31, 2015
  24. 09/11 ref-filter: introduce remote_ref_atom_parser()Karthik Nayak, Dec 16, 2015
  25. Eric SunshineDec 17, 2015
  26. Karthik NayakDec 24, 2015
  27. 10/11 ref-filter: introduce contents_atom_parser()Karthik Nayak, Dec 16, 2015
  28. Eric SunshineDec 17, 2015
  29. Karthik NayakDec 24, 2015
  30. 11/11 ref-filter: introduce objectname_atom_parser()Karthik Nayak, Dec 16, 2015
  31. Eric SunshineDec 18, 2015
  32. Karthik NayakDec 25, 2015
  33. Eric SunshineDec 25, 2015
  34. Karthik NayakDec 25, 2015

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.