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

Re: [PATCH/RFC 07/10] ref-filter: introduce align_atom_parser()

From
Eric Sunshine <sunshine@sunshineco.com>
Date
Dec 2, 2015, 21:23 UTC
Message-ID
<CAPig+cTYt1qaSLLOmwyZ-BQzOjWGxZ5koJNma1qc67UipPFW5w@mail.gmail.com>
In-Reply-To
<1447271075-15364-8-git-send-email-Karthik.188@gmail.com>
On Wed, Nov 11, 2015 at 2:44 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> Introduce align_atom_parser() which will parse 'align' atoms and store
> the required width and position into the 'used_atom' structure. While
> we're here, add support for the usage of 'width=' and 'position=' when
> using the 'align' atom (e.g. %(align:position=middle,width=30)).

This patch is doing too much by both moving code around and modifying that code (somewhat dramatically), thus it is difficult for reviewers to compare the old and new behaviors. It deserves to be split apart into at least two patches. First, the code movement patch which introduces align_atom_parser() (and possibly get_align_position()) without any behavior or logical change; then the patch which changes behavior to recognize the spelled-out forms "width=" and "position=". You may even want to spilt it into more patches, for instance by doing the get_align_position() extraction in its own patch.

Show 23 quoted lines
> Add documentation and modify the existing tests in t6302 to reflect
> the same.
>
> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> @@ -129,14 +129,16 @@ color::
>  align::
>         Left-, middle-, or right-align the content between
> -       %(align:...) and %(end). The "align:" is followed by `<width>`
> -       and `<position>` in any order separated by a comma, where the
> -       `<position>` is either left, right or middle, default being
> -       left and `<width>` is the total length of the content with
> -       alignment. If the contents length is more than the width then
> -       no alignment is performed. If used with '--quote' everything
> -       in between %(align:...) and %(end) is quoted, but if nested
> -       then only the topmost level performs quoting.
> +       %(align:...) and %(end). The "align:" is followed by
> +       `width=<width>` and `position=<position>` in any order
> +       separated by a comma, where the `<position>` is either left,
> +       right or middle, default being left and `<width>` is the total
> +       length of the content with alignment. The prefix for the
> +       arguments is not mandatory. If the contents length is more

This paragraph is so bulky that it's very easy to overlook the bit about the "prefix for the arguments" being optional, and it's not necessarily even clear to the casual reader what that means. It might, therefore, be a good idea to spell it out explicitly. For instance, you might say something like:

    For brevity, the "width=" and/or "position=" prefixes may be
    omitted, and bare <width> and <position> used instead.
    For instance, `%(align:<width>,<position>)`.
Show 7 quoted lines
> +       than the width then no alignment is performed. If used with
> +       '--quote' everything in between %(align:...) and %(end) is
> +       quoted, but if nested then only the topmost level performs
> +       quoting.
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -63,6 +69,61 @@ void color_atom_parser(struct used_atom *atom)
> +static align_type get_align_position(const char *type)

Taken in context of the callers, this isn't a great function name, as it implies that it is retrieving some value, when in fact it is parsing the input argument. A better name might be parse_align_position().

Likewise, 'type' isn't necessarily a great argument name. You might instead call it 'pos' or even just short and sweet 's'.

