On Sat, Aug 15, 2015 at 2:00 PM, 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)".
>
> This is done by calling the strbuf_utf8_align() function in utf8.c.
>
> 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 3259363..eac99d0 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -10,6 +10,7 @@
> #include "quote.h"
> #include "ref-filter.h"
> #include "revision.h"
> +#include "utf8.h"
>
> typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
>
> @@ -53,16 +54,27 @@ static struct {
> { "flag" },
> { "HEAD" },
> { "color" },
> + { "align" },
> + { "end" },
> +};
> +
> +struct align {
> + align_type position;
> + unsigned int width;
> };
>
> struct ref_formatting_state {
> struct strbuf output;
> struct ref_formatting_state *prev;
> + void (*attend)(struct ref_formatting_state *state);
Junio's suggestion for this member was "at end"; that is what to do
when you are "at"-the-%(end), not "attend", which isn't meaningful
here. You could also call it 'end_scope', 'finish' or 'close' or
'finalize' or something.
> + void *cb_data;
> int quote_style;
> };
>
> struct atom_value {
> const char *s;
> + struct align *align;
> + void (*handler)(struct atom_value *atomv, struct ref_formatting_state **state);
> unsigned long ul; /* used for sorting when not FIELD_STR */
> };
>
> @@ -137,12 +149,12 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
>
> static struct ref_formatting_state *push_new_state(struct ref_formatting_state **state)
> {
> - struct ref_formatting_state *new_state = xcalloc(1, sizeof(struct ref_formatting_state));
> - struct ref_formatting_state *tmp = *state;
> + struct ref_formatting_state *new = xcalloc(1, sizeof(struct ref_formatting_state));
> + struct ref_formatting_state *old = *state;
>
> - *state = new_state;
> - new_state->prev = tmp;
> - return new_state;
> + *state = new;
> + new->prev = old;
> + return new;
> }
What are these changes about? They appear only to be renaming some
variables which were introduced in patch 3. It would make more sense
to give them the desired names in the patch which introduces them.
> static void pop_state(struct ref_formatting_state **state)
> @@ -625,6 +637,34 @@ static inline char *copy_advance(char *dst, const char *src)
> return dst;
> }
>
> +static void align_handler(struct ref_formatting_state *state)
The names 'align_handler' and 'align_atom_handler' are confusingly
similar. Perhaps name this end_align() or do_align() or
apply_alignment() or something?
> +{
> + struct strbuf aligned = STRBUF_INIT;
> + struct ref_formatting_state *return_to = state->prev;
> + struct align *align = (struct align *)state->cb_data;
> +
> + strbuf_utf8_align(&aligned, align->position, align->width, state->output.buf);
> + strbuf_addbuf(&return_to->output, &aligned);
A couple comments:
First, why is 'strbuf aligned' needed? Can't you instead just invoke
strbuf_utf8_align(&return_to->output, ...)?
Second, I realize that Junio suggested the 'return_to' idea, but it
seems like it could become overly painful since each handler of this
sort is going to have to perform the same manipulation to append its
collected output to its parent state's output. What if you instead
make it the responsibility of pop_state() to append the 'output' from
the state being popped to the "prev" state's 'output'? This way, it
happens automatically, thus reducing code in each individual handler,
and reducing the burden of having to keep writing the same code.
> + strbuf_release(&aligned);
> +}
> +
> +static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_state **state)
> +{
> + struct ref_formatting_state *new = push_new_state(state);
> + strbuf_init(&new->output, 0);
I think this strbuf_init() should be the responsibility of
push_new_state(), as mentioned in my patch 3 review, otherwise every
caller of push_new_state() will have to remember to do this.
> + new->attend = align_handler;
> + new->cb_data = atomv->align;
> +}
> +
> +static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state **state)
> +{
> + struct ref_formatting_state *current = *state;
> + if (!current->attend)
> + die(_("format: `end` atom used without a supporting atom"));
> + current->attend(current);
> + pop_state(state);
> +}
> +
> /*
> * Parse the object referred by ref, and grab needed value.
> */
> @@ -653,6 +693,7 @@ static void populate_value(struct ref_array_item *ref)
> int deref = 0;
> const char *refname;
> const char *formatp;
> + const char *valp;
> struct branch *branch = NULL;
>
> if (*name == '*') {
> @@ -718,6 +759,34 @@ static void populate_value(struct ref_array_item *ref)
> else
> v->s = " ";
> continue;
> + } else if (skip_prefix(name, "align:", &valp)) {
> + struct align *align = xmalloc(sizeof(struct align));
Who is responsible for freeing this memory?
> + char *ep = strchr(valp, ',');
> +
> + if (ep)
> + *ep = '\0';
> +
> + if (strtoul_ui(valp, 10, &align->width))
> + die(_("positive width expected align:%s"), valp);
> +
> + if (!ep || starts_with(ep + 1, "left"))
> + align->position = ALIGN_LEFT;
> + else if (starts_with(ep + 1, "right"))
> + align->position = ALIGN_RIGHT;
> + else if (starts_with(ep + 1, "middle"))
> + align->position = ALIGN_MIDDLE;
Shouldn't these be strcmp() rather than starts_with()? You don't want
to match "leftfoot" as "left".
> + else
> + die(_("improper format entered align:%s"), ep + 1);
> +
> + if (ep)
> + *ep = ',';
What's this conditional about? Why restore the comma?
> +
> + v->align = align;
> + v->handler = align_atom_handler;
Junio's proposal for using handlers[1] is a pretty big change which
would generalize atom processing overall, and which you haven't
implemented here. Instead, your use of handlers is just to avoid
having special-case 'align' and 'end' conditionals spread throughout
several functions. Am I understanding that correctly?
Is the idea to leave that larger change for a later date (and possibly
a different programmer)?
[1]: http://article.gmane.org/gmane.comp.version-control.git/275710
> + continue;
> + } else if (!strcmp(name, "end")) {
> + v->handler = end_atom_handler;
> + continue;
> } else
> continue;
>
> @@ -1296,6 +1365,8 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
> if (cp < sp)
> append_literal(cp, sp, state);
> get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);
> + if (atomv->handler)
> + atomv->handler(atomv, &state);
> append_atom(atomv, state);
> }
> if (*cp) {
> diff --git a/ref-filter.h b/ref-filter.h
> index 45026d0..144a633 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -5,6 +5,7 @@
> #include "refs.h"
> #include "commit.h"
> #include "parse-options.h"
> +#include "utf8.h"
Why does this need to be #included here?
> /* Quoting styles */
> #define QUOTE_NONE 0
--
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