[PATCH v11 03/13] ref-filter: introduce ref_formatting_state

[PATCH v11 03/13] ref-filter: introduce ref_formatting_state

[Karthik Nayak]

Introduce ref_formatting_state which will hold the formatted
output strbuf and is used for nesting of modifier atoms.

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 ref-filter.c | 65 ++++++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 48 insertions(+), 17 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index edfb1c7..3259363 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -55,6 +55,12 @@ static struct {
 	{ "color" },
 };
 
+struct ref_formatting_state {
+	struct strbuf output;
+	struct ref_formatting_state *prev;
+	int quote_style;
+};
+
 struct atom_value {
 	const char *s;
 	unsigned long ul; /* used for sorting when not FIELD_STR */
@@ -129,6 +135,26 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
 	return at;
 }
 
+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;
+
+	*state = new_state;
+	new_state->prev = tmp;
+	return new_state;
+}
+
+static void pop_state(struct ref_formatting_state **state)
+{
+	struct ref_formatting_state *current = *state;
+	struct ref_formatting_state *prev = current->prev;
+
+	strbuf_release(&current->output);
+	free(current);
+	*state = prev;
+}
+
 /*
  * In a format string, find the next occurrence of %(atom).
  */
@@ -1195,23 +1221,23 @@ void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)
 	qsort(array->items, array->nr, sizeof(struct ref_array_item *), compare_refs);
 }
 
-static void append_atom(struct atom_value *v, int quote_style, struct strbuf *output)
+static void append_atom(struct atom_value *v, struct ref_formatting_state *state)
 {
-	switch (quote_style) {
+	switch (state->quote_style) {
 	case QUOTE_NONE:
-		strbuf_addstr(output, v->s);
+		strbuf_addstr(&state->output, v->s);
 		break;
 	case QUOTE_SHELL:
-		sq_quote_buf(output, v->s);
+		sq_quote_buf(&state->output, v->s);
 		break;
 	case QUOTE_PERL:
-		perl_quote_buf(output, v->s);
+		perl_quote_buf(&state->output, v->s);
 		break;
 	case QUOTE_PYTHON:
-		python_quote_buf(output, v->s);
+		python_quote_buf(&state->output, v->s);
 		break;
 	case QUOTE_TCL:
-		tcl_quote_buf(output, v->s);
+		tcl_quote_buf(&state->output, v->s);
 		break;
 	}
 }
@@ -1234,7 +1260,7 @@ static int hex2(const char *cp)
 		return -1;
 }
 
-static void append_literal(const char *cp, const char *ep, struct strbuf *output)
+static void append_literal(const char *cp, const char *ep, struct ref_formatting_state *state)
 {
 	while (*cp && (!ep || cp < ep)) {
 		if (*cp == '%') {
@@ -1243,13 +1269,13 @@ static void append_literal(const char *cp, const char *ep, struct strbuf *output
 			else {
 				int ch = hex2(cp + 1);
 				if (0 <= ch) {
-					strbuf_addch(output, ch);
+					strbuf_addch(&state->output, ch);
 					cp += 3;
 					continue;
 				}
 			}
 		}
-		strbuf_addch(output, *cp);
+		strbuf_addch(&state->output, *cp);
 		cp++;
 	}
 }
@@ -1257,20 +1283,24 @@ static void append_literal(const char *cp, const char *ep, struct strbuf *output
 void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)
 {
 	const char *cp, *sp, *ep;
-	struct strbuf output = STRBUF_INIT;
+	struct strbuf *final_buf;
+	struct ref_formatting_state *state = NULL;
+
+	state = push_new_state(&state);
+	state->quote_style = quote_style;
 
 	for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
 		struct atom_value *atomv;
 
 		ep = strchr(sp, ')');
 		if (cp < sp)
-			append_literal(cp, sp, &output);
+			append_literal(cp, sp, state);
 		get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);
-		append_atom(atomv, quote_style, &output);
+		append_atom(atomv, state);
 	}
 	if (*cp) {
 		sp = cp + strlen(cp);
-		append_literal(cp, sp, &output);
+		append_literal(cp, sp, state);
 	}
 	if (need_color_reset_at_eol) {
 		struct atom_value resetv;
@@ -1279,11 +1309,12 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
 		if (color_parse("reset", color) < 0)
 			die("BUG: couldn't parse 'reset' as a color");
 		resetv.s = color;
-		append_atom(&resetv, quote_style, &output);
+		append_atom(&resetv, state);
 	}
-	fwrite(output.buf, 1, output.len, stdout);
+	final_buf = &state->output;
+	fwrite(final_buf->buf, 1, final_buf->len, stdout);
+	pop_state(&state);
 	putchar('\n');
-	strbuf_release(&output);
 }
 
 /*  If no sorting option is given, use refname to sort as default */
