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

Re: [PATCH v14 04/13] ref-filter: implement an `align` atom

From
Karthik Nayak <karthik.188@gmail.com>
Date
Aug 30, 2015, 13:38 UTC
Message-ID
<CAOLa=ZTHtps2gJb9asSzG_4NOwx_xiA7OuCzjW=BTTFfkaYoQg@mail.gmail.com>
In-Reply-To
<CAPig+cRHRPehkd+9PwOqpXkRUvaJa42zLtCKMEfv2W=ZJUZJzA@mail.gmail.com>
On Sun, Aug 30, 2015 at 8:57 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 34 quoted lines
> On Sat, Aug 29, 2015 at 10:12 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Implement an `align` atom which left-, middle-, or right-aligns the
>> content between %(align:..) and %(end).
>>
>> It is followed by `:<width>,<position>`, where the `<position>` is
>> either left, right or middle and `<width>` is the size of the area
>> into which the content will be placed. If the content between
>> %(align:) and %(end) is more than the width then no alignment is
>> performed. e.g. to align a refname atom to the middle with a total
>> width of 40 we can do: --format="%(align:middle,40)%(refname)%(end)".
>>
>> We now have a `handler()` for each atom_value which will be called
>> when that atom_value is being parsed, and similarly an `at_end`
>> function for each element of the stack which is to be called when the
>> `end` atom is encountered. Using this we implement the `align` atom
>> which aligns the given strbuf by calling `strbuf_utf8_align()` from
>> utf8.c.
>>
>> Extract perform_quote_formatting() from append_atom(). Given a string
>> a quote_value and a strbuf, perform_quote_formatting() formats the
>> string based on the quote_value and stores it into the strbuf.
>>
>> Ensure that quote formatting is performed on the whole of
>> %(align)...%(end) rather than individual atoms. We do this by skipping
>> individual quote formatting for atoms whenever the stack has more than
>> one element, and performing formatting for the entire stack element
>> when the `%(end)` atoms is encountered.
>
> This patch seems to be conflating three distinct changes:
>
> 1. adding %(align:) atom
> 2. extracting quoting logic to a separate function
> 3. quoting top-level %(align:) but not contained atoms
>
I'll extract 2 to a separate commit.
Show 13 quoted lines
> In fact, #3 seems too specially tied to %(align:)...%(end). One might
> expect that the logic for determining when to quote should be
> independent of any particular atom, which suggests that this logic is
> being handled at the wrong level, and that %(align:) shouldn't have to
> know anything about quoting. I'd have thought the quoting logic would
> naturally accompany the introduction of the formatting state stack
> mechanism in patch 2/13, and that that would generically work with all
> atoms, including %(align:) and whatever new ones are added in the
> future.
>
> But, I may not have been following the quoting discussion closely, so
> perhaps these observations are incorrect?  See more below regarding
> %(if:).
You're right, more below.
Show 64 quoted lines
>
>> Add documentation and tests for the same.
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 432cea0..21c8b5f 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -53,6 +54,13 @@ static struct {
>>         { "flag" },
>>         { "HEAD" },
>>         { "color" },
>> +       { "align" },
>> +       { "end" },
>> +};
>> +
>> +struct align {
>> +       align_type position;
>> +       unsigned int width;
>>  };
>>
>>  #define REF_FORMATTING_STATE_INIT  { 0, NULL }
>> @@ -69,6 +79,8 @@ struct ref_formatting_state {
>>
>>  struct atom_value {
>>         const char *s;
>> +       struct align *align;
>
> Why does 'align' need to be heap-allocated rather than just being a
> direct member of 'atom_value'? Does 'align' need to exist beyond the
> lifetime of its 'atom_value'? If not, making it a direct member might
> simplify resource management (no need to free it).
>
>> +       void (*handler)(struct atom_value *atomv, struct ref_formatting_state *state);
>>         unsigned long ul; /* used for sorting when not FIELD_STR */
>>  };
>>
>> @@ -632,6 +644,84 @@ static inline char *copy_advance(char *dst, const char *src)
>> +static void append_atom(struct atom_value *v, struct ref_formatting_state *state)
>> +{
>> +       /*
>> +        * Quote formatting is only done when the stack has a single
>> +        * element. Otherwise quote formatting is done on the
>> +        * element's entire output strbuf when the %(end) atom is
>> +        * encountered.
>> +        */
>> +       if (!state->stack->prev)
>
> With the disclaimer that I wasn't following the quoting discussion
> closely: Is this condition going to be sufficient for all cases, such
> as an %(if:) atom? That is, if you have:
>
>     %(if:notempty)%(bloop)%(then) --option=%(bloop)%(end)
>
> isn't the intention that, %(bloop) within the %(then) section should
> be quoted but not the literal "--option="?
>
> The condition `!state->stack->prev' would be insufficient to handle
> this if %(if:) pushes one or more states onto the stack, no? This
> implies that you might want an explicit flag for enabling/disabling
> quoting rather than relying upon the state of the stack, and that
> individual atom handlers would control that flag.
>
> Or, am I misunderstanding due to not having followed the discussion closely?
Yes this wont be work for what you've said.

To sum up the discussion: We didn't want atoms within the %(align)....%(end) to be quoted separately rather the whole aligned buffer to be quoted at the end.

But this does conflict with %(if)...%(end). So it makes sense to change it to only have checks for %(align) atom being used.

So probably:
static void append_atom(struct atom_value *v, struct
ref_formatting_state *state)
{
    if (state->stack->at_end == align_handler)
        strbuf_addstr(&state->stack->output, v->s);
    else
        quote_formatting(&state->stack->output, v->s, state->quote_style);
}
and
static void end_atom_handler(struct atom_value *atomv, struct
ref_formatting_state *state)
{
    struct ref_formatting_stack *current = state->stack;
    struct strbuf s = STRBUF_INIT;
    if (!current->at_end)
        die(_("format: `end` atom used without a supporting atom"));
    current->at_end(current);
    /*
     * Whenever we have more than one stack element that means we
     * are using a align modifier atom. In that case we need to
     * perform quote formatting.
     */
    if (current->at_end == align_handler) {
        quote_formatting(&s, current->output.buf, state->quote_style);
        strbuf_reset(&current->output);
        strbuf_addbuf(&current->output, &s);
    }
    strbuf_release(&s);
    pop_stack_element(&state->stack);
}
Show 26 quoted lines
>
>> +               quote_formatting(&state->stack->output, v->s, state->quote_style);
>> +       else
>> +               strbuf_addstr(&state->stack->output, v->s);
>> +}
>> +
>> +static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)
>> +{
>> +       struct ref_formatting_stack *current = state->stack;
>> +       struct strbuf s = STRBUF_INIT;
>> +
>> +       if (!current->at_end)
>> +               die(_("format: `end` atom used without a supporting atom"));
>> +       current->at_end(current);
>> +       /*
>> +        * Whenever we have more than one stack element that means we
>> +        * are using a certain modifier atom. In that case we need to
>> +        * perform quote formatting.
>> +        */
>> +       if (state->stack->prev) {
>> +               quote_formatting(&s, current->output.buf, state->quote_style);
>> +               strbuf_reset(&current->output);
>> +               strbuf_addbuf(&current->output, &s);
>
> strbuf_swap() can replace the above two lines.
>
Yes, this makes sense.
Show 19 quoted lines
>> +       }
>> +       strbuf_release(&s);
>> +       pop_stack_element(&state->stack);
>> +}
>> +
>>  /*
>>   * Parse the object referred by ref, and grab needed value.
>>   */
>> @@ -725,6 +818,37 @@ static void populate_value(struct ref_array_item *ref)
>>                         else
>>                                 v->s = " ";
>>                         continue;
>> +               } else if (!strcmp(name, "align"))
>> +                       die(_("format: incomplete use of the `align` atom"));
>
> Why does %(align) get flagged as a malformation of %(align:), whereas
> %(color) does not get flagged as a malformation of %(color:)? Why does
> one deserve special treatment but not the other?
>
Didn't see that, I think its needed to add a check for both like :
else if (!strcmp(name, "align") || !strcmp(name, "color"))
            die(_("format: improper usage of %s atom"), name);

I had a look if any other atoms need a subvalue to operate, couldn't find any.

Show 12 quoted lines
>> +               else if (skip_prefix(name, "align:", &valp)) {
>> +                       struct align *align = xmalloc(sizeof(struct align));
>> +                       struct strbuf **s = strbuf_split_str(valp, ',', 0);
>> +
>> +                       /* If the position is given trim the ',' from the first strbuf */
>> +                       if (s[1])
>> +                               strbuf_remove(s[0], s[0]->len - 1, 1);
>
> This is a truncation operation, which may be more idiomatically stated as:
>
>     strbuf_setlen(s[0], s[0]->len - 1);
>
Will do.
Show 21 quoted lines
>> +
>> +                       if (strtoul_ui(s[0]->buf, 10, &align->width))
>> +                               die(_("positive width expected align:%s"), s[0]->buf);
>> +
>> +                       /* If no position is given, default to ALIGN_LEFT */
>> +                       if (!s[1] || !strcmp(s[1]->buf, "left"))
>> +                               align->position = ALIGN_LEFT;
>
> If you structured the code like this:
>
>     if (!s[1])
>         align->position = ALIGN_LEFT;
>     else if (!strcmp(s[1]->buf, "left"))
>         align->position = ALIGN_LEFT;
>     else if ...
>
> then the comment about "default" would become unnecessary, and it
> would be easier to change the default in the future (if the need ever
> arose), however, this is a very minor point, and I don't care
> strongly.
>
Since there are other changes to be made, ill add this along.
Show 46 quoted lines
>> +                       else if (!strcmp(s[1]->buf, "right"))
>> +                               align->position = ALIGN_RIGHT;
>> +                       else if (!strcmp(s[1]->buf, "middle"))
>> +                               align->position = ALIGN_MIDDLE;
>> +                       else
>> +                               die(_("improper format entered align:%s"), s[1]->buf);
>> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
>> index 505a360..cef7a41 100755
>> --- a/t/t6302-for-each-ref-filter.sh
>> +++ b/t/t6302-for-each-ref-filter.sh
>> @@ -81,4 +81,89 @@ test_expect_success 'filtering with --contains' '
>> +# Everything in between the %(align)...%(end) atom must be quoted, hence we test this by
>> +# introducing single quote's in %(align)...%(end), which must not be escaped.
>> +
>> +sq="'"
>> +
>> +test_expect_success 'alignment with format quote' '
>> +       cat >expect <<-EOF &&
>> +       refname is ${sq}           ${sq}\\${sq}${sq}master${sq}\\${sq}${sq}           ${sq}|
>> +       refname is ${sq}            ${sq}\\${sq}${sq}side${sq}\\${sq}${sq}            ${sq}|
>> +       refname is ${sq}          ${sq}\\${sq}${sq}odd/spot${sq}\\${sq}${sq}          ${sq}|
>> +       refname is ${sq}         ${sq}\\${sq}${sq}double-tag${sq}\\${sq}${sq}         ${sq}|
>> +       refname is ${sq}            ${sq}\\${sq}${sq}four${sq}\\${sq}${sq}            ${sq}|
>> +       refname is ${sq}            ${sq}\\${sq}${sq}one${sq}\\${sq}${sq}             ${sq}|
>> +       refname is ${sq}         ${sq}\\${sq}${sq}signed-tag${sq}\\${sq}${sq}         ${sq}|
>> +       refname is ${sq}           ${sq}\\${sq}${sq}three${sq}\\${sq}${sq}            ${sq}|
>> +       refname is ${sq}            ${sq}\\${sq}${sq}two${sq}\\${sq}${sq}             ${sq}|
>> +       EOF
>> +       git for-each-ref --shell --format="refname is %(align:30,middle)${sq}%(refname:short)${sq}%(end)|" >actual &&
>
> This can be much less noisy if you change the second argument of
> test_expect_success to use double-quote rather than single-quote. That
> would allow you to use single-quote directly in the test and expected
> output rather than having to use ${sq} indirection.
>
>     test_expect_success 'alignment with format quote' "
>         cat >expect <<-\EOF &&
>         refname is '           '''master'''           '|
>         ...
>         EOF
>         git for-each-ref --shell --format='refname ...
> \\'%(rename:short)\\'...|'
>     "
>
> or something.
>
Will check it out, thanks for the review.
-- 
Regards,
Karthik Nayak
Previous: Eric SunshineNext: Eric Sunshine
Message 9 of 52 in “Port tag.c to use ref-filter.c”
  1. 00/13 Port tag.c to use ref-filter.cKarthik Nayak, Aug 29, 2015
  2. 01/13 ref-filter: move `struct atom_value` to ref-filter.cKarthik Nayak, Aug 29, 2015
  3. 02/13 ref-filter: introduce ref_formatting_state and ref_formatting_stackKarthik Nayak, Aug 29, 2015
  4. 03/13 utf8: add function to align a string into given strbufKarthik Nayak, Aug 29, 2015
  5. Torsten BögershausenAug 29, 2015
  6. Karthik NayakAug 29, 2015
  7. 04/13 ref-filter: implement an `align` atomKarthik Nayak, Aug 29, 2015
  8. Eric SunshineAug 30, 2015
  9. Karthik NayakAug 30, 2015
  10. Eric SunshineAug 30, 2015
  11. Karthik NayakAug 31, 2015
  12. Eric SunshineAug 31, 2015
  13. Matthieu MoyAug 31, 2015
  14. Eric SunshineAug 31, 2015
  15. Karthik NayakSep 1, 2015
  16. Matthieu MoySep 1, 2015
  17. Karthik NayakSep 1, 2015
  18. Karthik NayakAug 30, 2015
  19. Eric SunshineAug 30, 2015
  20. Karthik NayakAug 31, 2015
  21. Junio C HamanoAug 30, 2015
  22. Eric SunshineAug 30, 2015
  23. Karthik NayakAug 31, 2015
  24. Karthik NayakAug 31, 2015
  25. Matthieu MoyAug 31, 2015
  26. Karthik NayakAug 31, 2015
  27. 05/13 ref-filter: add option to filter out tags, branches and remotesKarthik Nayak, Aug 29, 2015
  28. Eric SunshineAug 30, 2015
  29. Karthik NayakAug 30, 2015
  30. Eric SunshineAug 30, 2015
  31. 06/13 ref-filter: introduce format_ref_array_item()Karthik Nayak, Aug 29, 2015
  32. Eric SunshineAug 30, 2015
  33. Karthik NayakAug 30, 2015
  34. Karthik NayakAug 30, 2015
  35. 07/13 ref-filter: add support for %(contents:lines=X)Karthik Nayak, Aug 29, 2015
  36. Eric SunshineAug 30, 2015
  37. Karthik NayakAug 30, 2015
  38. Eric SunshineAug 30, 2015
  39. Karthik NayakAug 30, 2015
  40. Eric SunshineAug 30, 2015
  41. Karthik NayakAug 31, 2015
  42. 08/13 ref-filter: add support to sort by versionKarthik Nayak, Aug 29, 2015
  43. 09/13 ref-filter: add option to match literal patternKarthik Nayak, Aug 29, 2015
  44. 10/13 tag.c: use 'ref-filter' data structuresKarthik Nayak, Aug 29, 2015
  45. 11/13 tag.c: use 'ref-filter' APIsKarthik Nayak, Aug 29, 2015
  46. 12/13 tag.c: implement '--format' optionKarthik Nayak, Aug 29, 2015
  47. 13/13 tag.c: implement '--merged' and '--no-merged' optionsKarthik Nayak, Aug 29, 2015
  48. Matthieu MoyAug 31, 2015
  49. Karthik NayakAug 31, 2015
  50. Matthieu MoyAug 31, 2015
  51. Karthik NayakAug 31, 2015
  52. Karthik NayakSep 1, 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.