On Sat, Aug 29, 2015 at 10:12 AM, Karthik Nayak <karthik.188@xxxxxxxxx> 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
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:).
> Add documentation and tests for the same.
>
> Signed-off-by: Karthik Nayak <karthik.188@xxxxxxxxx>
> ---
> 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?
> + 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(¤t->output);
> + strbuf_addbuf(¤t->output, &s);
strbuf_swap() can replace the above two 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?
> + 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);
> +
> + 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.
> + 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.
> + test_cmp expect actual
> +'
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html