Show 20 quoted lines
> +{
> +       if (!strcmp(type, "right"))
> +               return ALIGN_RIGHT;
> +       else if (!strcmp(type, "middle"))
> +               return ALIGN_MIDDLE;
> +       else if (!strcmp(type, "left"))
> +               return ALIGN_LEFT;
> +       return -1;
> +}
> +
> +void align_atom_parser(struct used_atom *atom)
> +{
> +       struct align *align = &atom->u.align;
> +       const char *buf = NULL;
> +       struct strbuf **s, **to_free;
> +       int width = -1;
> +
> +       match_atom_name(atom->str, "align", &buf);
> +       if (!buf)
> +               die(_("expected format: %%(align:<width>,<position>)"));

Is this still the way you want this error message to appear, or should it show the long-form of the arguments? (I don't care strongly.)

> +       s = to_free = strbuf_split_str_without_term(buf, ',', 0);
> +
> +       /*  By default align to ALGIN_LEFT */

What is ALGIN? Regardless of the answer, this comment is not particularly useful since it merely repeats what the code itself already states clearly.

Show 7 quoted lines
> +       align->position = ALIGN_LEFT;
> +
> +       while (*s) {
> +               int position;
> +               buf = s[0]->buf;
> +
> +               position = get_align_position(buf);

Why is this assignment way up here rather than down below in the penultimate 'else' arm where its result is actually being checked? By moving it closer to the point of use, the logic becomes easier to understand.

> +               if (skip_prefix(buf, "position=", &buf)) {
> +                       position = get_align_position(buf);
> +                       if (position == -1)
> +                               die(_("improper format entered align:%s"), s[0]->buf);

At this point, you can give a better error message since you know that you were parsing a "position=" argument. Maybe something like "unrecognized position: %s".

> +                       align->position = position;
> +               } else if (skip_prefix(buf, "width=", &buf)) {
> +                       if (strtoul_ui(buf, 10, (unsigned int *)&width))
> +                               die(_("improper format entered align:%s"), s[0]->buf);
Ditto regarding better error message.
Show 6 quoted lines
> +               } else if (!strtoul_ui(buf, 10, (unsigned int *)&width))
> +                               ;
> +               else if (position != -1)
> +                       align->position = position;
> +               else
> +                       die(_("improper format entered align:%s"), s[0]->buf);

Here too, it would be more user-friendly to say "unrecognized %%(align) argument: %s".

Show 26 quoted lines
> +               s++;
> +       }
> +
> +       if (width < 0)
> +               die(_("positive width expected with the %%(align) atom"));
> +       align->width = width;
> +       strbuf_list_free(to_free);
> +}
> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
> @@ -97,7 +97,7 @@ test_expect_success 'left alignment is default' '
>         refname is refs/tags/three    |refs/tags/three
>         refname is refs/tags/two      |refs/tags/two
>         EOF
> -       git for-each-ref --format="%(align:30)refname is %(refname)%(end)|%(refname)" >actual &&
> +       git for-each-ref --format="%(align:width=30)refname is %(refname)%(end)|%(refname)" >actual &&
>         test_cmp expect actual
>  '
>
> @@ -113,7 +113,7 @@ test_expect_success 'middle alignment' '
>         |  refname is refs/tags/three  |refs/tags/three
>         |   refname is refs/tags/two   |refs/tags/two
>         EOF
> -       git for-each-ref --format="|%(align:middle,30)refname is %(refname)%(end)|%(refname)" >actual &&
> +       git for-each-ref --format="|%(align:position=middle,30)refname is %(refname)%(end)|%(refname)" >actual &&
>         test_cmp expect actual
>  '

While it may sometimes be reasonable to re-purpose existing tests like this, this probably is not one of those cases. Instead, you should be adding new tests to check all the permutations of the new argument handling. For instance:

    %(align:42)
    %(align:middle,42)
    %(align:42,middle)
    %(align:position=middle,42)
    %(align:42,position=middle)
    %(align:middle,width=42)
    %(align:width=42,middle)
    %(align:position=middle,width=42)
    %(align:width=42,position=middle)

And, it wouldn't hurt to test handling of redundant or extra position and width arguments. Should multiple arguments of the same type result in an error, or should "last one wins (sliently)" be the policy? Once you decide upon a policy, add tests to check that that policy works as expected.

In this case, "last one wins (silently)" may be more friendly to script writers, so it might be the better choice. You'd want to add appropriate tests, using the various permutations. For instance:

    %(align:42,width=43)
    %(align:width=43,42)
    %(align:42,position=middle,right)
    %(align:42,right,position=middle)
Previous: Karthik NayakNext: Karthik Nayak
Message 26 of 50 in “ref-filter: use parsing functions”
  1. 00/10 ref-filter: use parsing functionsKarthik Nayak, Nov 11, 2015
  2. 01/10 ref-filter: introduce a parsing function for each atom in valid_atomKarthik Nayak, Nov 11, 2015
  3. Eric SunshineNov 23, 2015
  4. Karthik NayakNov 25, 2015
  5. Eric SunshineNov 25, 2015
  6. Karthik NayakNov 26, 2015
  7. Junio C HamanoDec 11, 2015
  8. Karthik NayakDec 12, 2015
  9. 02/10 ref-filter: introduce struct used_atomKarthik Nayak, Nov 11, 2015
  10. Eric SunshineDec 1, 2015
  11. 03/10 ref-fitler: bump match_atom() name to the topKarthik Nayak, Nov 11, 2015
  12. 04/10 ref-filter: skip deref specifier in match_atom_name()Karthik Nayak, Nov 11, 2015
  13. Eric SunshineDec 1, 2015
  14. Karthik NayakDec 3, 2015
  15. 05/10 ref-filter: introduce color_atom_parser()Karthik Nayak, Nov 11, 2015
  16. Eric SunshineDec 1, 2015
  17. Karthik NayakDec 3, 2015
  18. Eric SunshineDec 13, 2015
  19. Karthik NayakDec 13, 2015
  20. 06/10 strbuf: introduce strbuf_split_str_without_term()Karthik Nayak, Nov 11, 2015
  21. Eric SunshineDec 2, 2015
  22. Karthik NayakDec 3, 2015
  23. Junio C HamanoDec 11, 2015
  24. Karthik NayakDec 12, 2015
  25. 07/10 ref-filter: introduce align_atom_parser()Karthik Nayak, Nov 11, 2015
  26. Eric SunshineDec 2, 2015
  27. Karthik NayakDec 7, 2015
  28. 08/10 ref-filter: introduce remote_ref_atom_parser()Karthik Nayak, Nov 11, 2015
  29. Eric SunshineDec 13, 2015
  30. Karthik NayakDec 13, 2015
  31. Eric SunshineDec 13, 2015
  32. Karthik NayakDec 13, 2015
  33. Eric SunshineDec 13, 2015
  34. Karthik NayakDec 13, 2015
  35. 09/10 ref-filter: introduce contents_atom_parser()Karthik Nayak, Nov 11, 2015
  36. Eric SunshineDec 13, 2015
  37. Eric SunshineDec 13, 2015
  38. Karthik NayakDec 13, 2015
  39. Karthik NayakDec 13, 2015
  40. Jeff KingNov 24, 2015
  41. Karthik NayakNov 25, 2015
  42. 10/10 ref-filter: introduce objectname_atom_parser()Karthik Nayak, Nov 25, 2015
  43. Eric SunshineDec 13, 2015
  44. Karthik NayakDec 13, 2015
  45. Junio C HamanoDec 11, 2015
  46. Eric SunshineDec 13, 2015
  47. Karthik NayakDec 13, 2015
  48. Eric SunshineDec 13, 2015
  49. Karthik NayakDec 16, 2015
  50. Junio C HamanoDec 14, 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.