-- 
2.5.0

Re: [PATCH v11 03/13] ref-filter: introduce ref_formatting_state

[Eric Sunshine]

On Sat, Aug 15, 2015 at 2:00 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
> Introduce ref_formatting_state which will hold the formatted
> output strbuf and is used for nesting of modifier atoms.
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> index edfb1c7..3259363 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -55,6 +55,12 @@ static struct {
>         { "color" },
>  };
>
> +struct ref_formatting_state {
> +       struct strbuf output;
> +       struct ref_formatting_state *prev;

Upon initial read-through of this patch, I found the name 'prev'
confusing since it seems you sometimes treat this as a linked-list
and, for a linked-list, this member is customarily named 'next'.
However, you also sometimes treat it as a stack, in which case 'prev'
makes a certain amount of sense semantically, although so does 'next'.
I'd probably have named it 'next', however, attr.c:struct attr_stack
names its member 'prev', so there is some precedence for the current
choice.

Also, it's customary for this member to be the first (or less
frequently last) member in the structure.

> +       int quote_style;
> +};
> +
>  struct atom_value {
>         const char *s;
>         unsigned long ul; /* used for sorting when not FIELD_STR */
> @@ -129,6 +135,26 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
>         return at;
>  }
>
> +static struct ref_formatting_state *push_new_state(struct ref_formatting_state **state)

This interface seems confused. The caller receives the new state as
both the return value of the function and as an out-value of its sole
argument. I'd suggest choosing one or the other.

Which one you choose depends upon how you view the operation and the
data structure. If you view it as linked-list manipulation, then you'd
probably want the new state returned, and accept a pointer argument
(rather than pointer-to-pointer). On the other hand, if you view it as
a stack, then you'd probably want to have it return void and
manipulate the sole argument. For this case, it might be clearer to
name the argument 'stack' rather than 'state'.

> +{
> +       struct ref_formatting_state *new_state = xcalloc(1, sizeof(struct ref_formatting_state));
> +       struct ref_formatting_state *tmp = *state;
> +
> +       *state = new_state;
> +       new_state->prev = tmp;
> +       return new_state;
> +}

A couple issues:

First, you need to initialize the strbuf member of
ref_formatting_state after allocation:

    new_state = xcalloc(1, sizeof(struct ref_formatting_state));
    strbuf_init(&new_state->output, 0);

Second, if you re-order the code slightly, the 'tmp' variable becomes
unnecessary.

Assuming that your intention was to match pop_state() and treat this
opaquely as a stack rather than exposing its linked-list
implementation, I'd have expected the function to look something like
this:

    static void push_new_state(struct ref_formatting_state **stack)
    {
        struct ref_formatting_state *s = xcalloc(...);
        s->next = *stack;
        strbuf_init(&s->output, 0);
        *stack = s;
    }

> +static void pop_state(struct ref_formatting_state **state)
> +{
> +       struct ref_formatting_state *current = *state;
> +       struct ref_formatting_state *prev = current->prev;
> +
> +       strbuf_release(&current->output);
> +       free(current);
> +       *state = prev;
> +}

This interface suggests that you're treating it as an opaque stack, in
which case naming the argument 'stack' might be clearer.

