{"thread":{"id":"40096","subject":"[PATCH v11 03/13] ref-filter: introduce ref_formatting_state","startedAt":"2015-08-15T18:00:33Z","lastAt":"2015-08-17T12:43:16Z","messageCount":4,"participants":["Karthik Nayak","Eric Sunshine"],"isPatch":true,"patchVersion":11,"patchTotal":13},"messages":[{"id":"268127","messageId":"1439661643-16094-4-git-send-email-Karthik.188@gmail.com","threadId":"40096","inReplyTo":null,"subject":"[PATCH v11 03/13] ref-filter: introduce ref_formatting_state","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-15T18:00:33Z","receivedAt":"2015-08-15T18:00:33Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Introduce ref_formatting_state which will hold the formatted\noutput strbuf and is used for nesting of modifier atoms.\n\nMentored-by: Christian Couder <christian.couder@gmail.com>\nMentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n ref-filter.c | 65 ++++++++++++++++++++++++++++++++++++++++++++----------------\n 1 file changed, 48 insertions(+), 17 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex edfb1c7..3259363 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -55,6 +55,12 @@ static struct {\n \t{ \"color\" },\n };\n \n+struct ref_formatting_state {\n+\tstruct strbuf output;\n+\tstruct ref_formatting_state *prev;\n+\tint quote_style;\n+};\n+\n struct atom_value {\n \tconst char *s;\n \tunsigned long ul; /* used for sorting when not FIELD_STR */\n@@ -129,6 +135,26 @@ int parse_ref_filter_atom(const char *atom, const char *ep)\n \treturn at;\n }\n \n+static struct ref_formatting_state *push_new_state(struct ref_formatting_state **state)\n+{\n+\tstruct ref_formatting_state *new_state = xcalloc(1, sizeof(struct ref_formatting_state));\n+\tstruct ref_formatting_state *tmp = *state;\n+\n+\t*state = new_state;\n+\tnew_state->prev = tmp;\n+\treturn new_state;\n+}\n+\n+static void pop_state(struct ref_formatting_state **state)\n+{\n+\tstruct ref_formatting_state *current = *state;\n+\tstruct ref_formatting_state *prev = current->prev;\n+\n+\tstrbuf_release(&current->output);\n+\tfree(current);\n+\t*state = prev;\n+}\n+\n /*\n  * In a format string, find the next occurrence of %(atom).\n  */\n@@ -1195,23 +1221,23 @@ void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)\n \tqsort(array->items, array->nr, sizeof(struct ref_array_item *), compare_refs);\n }\n \n-static void append_atom(struct atom_value *v, int quote_style, struct strbuf *output)\n+static void append_atom(struct atom_value *v, struct ref_formatting_state *state)\n {\n-\tswitch (quote_style) {\n+\tswitch (state->quote_style) {\n \tcase QUOTE_NONE:\n-\t\tstrbuf_addstr(output, v->s);\n+\t\tstrbuf_addstr(&state->output, v->s);\n \t\tbreak;\n \tcase QUOTE_SHELL:\n-\t\tsq_quote_buf(output, v->s);\n+\t\tsq_quote_buf(&state->output, v->s);\n \t\tbreak;\n \tcase QUOTE_PERL:\n-\t\tperl_quote_buf(output, v->s);\n+\t\tperl_quote_buf(&state->output, v->s);\n \t\tbreak;\n \tcase QUOTE_PYTHON:\n-\t\tpython_quote_buf(output, v->s);\n+\t\tpython_quote_buf(&state->output, v->s);\n \t\tbreak;\n \tcase QUOTE_TCL:\n-\t\ttcl_quote_buf(output, v->s);\n+\t\ttcl_quote_buf(&state->output, v->s);\n \t\tbreak;\n \t}\n }\n@@ -1234,7 +1260,7 @@ static int hex2(const char *cp)\n \t\treturn -1;\n }\n \n-static void append_literal(const char *cp, const char *ep, struct strbuf *output)\n+static void append_literal(const char *cp, const char *ep, struct ref_formatting_state *state)\n {\n \twhile (*cp && (!ep || cp < ep)) {\n \t\tif (*cp == '%') {\n@@ -1243,13 +1269,13 @@ static void append_literal(const char *cp, const char *ep, struct strbuf *output\n \t\t\telse {\n \t\t\t\tint ch = hex2(cp + 1);\n \t\t\t\tif (0 <= ch) {\n-\t\t\t\t\tstrbuf_addch(output, ch);\n+\t\t\t\t\tstrbuf_addch(&state->output, ch);\n \t\t\t\t\tcp += 3;\n \t\t\t\t\tcontinue;\n \t\t\t\t}\n \t\t\t}\n \t\t}\n-\t\tstrbuf_addch(output, *cp);\n+\t\tstrbuf_addch(&state->output, *cp);\n \t\tcp++;\n \t}\n }\n@@ -1257,20 +1283,24 @@ static void append_literal(const char *cp, const char *ep, struct strbuf *output\n void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)\n {\n \tconst char *cp, *sp, *ep;\n-\tstruct strbuf output = STRBUF_INIT;\n+\tstruct strbuf *final_buf;\n+\tstruct ref_formatting_state *state = NULL;\n+\n+\tstate = push_new_state(&state);\n+\tstate->quote_style = quote_style;\n \n \tfor (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {\n \t\tstruct atom_value *atomv;\n \n \t\tep = strchr(sp, ')');\n \t\tif (cp < sp)\n-\t\t\tappend_literal(cp, sp, &output);\n+\t\t\tappend_literal(cp, sp, state);\n \t\tget_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);\n-\t\tappend_atom(atomv, quote_style, &output);\n+\t\tappend_atom(atomv, state);\n \t}\n \tif (*cp) {\n \t\tsp = cp + strlen(cp);\n-\t\tappend_literal(cp, sp, &output);\n+\t\tappend_literal(cp, sp, state);\n \t}\n \tif (need_color_reset_at_eol) {\n \t\tstruct atom_value resetv;\n@@ -1279,11 +1309,12 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu\n \t\tif (color_parse(\"reset\", color) < 0)\n \t\t\tdie(\"BUG: couldn't parse 'reset' as a color\");\n \t\tresetv.s = color;\n-\t\tappend_atom(&resetv, quote_style, &output);\n+\t\tappend_atom(&resetv, state);\n \t}\n-\tfwrite(output.buf, 1, output.len, stdout);\n+\tfinal_buf = &state->output;\n+\tfwrite(final_buf->buf, 1, final_buf->len, stdout);\n+\tpop_state(&state);\n \tputchar('\\n');\n-\tstrbuf_release(&output);\n }\n \n /*  If no sorting option is given, use refname to sort as default */\n-- \n2.5.0\n"},{"id":"268156","messageId":"CAPig+cRmFCpbJVzrAYqPRcJVKD7czfi7Mni_BzaoB2RkJnFJMg@mail.gmail.com","threadId":"40096","inReplyTo":"1439661643-16094-4-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v11 03/13] ref-filter: introduce ref_formatting_state","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-16T23:31:22Z","receivedAt":"2015-08-16T23:31:22Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Aug 15, 2015 at 2:00 PM, Karthik Nayak <karthik.188@gmail.com> wrote:\n> Introduce ref_formatting_state which will hold the formatted\n> output strbuf and is used for nesting of modifier atoms.\n>\n> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>\n> ---\n> diff --git a/ref-filter.c b/ref-filter.c\n> index edfb1c7..3259363 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -55,6 +55,12 @@ static struct {\n>         { \"color\" },\n>  };\n>\n> +struct ref_formatting_state {\n> +       struct strbuf output;\n> +       struct ref_formatting_state *prev;\n\nUpon initial read-through of this patch, I found the name 'prev'\nconfusing since it seems you sometimes treat this as a linked-list\nand, for a linked-list, this member is customarily named 'next'.\nHowever, you also sometimes treat it as a stack, in which case 'prev'\nmakes a certain amount of sense semantically, although so does 'next'.\nI'd probably have named it 'next', however, attr.c:struct attr_stack\nnames its member 'prev', so there is some precedence for the current\nchoice.\n\nAlso, it's customary for this member to be the first (or less\nfrequently last) member in the structure.\n\n> +       int quote_style;\n> +};\n> +\n>  struct atom_value {\n>         const char *s;\n>         unsigned long ul; /* used for sorting when not FIELD_STR */\n> @@ -129,6 +135,26 @@ int parse_ref_filter_atom(const char *atom, const char *ep)\n>         return at;\n>  }\n>\n> +static struct ref_formatting_state *push_new_state(struct ref_formatting_state **state)\n\nThis interface seems confused. The caller receives the new state as\nboth the return value of the function and as an out-value of its sole\nargument. I'd suggest choosing one or the other.\n\nWhich one you choose depends upon how you view the operation and the\ndata structure. If you view it as linked-list manipulation, then you'd\nprobably want the new state returned, and accept a pointer argument\n(rather than pointer-to-pointer). On the other hand, if you view it as\na stack, then you'd probably want to have it return void and\nmanipulate the sole argument. For this case, it might be clearer to\nname the argument 'stack' rather than 'state'.\n\n> +{\n> +       struct ref_formatting_state *new_state = xcalloc(1, sizeof(struct ref_formatting_state));\n> +       struct ref_formatting_state *tmp = *state;\n> +\n> +       *state = new_state;\n> +       new_state->prev = tmp;\n> +       return new_state;\n> +}\n\nA couple issues:\n\nFirst, you need to initialize the strbuf member of\nref_formatting_state after allocation:\n\n    new_state = xcalloc(1, sizeof(struct ref_formatting_state));\n    strbuf_init(&new_state->output, 0);\n\nSecond, if you re-order the code slightly, the 'tmp' variable becomes\nunnecessary.\n\nAssuming that your intention was to match pop_state() and treat this\nopaquely as a stack rather than exposing its linked-list\nimplementation, I'd have expected the function to look something like\nthis:\n\n    static void push_new_state(struct ref_formatting_state **stack)\n    {\n        struct ref_formatting_state *s = xcalloc(...);\n        s->next = *stack;\n        strbuf_init(&s->output, 0);\n        *stack = s;\n    }\n\n> +static void pop_state(struct ref_formatting_state **state)\n> +{\n> +       struct ref_formatting_state *current = *state;\n> +       struct ref_formatting_state *prev = current->prev;\n> +\n> +       strbuf_release(&current->output);\n> +       free(current);\n> +       *state = prev;\n> +}\n\nThis interface suggests that you're treating it as an opaque stack, in\nwhich case naming the argument 'stack' might be clearer.\n\n>  /*\n>   * In a format string, find the next occurrence of %(atom).\n>   */\n> @@ -1195,23 +1221,23 @@ void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)\n>         qsort(array->items, array->nr, sizeof(struct ref_array_item *), compare_refs);\n>  }\n>\n> -static void append_atom(struct atom_value *v, int quote_style, struct strbuf *output)\n> +static void append_atom(struct atom_value *v, struct ref_formatting_state *state)\n>  {\n> -       switch (quote_style) {\n> +       switch (state->quote_style) {\n>         case QUOTE_NONE:\n> -               strbuf_addstr(output, v->s);\n> +               strbuf_addstr(&state->output, v->s);\n>                 break;\n>         case QUOTE_SHELL:\n> -               sq_quote_buf(output, v->s);\n> +               sq_quote_buf(&state->output, v->s);\n>                 break;\n>         case QUOTE_PERL:\n> -               perl_quote_buf(output, v->s);\n> +               perl_quote_buf(&state->output, v->s);\n>                 break;\n>         case QUOTE_PYTHON:\n> -               python_quote_buf(output, v->s);\n> +               python_quote_buf(&state->output, v->s);\n>                 break;\n>         case QUOTE_TCL:\n> -               tcl_quote_buf(output, v->s);\n> +               tcl_quote_buf(&state->output, v->s);\n>                 break;\n>         }\n>  }\n\nThis patch touches all the same lines as the previous patch which\nconverted the code to append to a strbuf rather than emit to stdout,\nthus it makes the previous patch seem wasted and the current patch\nmuch noisier. As such, it might make sense to repartition these two\npatches as so:\n\npatch 2: Introduce ref_formatting_state (but without the 'prev'\nfield), and update callers to append to its 'output' member rather\nthan emitting to stdout.\n\npatch 3: Introduce the ref_formatting_state stack machinery; this\npatch would also add the 'prev' field.\n\n> @@ -1234,7 +1260,7 @@ static int hex2(const char *cp)\n>                 return -1;\n>  }\n>\n> -static void append_literal(const char *cp, const char *ep, struct strbuf *output)\n> +static void append_literal(const char *cp, const char *ep, struct ref_formatting_state *state)\n>  {\n>         while (*cp && (!ep || cp < ep)) {\n>                 if (*cp == '%') {\n> @@ -1243,13 +1269,13 @@ static void append_literal(const char *cp, const char *ep, struct strbuf *output\n>                         else {\n>                                 int ch = hex2(cp + 1);\n>                                 if (0 <= ch) {\n> -                                       strbuf_addch(output, ch);\n> +                                       strbuf_addch(&state->output, ch);\n>                                         cp += 3;\n>                                         continue;\n>                                 }\n>                         }\n>                 }\n> -               strbuf_addch(output, *cp);\n> +               strbuf_addch(&state->output, *cp);\n>                 cp++;\n>         }\n>  }\n> @@ -1257,20 +1283,24 @@ static void append_literal(const char *cp, const char *ep, struct strbuf *output\n>  void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)\n>  {\n>         const char *cp, *sp, *ep;\n> -       struct strbuf output = STRBUF_INIT;\n> +       struct strbuf *final_buf;\n> +       struct ref_formatting_state *state = NULL;\n> +\n> +       state = push_new_state(&state);\n> +       state->quote_style = quote_style;\n>\n>         for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {\n>                 struct atom_value *atomv;\n>\n>                 ep = strchr(sp, ')');\n>                 if (cp < sp)\n> -                       append_literal(cp, sp, &output);\n> +                       append_literal(cp, sp, state);\n>                 get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);\n> -               append_atom(atomv, quote_style, &output);\n> +               append_atom(atomv, state);\n>         }\n>         if (*cp) {\n>                 sp = cp + strlen(cp);\n> -               append_literal(cp, sp, &output);\n> +               append_literal(cp, sp, state);\n>         }\n>         if (need_color_reset_at_eol) {\n>                 struct atom_value resetv;\n> @@ -1279,11 +1309,12 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu\n>                 if (color_parse(\"reset\", color) < 0)\n>                         die(\"BUG: couldn't parse 'reset' as a color\");\n>                 resetv.s = color;\n> -               append_atom(&resetv, quote_style, &output);\n> +               append_atom(&resetv, state);\n>         }\n> -       fwrite(output.buf, 1, output.len, stdout);\n> +       final_buf = &state->output;\n> +       fwrite(final_buf->buf, 1, final_buf->len, stdout);\n> +       pop_state(&state);\n>         putchar('\\n');\n> -       strbuf_release(&output);\n>  }\n>\n>  /*  If no sorting option is given, use refname to sort as default */\n> --\n> 2.5.0\n"},{"id":"268165","messageId":"CAPig+cSwZaNbUDG+46LfAGB4GjDm5sbkroO_mR+hvCcVHq00cg@mail.gmail.com","threadId":"40096","inReplyTo":"CAPig+cRmFCpbJVzrAYqPRcJVKD7czfi7Mni_BzaoB2RkJnFJMg@mail.gmail.com","subject":"Re: [PATCH v11 03/13] ref-filter: introduce ref_formatting_state","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-17T05:15:20Z","receivedAt":"2015-08-17T05:15:20Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Aug 16, 2015 at 7:31 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Sat, Aug 15, 2015 at 2:00 PM, Karthik Nayak <karthik.188@gmail.com> wrote:\n>> +struct ref_formatting_state {\n>> +       struct strbuf output;\n>> +       struct ref_formatting_state *prev;\n>\n> Upon initial read-through of this patch, I found the name 'prev'\n> confusing since it seems you sometimes treat this as a linked-list\n> and, for a linked-list, this member is customarily named 'next'.\n> However, you also sometimes treat it as a stack, in which case 'prev'\n> makes a certain amount of sense semantically, although so does 'next'.\n> I'd probably have named it 'next', however, attr.c:struct attr_stack\n> names its member 'prev', so there is some precedence for the current\n> choice.\n\ns/precedence/precedent/\n"},{"id":"268180","messageId":"CAOLa=ZTZEVaBCvxS0TekJGQq3pmi+-KCwL_QrTdpTqhrtBKdrg@mail.gmail.com","threadId":"40096","inReplyTo":"CAPig+cRmFCpbJVzrAYqPRcJVKD7czfi7Mni_BzaoB2RkJnFJMg@mail.gmail.com","subject":"Re: [PATCH v11 03/13] ref-filter: introduce ref_formatting_state","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-17T12:43:16Z","receivedAt":"2015-08-17T12:43:16Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Mon, Aug 17, 2015 at 5:01 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Sat, Aug 15, 2015 at 2:00 PM, Karthik Nayak <karthik.188@gmail.com> wrote:\n>> Introduce ref_formatting_state which will hold the formatted\n>> output strbuf and is used for nesting of modifier atoms.\n>>\n>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>\n>> ---\n>> diff --git a/ref-filter.c b/ref-filter.c\n>> index edfb1c7..3259363 100644\n>> --- a/ref-filter.c\n>> +++ b/ref-filter.c\n>> @@ -55,6 +55,12 @@ static struct {\n>>         { \"color\" },\n>>  };\n>>\n>> +struct ref_formatting_state {\n>> +       struct strbuf output;\n>> +       struct ref_formatting_state *prev;\n>\n> Upon initial read-through of this patch, I found the name 'prev'\n> confusing since it seems you sometimes treat this as a linked-list\n> and, for a linked-list, this member is customarily named 'next'.\n> However, you also sometimes treat it as a stack, in which case 'prev'\n> makes a certain amount of sense semantically, although so does 'next'.\n> I'd probably have named it 'next', however, attr.c:struct attr_stack\n> names its member 'prev', so there is some precedence for the current\n> choice.\n\nIts sort of a stack in all sense, I mean we get new elements and push it onto\nthe top and pop them once we're done. So that's why prev, also it's\nmore of pushing the\ncurrent state into the previously defined state.\n\n>\n> Also, it's customary for this member to be the first (or less\n> frequently last) member in the structure.\n>\n\nOk will do that.\n\n>> +       int quote_style;\n>> +};\n>> +\n>>  struct atom_value {\n>>         const char *s;\n>>         unsigned long ul; /* used for sorting when not FIELD_STR */\n>> @@ -129,6 +135,26 @@ int parse_ref_filter_atom(const char *atom, const char *ep)\n>>         return at;\n>>  }\n>>\n>> +static struct ref_formatting_state *push_new_state(struct ref_formatting_state **state)\n>\n> This interface seems confused. The caller receives the new state as\n> both the return value of the function and as an out-value of its sole\n> argument. I'd suggest choosing one or the other.\n>\n> Which one you choose depends upon how you view the operation and the\n> data structure. If you view it as linked-list manipulation, then you'd\n> probably want the new state returned, and accept a pointer argument\n> (rather than pointer-to-pointer). On the other hand, if you view it as\n> a stack, then you'd probably want to have it return void and\n> manipulate the sole argument. For this case, it might be clearer to\n> name the argument 'stack' rather than 'state'.\n>\n\nI guess it makes to stick to the stack arrangement, will change the argument\nname to stack.\n\n>> +{\n>> +       struct ref_formatting_state *new_state = xcalloc(1, sizeof(struct ref_formatting_state));\n>> +       struct ref_formatting_state *tmp = *state;\n>> +\n>> +       *state = new_state;\n>> +       new_state->prev = tmp;\n>> +       return new_state;\n>> +}\n>\n> A couple issues:\n>\n> First, you need to initialize the strbuf member of\n> ref_formatting_state after allocation:\n>\n>     new_state = xcalloc(1, sizeof(struct ref_formatting_state));\n>     strbuf_init(&new_state->output, 0);\n>\n\nWas under the impression that strbuf_init() is not needed after a xcalloc,\nhad to look at that again.\n\n> Second, if you re-order the code slightly, the 'tmp' variable becomes\n> unnecessary.\n>\n> Assuming that your intention was to match pop_state() and treat this\n> opaquely as a stack rather than exposing its linked-list\n> implementation, I'd have expected the function to look something like\n> this:\n>\n>     static void push_new_state(struct ref_formatting_state **stack)\n>     {\n>         struct ref_formatting_state *s = xcalloc(...);\n>         s->next = *stack;\n>         strbuf_init(&s->output, 0);\n>         *stack = s;\n>     }\n>\n\nYou mean:\n\nstatic void push_new_state(struct ref_formatting_state **stack)\n{\n    struct ref_formatting_state *s = xcalloc(...);\n\n    strbuf_init(&s->output, 0);\n    s->prev = *stack;\n    *stack = s;\n}\n\n>> +static void pop_state(struct ref_formatting_state **state)\n>> +{\n>> +       struct ref_formatting_state *current = *state;\n>> +       struct ref_formatting_state *prev = current->prev;\n>> +\n>> +       strbuf_release(&current->output);\n>> +       free(current);\n>> +       *state = prev;\n>> +}\n>\n> This interface suggests that you're treating it as an opaque stack, in\n> which case naming the argument 'stack' might be clearer.\n>\n\nWill do thanks.\n\n>>  /*\n>>   * In a format string, find the next occurrence of %(atom).\n>>   */\n>> @@ -1195,23 +1221,23 @@ void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)\n>>         qsort(array->items, array->nr, sizeof(struct ref_array_item *), compare_refs);\n>>  }\n>>\n>> -static void append_atom(struct atom_value *v, int quote_style, struct strbuf *output)\n>> +static void append_atom(struct atom_value *v, struct ref_formatting_state *state)\n>>  {\n>> -       switch (quote_style) {\n>> +       switch (state->quote_style) {\n>>         case QUOTE_NONE:\n>> -               strbuf_addstr(output, v->s);\n>> +               strbuf_addstr(&state->output, v->s);\n>>                 break;\n>>         case QUOTE_SHELL:\n>> -               sq_quote_buf(output, v->s);\n>> +               sq_quote_buf(&state->output, v->s);\n>>                 break;\n>>         case QUOTE_PERL:\n>> -               perl_quote_buf(output, v->s);\n>> +               perl_quote_buf(&state->output, v->s);\n>>                 break;\n>>         case QUOTE_PYTHON:\n>> -               python_quote_buf(output, v->s);\n>> +               python_quote_buf(&state->output, v->s);\n>>                 break;\n>>         case QUOTE_TCL:\n>> -               tcl_quote_buf(output, v->s);\n>> +               tcl_quote_buf(&state->output, v->s);\n>>                 break;\n>>         }\n>>  }\n>\n> This patch touches all the same lines as the previous patch which\n> converted the code to append to a strbuf rather than emit to stdout,\n> thus it makes the previous patch seem wasted and the current patch\n> much noisier. As such, it might make sense to repartition these two\n> patches as so:\n>\n> patch 2: Introduce ref_formatting_state (but without the 'prev'\n> field), and update callers to append to its 'output' member rather\n> than emitting to stdout.\n>\n> patch 3: Introduce the ref_formatting_state stack machinery; this\n> patch would also add the 'prev' field.\n>\n\nThis sounds good, will do this.\n\n--\nRegards,\nKarthik Nayak\n"}]}