>  /*
>   * In a format string, find the next occurrence of %(atom).
>   */
> @@ -1195,23 +1221,23 @@ void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)
>         qsort(array->items, array->nr, sizeof(struct ref_array_item *), compare_refs);
>  }
>
> -static void append_atom(struct atom_value *v, int quote_style, struct strbuf *output)
> +static void append_atom(struct atom_value *v, struct ref_formatting_state *state)
>  {
> -       switch (quote_style) {
> +       switch (state->quote_style) {
>         case QUOTE_NONE:
> -               strbuf_addstr(output, v->s);
> +               strbuf_addstr(&state->output, v->s);
>                 break;
>         case QUOTE_SHELL:
> -               sq_quote_buf(output, v->s);
> +               sq_quote_buf(&state->output, v->s);
>                 break;
>         case QUOTE_PERL:
> -               perl_quote_buf(output, v->s);
> +               perl_quote_buf(&state->output, v->s);
>                 break;
>         case QUOTE_PYTHON:
> -               python_quote_buf(output, v->s);
> +               python_quote_buf(&state->output, v->s);
>                 break;
>         case QUOTE_TCL:
> -               tcl_quote_buf(output, v->s);
> +               tcl_quote_buf(&state->output, v->s);
>                 break;
>         }
>  }

This patch touches all the same lines as the previous patch which
converted the code to append to a strbuf rather than emit to stdout,
thus it makes the previous patch seem wasted and the current patch
much noisier. As such, it might make sense to repartition these two
patches as so:

patch 2: Introduce ref_formatting_state (but without the 'prev'
field), and update callers to append to its 'output' member rather
than emitting to stdout.

patch 3: Introduce the ref_formatting_state stack machinery; this
patch would also add the 'prev' field.

> @@ -1234,7 +1260,7 @@ static int hex2(const char *cp)
>                 return -1;
>  }
>
> -static void append_literal(const char *cp, const char *ep, struct strbuf *output)
> +static void append_literal(const char *cp, const char *ep, struct ref_formatting_state *state)
>  {
>         while (*cp && (!ep || cp < ep)) {
>                 if (*cp == '%') {
> @@ -1243,13 +1269,13 @@ static void append_literal(const char *cp, const char *ep, struct strbuf *output
>                         else {
>                                 int ch = hex2(cp + 1);
>                                 if (0 <= ch) {
> -                                       strbuf_addch(output, ch);
> +                                       strbuf_addch(&state->output, ch);
>                                         cp += 3;
>                                         continue;
>                                 }
>                         }
>                 }
> -               strbuf_addch(output, *cp);
> +               strbuf_addch(&state->output, *cp);
>                 cp++;
>         }
>  }
> @@ -1257,20 +1283,24 @@ static void append_literal(const char *cp, const char *ep, struct strbuf *output
>  void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)
>  {
>         const char *cp, *sp, *ep;
> -       struct strbuf output = STRBUF_INIT;
> +       struct strbuf *final_buf;
> +       struct ref_formatting_state *state = NULL;
> +
> +       state = push_new_state(&state);
> +       state->quote_style = quote_style;
>
>         for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {
>                 struct atom_value *atomv;
>
>                 ep = strchr(sp, ')');
>                 if (cp < sp)
> -                       append_literal(cp, sp, &output);
> +                       append_literal(cp, sp, state);
>                 get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);
> -               append_atom(atomv, quote_style, &output);
> +               append_atom(atomv, state);
>         }
>         if (*cp) {
>                 sp = cp + strlen(cp);
> -               append_literal(cp, sp, &output);
> +               append_literal(cp, sp, state);
>         }
>         if (need_color_reset_at_eol) {
>                 struct atom_value resetv;
> @@ -1279,11 +1309,12 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
>                 if (color_parse("reset", color) < 0)
>                         die("BUG: couldn't parse 'reset' as a color");
>                 resetv.s = color;
> -               append_atom(&resetv, quote_style, &output);
> +               append_atom(&resetv, state);
>         }
> -       fwrite(output.buf, 1, output.len, stdout);
> +       final_buf = &state->output;
> +       fwrite(final_buf->buf, 1, final_buf->len, stdout);
> +       pop_state(&state);
>         putchar('\n');
> -       strbuf_release(&output);
>  }
>
>  /*  If no sorting option is given, use refname to sort as default */
> --
> 2.5.0

Re: [PATCH v11 03/13] ref-filter: introduce ref_formatting_state

[Eric Sunshine]

On Sun, Aug 16, 2015 at 7:31 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sat, Aug 15, 2015 at 2:00 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> +struct ref_formatting_state {
>> +       struct strbuf output;
>> +       struct ref_formatting_state *prev;
>
> Upon initial read-through of this patch, I found the name 'prev'
> confusing since it seems you sometimes treat this as a linked-list
> and, for a linked-list, this member is customarily named 'next'.
> However, you also sometimes treat it as a stack, in which case 'prev'
> makes a certain amount of sense semantically, although so does 'next'.
> I'd probably have named it 'next', however, attr.c:struct attr_stack
> names its member 'prev', so there is some precedence for the current
> choice.

s/precedence/precedent/

Re: [PATCH v11 03/13] ref-filter: introduce ref_formatting_state

[Karthik Nayak]

On Mon, Aug 17, 2015 at 5:01 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sat, Aug 15, 2015 at 2:00 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Introduce ref_formatting_state which will hold the formatted
>> output strbuf and is used for nesting of modifier atoms.
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> index edfb1c7..3259363 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -55,6 +55,12 @@ static struct {
>>         { "color" },
>>  };
>>
>> +struct ref_formatting_state {
>> +       struct strbuf output;
>> +       struct ref_formatting_state *prev;
>
> Upon initial read-through of this patch, I found the name 'prev'
> confusing since it seems you sometimes treat this as a linked-list
> and, for a linked-list, this member is customarily named 'next'.
> However, you also sometimes treat it as a stack, in which case 'prev'
> makes a certain amount of sense semantically, although so does 'next'.
> I'd probably have named it 'next', however, attr.c:struct attr_stack
> names its member 'prev', so there is some precedence for the current
> choice.

Its sort of a stack in all sense, I mean we get new elements and push it onto
the top and pop them once we're done. So that's why prev, also it's
more of pushing the
current state into the previously defined state.

>
> Also, it's customary for this member to be the first (or less
> frequently last) member in the structure.
>

Ok will do that.

>> +       int quote_style;
>> +};
>> +
>>  struct atom_value {
>>         const char *s;
>>         unsigned long ul; /* used for sorting when not FIELD_STR */
>> @@ -129,6 +135,26 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
>>         return at;
>>  }
>>
>> +static struct ref_formatting_state *push_new_state(struct ref_formatting_state **state)
>
> This interface seems confused. The caller receives the new state as
> both the return value of the function and as an out-value of its sole
> argument. I'd suggest choosing one or the other.
>
> Which one you choose depends upon how you view the operation and the
> data structure. If you view it as linked-list manipulation, then you'd
> probably want the new state returned, and accept a pointer argument
> (rather than pointer-to-pointer). On the other hand, if you view it as
> a stack, then you'd probably want to have it return void and
> manipulate the sole argument. For this case, it might be clearer to
> name the argument 'stack' rather than 'state'.
>

I guess it makes to stick to the stack arrangement, will change the argument
name to stack.

>> +{
>> +       struct ref_formatting_state *new_state = xcalloc(1, sizeof(struct ref_formatting_state));
>> +       struct ref_formatting_state *tmp = *state;
>> +
>> +       *state = new_state;
>> +       new_state->prev = tmp;
>> +       return new_state;
>> +}
>
> A couple issues:
>
> First, you need to initialize the strbuf member of
> ref_formatting_state after allocation:
>
>     new_state = xcalloc(1, sizeof(struct ref_formatting_state));
>     strbuf_init(&new_state->output, 0);
>

Was under the impression that strbuf_init() is not needed after a xcalloc,
had to look at that again.

> Second, if you re-order the code slightly, the 'tmp' variable becomes
> unnecessary.
>
> Assuming that your intention was to match pop_state() and treat this
> opaquely as a stack rather than exposing its linked-list
> implementation, I'd have expected the function to look something like
> this:
>
>     static void push_new_state(struct ref_formatting_state **stack)
>     {
>         struct ref_formatting_state *s = xcalloc(...);
>         s->next = *stack;
>         strbuf_init(&s->output, 0);
>         *stack = s;
>     }
>

You mean:

static void push_new_state(struct ref_formatting_state **stack)
{
    struct ref_formatting_state *s = xcalloc(...);

    strbuf_init(&s->output, 0);
    s->prev = *stack;
    *stack = s;
}

>> +static void pop_state(struct ref_formatting_state **state)
>> +{
>> +       struct ref_formatting_state *current = *state;
>> +       struct ref_formatting_state *prev = current->prev;
>> +
>> +       strbuf_release(&current->output);
>> +       free(current);
>> +       *state = prev;
>> +}
>
> This interface suggests that you're treating it as an opaque stack, in
> which case naming the argument 'stack' might be clearer.
>

Will do thanks.

>>  /*
>>   * In a format string, find the next occurrence of %(atom).
>>   */
>> @@ -1195,23 +1221,23 @@ void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)
>>         qsort(array->items, array->nr, sizeof(struct ref_array_item *), compare_refs);
>>  }
>>
>> -static void append_atom(struct atom_value *v, int quote_style, struct strbuf *output)
>> +static void append_atom(struct atom_value *v, struct ref_formatting_state *state)
>>  {
>> -       switch (quote_style) {
>> +       switch (state->quote_style) {
>>         case QUOTE_NONE:
>> -               strbuf_addstr(output, v->s);
>> +               strbuf_addstr(&state->output, v->s);
>>                 break;
>>         case QUOTE_SHELL:
>> -               sq_quote_buf(output, v->s);
>> +               sq_quote_buf(&state->output, v->s);
>>                 break;
>>         case QUOTE_PERL:
>> -               perl_quote_buf(output, v->s);
>> +               perl_quote_buf(&state->output, v->s);
>>                 break;
>>         case QUOTE_PYTHON:
>> -               python_quote_buf(output, v->s);
>> +               python_quote_buf(&state->output, v->s);
>>                 break;
>>         case QUOTE_TCL:
>> -               tcl_quote_buf(output, v->s);
>> +               tcl_quote_buf(&state->output, v->s);
>>                 break;
>>         }
>>  }
>
> This patch touches all the same lines as the previous patch which
> converted the code to append to a strbuf rather than emit to stdout,
> thus it makes the previous patch seem wasted and the current patch
> much noisier. As such, it might make sense to repartition these two
> patches as so:
>
> patch 2: Introduce ref_formatting_state (but without the 'prev'
> field), and update callers to append to its 'output' member rather
> than emitting to stdout.
>
> patch 3: Introduce the ref_formatting_state stack machinery; this
> patch would also add the 'prev' field.
>

This sounds good, will do this.

--
Regards,
Karthik Nayak