{"thread":{"id":"40107","subject":"[PATCH v11 05/13] ref-filter: implement an `align` atom","startedAt":"2015-08-15T18:00:35Z","lastAt":"2015-08-17T19:12:27Z","messageCount":9,"participants":["Karthik Nayak","Andreas Schwab","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":11,"patchTotal":13},"messages":[{"id":"268138","messageId":"1439661643-16094-6-git-send-email-Karthik.188@gmail.com","threadId":"40107","inReplyTo":null,"subject":"[PATCH v11 05/13] ref-filter: implement an `align` atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-15T18:00:35Z","receivedAt":"2015-08-15T18:00:35Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Implement an `align` atom which left-, middle-, or right-aligns the\ncontent between %(align:..) and %(end).\n\nIt is followed by `:,`, where the `` is\neither left, right or middle and `` is the size of the area\ninto which the content will be placed. If the content between\n%(align:) and %(end) is more than the width then no alignment is\nperformed. e.g. to align a refname atom to the middle with a total\nwidth of 40 we can do: --format=\"%(align:middle,40)%(refname)%(end)\".\n\nThis is done by calling the strbuf_utf8_align() function in utf8.c.\n\nAdd documentation and tests for the same.\n\nMentored-by: Christian Couder \nMentored-by: Matthieu Moy \nSigned-off-by: Karthik Nayak \n---\n Documentation/git-for-each-ref.txt | 8 ++++\n ref-filter.c | 81 +++++++++++++++++++++++++++++++++++---\n ref-filter.h | 1 +\n t/t6302-for-each-ref-filter.sh | 48 ++++++++++++++++++++++\n 4 files changed, 133 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex e49d578..2b82334 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -127,6 +127,14 @@ color::\n \tChange output color. Followed by `:`, where names\n \tare described in `color.branch.*`.\n \n+align::\n+\tleft-, middle-, or right-align the content between %(align:..)\n+\tand %(end). Followed by `:,`, where the\n+\t`` is either left, right or middle and `` is\n+\tthe total length of the content with alignment. If the\n+\tcontents length is more than the width then no alignment is\n+\tperformed.\n+\n In addition to the above, for commit and tag objects, the header\n field names (`tree`, `parent`, `object`, `type`, and `tag`) can\n be used to specify the value in the header field.\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 3259363..eac99d0 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -10,6 +10,7 @@\n #include \"quote.h\"\n #include \"ref-filter.h\"\n #include \"revision.h\"\n+#include \"utf8.h\"\n \n typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;\n \n@@ -53,16 +54,27 @@ static struct {\n \t{ \"flag\" },\n \t{ \"HEAD\" },\n \t{ \"color\" },\n+\t{ \"align\" },\n+\t{ \"end\" },\n+};\n+\n+struct align {\n+\talign_type position;\n+\tunsigned int width;\n };\n \n struct ref_formatting_state {\n \tstruct strbuf output;\n \tstruct ref_formatting_state *prev;\n+\tvoid (*attend)(struct ref_formatting_state *state);\n+\tvoid *cb_data;\n \tint quote_style;\n };\n \n struct atom_value {\n \tconst char *s;\n+\tstruct align *align;\n+\tvoid (*handler)(struct atom_value *atomv, struct ref_formatting_state **state);\n \tunsigned long ul; /* used for sorting when not FIELD_STR */\n };\n \n@@ -137,12 +149,12 @@ int parse_ref_filter_atom(const char *atom, const char *ep)\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+\tstruct ref_formatting_state *new = xcalloc(1, sizeof(struct ref_formatting_state));\n+\tstruct ref_formatting_state *old = *state;\n \n-\t*state = new_state;\n-\tnew_state->prev = tmp;\n-\treturn new_state;\n+\t*state = new;\n+\tnew->prev = old;\n+\treturn new;\n }\n \n static void pop_state(struct ref_formatting_state **state)\n@@ -625,6 +637,34 @@ static inline char *copy_advance(char *dst, const char *src)\n \treturn dst;\n }\n \n+static void align_handler(struct ref_formatting_state *state)\n+{\n+\tstruct strbuf aligned = STRBUF_INIT;\n+\tstruct ref_formatting_state *return_to = state->prev;\n+\tstruct align *align = (struct align *)state->cb_data;\n+\n+\tstrbuf_utf8_align(&aligned, align->position, align->width, state->output.buf);\n+\tstrbuf_addbuf(&return_to->output, &aligned);\n+\tstrbuf_release(&aligned);\n+}\n+\n+static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_state **state)\n+{\n+\tstruct ref_formatting_state *new = push_new_state(state);\n+\tstrbuf_init(&new->output, 0);\n+\tnew->attend = align_handler;\n+\tnew->cb_data = atomv->align;\n+}\n+\n+static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state **state)\n+{\n+\tstruct ref_formatting_state *current = *state;\n+\tif (!current->attend)\n+\t\tdie(_(\"format: `end` atom used without a supporting atom\"));\n+\tcurrent->attend(current);\n+\tpop_state(state);\n+}\n+\n /*\n * Parse the object referred by ref, and grab needed value.\n */\n@@ -653,6 +693,7 @@ static void populate_value(struct ref_array_item *ref)\n \t\tint deref = 0;\n \t\tconst char *refname;\n \t\tconst char *formatp;\n+\t\tconst char *valp;\n \t\tstruct branch *branch = NULL;\n \n \t\tif (*name == '*') {\n@@ -718,6 +759,34 @@ static void populate_value(struct ref_array_item *ref)\n \t\t\telse\n \t\t\t\tv->s = \" \";\n \t\t\tcontinue;\n+\t\t} else if (skip_prefix(name, \"align:\", &valp)) {\n+\t\t\tstruct align *align = xmalloc(sizeof(struct align));\n+\t\t\tchar *ep = strchr(valp, ',');\n+\n+\t\t\tif (ep)\n+\t\t\t\t*ep = '\\0';\n+\n+\t\t\tif (strtoul_ui(valp, 10, &align->width))\n+\t\t\t\tdie(_(\"positive width expected align:%s\"), valp);\n+\n+\t\t\tif (!ep || starts_with(ep + 1, \"left\"))\n+\t\t\t\talign->position = ALIGN_LEFT;\n+\t\t\telse if (starts_with(ep + 1, \"right\"))\n+\t\t\t\talign->position = ALIGN_RIGHT;\n+\t\t\telse if (starts_with(ep + 1, \"middle\"))\n+\t\t\t\talign->position = ALIGN_MIDDLE;\n+\t\t\telse\n+\t\t\t\tdie(_(\"improper format entered align:%s\"), ep + 1);\n+\n+\t\t\tif (ep)\n+\t\t\t\t*ep = ',';\n+\n+\t\t\tv->align = align;\n+\t\t\tv->handler = align_atom_handler;\n+\t\t\tcontinue;\n+\t\t} else if (!strcmp(name, \"end\")) {\n+\t\t\tv->handler = end_atom_handler;\n+\t\t\tcontinue;\n \t\t} else\n \t\t\tcontinue;\n \n@@ -1296,6 +1365,8 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu\n \t\tif (cp < sp)\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\tif (atomv->handler)\n+\t\t\tatomv->handler(atomv, &state);\n \t\tappend_atom(atomv, state);\n \t}\n \tif (*cp) {\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 45026d0..144a633 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -5,6 +5,7 @@\n #include \"refs.h\"\n #include \"commit.h\"\n #include \"parse-options.h\"\n+#include \"utf8.h\"\n \n /* Quoting styles */\n #define QUOTE_NONE 0\ndiff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\nindex 505a360..b252a50 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -81,4 +81,52 @@ test_expect_success 'filtering with --contains' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'left alignment' '\n+\tcat >expect <<-\\EOF &&\n+\trefname is refs/heads/master |refs/heads/master\n+\trefname is refs/heads/side |refs/heads/side\n+\trefname is refs/odd/spot |refs/odd/spot\n+\trefname is refs/tags/double-tag|refs/tags/double-tag\n+\trefname is refs/tags/four |refs/tags/four\n+\trefname is refs/tags/one |refs/tags/one\n+\trefname is refs/tags/signed-tag|refs/tags/signed-tag\n+\trefname is refs/tags/three |refs/tags/three\n+\trefname is refs/tags/two |refs/tags/two\n+\tEOF\n+\tgit for-each-ref --format=\"%(align:30,left)refname is %(refname)%(end)|%(refname)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'middle alignment' '\n+\tcat >expect <<-\\EOF &&\n+\t| refname is refs/heads/master |refs/heads/master\n+\t| refname is refs/heads/side |refs/heads/side\n+\t| refname is refs/odd/spot |refs/odd/spot\n+\t|refname is refs/tags/double-tag|refs/tags/double-tag\n+\t| refname is refs/tags/four |refs/tags/four\n+\t| refname is refs/tags/one |refs/tags/one\n+\t|refname is refs/tags/signed-tag|refs/tags/signed-tag\n+\t| refname is refs/tags/three |refs/tags/three\n+\t| refname is refs/tags/two |refs/tags/two\n+\tEOF\n+\tgit for-each-ref --format=\"|%(align:30,middle)refname is %(refname)%(end)|%(refname)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'right alignment' '\n+\tcat >expect <<-\\EOF &&\n+\t| refname is refs/heads/master|refs/heads/master\n+\t| refname is refs/heads/side|refs/heads/side\n+\t| refname is refs/odd/spot|refs/odd/spot\n+\t|refname is refs/tags/double-tag|refs/tags/double-tag\n+\t| refname is refs/tags/four|refs/tags/four\n+\t| refname is refs/tags/one|refs/tags/one\n+\t|refname is refs/tags/signed-tag|refs/tags/signed-tag\n+\t| refname is refs/tags/three|refs/tags/three\n+\t| refname is refs/tags/two|refs/tags/two\n+\tEOF\n+\tgit for-each-ref --format=\"|%(align:30,right)refname is %(refname)%(end)|%(refname)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.5.0\n"},{"id":"268149","messageId":"CAOLa=ZS1gQuKYqYGjXx3M_XLFsc5uAxYcD6c-d-WJG7S_spYag@mail.gmail.com","threadId":"40107","inReplyTo":"1439661643-16094-6-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v11 05/13] ref-filter: implement an `align` atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-16T11:46:38Z","receivedAt":"2015-08-16T11:46:38Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"I think we need to squash this in\n\ndiff --git a/Documentation/git-for-each-ref.txt\nb/Documentation/git-for-each-ref.txt\nindex 3099631..17bd15e 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -129,7 +129,7 @@ color::\n\n align::\n left-, middle-, or right-align the content between %(align:..)\n- and %(end). Followed by `:,`, where the\n+ and %(end). Followed by `:>,`, where the\n `` is either left, right or middle and `` is\n the total length of the content with alignment. If the\n contents length is more than the width then no alignment is\n"},{"id":"268150","messageId":"87lhdbl9r2.fsf@igel.home","threadId":"40107","inReplyTo":"CAOLa=ZS1gQuKYqYGjXx3M_XLFsc5uAxYcD6c-d-WJG7S_spYag@mail.gmail.com","subject":"Re: [PATCH v11 05/13] ref-filter: implement an `align` atom","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2015-08-16T12:04:49Z","receivedAt":"2015-08-16T12:04:49Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"Karthik Nayak writes:\n\n> I think we need to squash this in\n>\n> diff --git a/Documentation/git-for-each-ref.txt\n> b/Documentation/git-for-each-ref.txt\n> index 3099631..17bd15e 100644\n> --- a/Documentation/git-for-each-ref.txt\n> +++ b/Documentation/git-for-each-ref.txt\n> @@ -129,7 +129,7 @@ color::\n>\n> align::\n> left-, middle-, or right-align the content between %(align:..)\n> - and %(end). Followed by `:,`, where the\n> + and %(end). Followed by `:>,`, where the\n\ns/>>/>/\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"268159","messageId":"CAPig+cQHdB9y+_+apWuz6hCZZfvXgTy6=jMrz7Zs_6JY0P=-zg@mail.gmail.com","threadId":"40107","inReplyTo":"87lhdbl9r2.fsf@igel.home","subject":"Re: [PATCH v11 05/13] ref-filter: implement an `align` atom","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-16T23:49:10Z","receivedAt":"2015-08-16T23:49:10Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Aug 16, 2015 at 8:04 AM, Andreas Schwab wrote:\n> Karthik Nayak writes:\n>> I think we need to squash this in\n>>\n>> diff --git a/Documentation/git-for-each-ref.txt\n>> b/Documentation/git-for-each-ref.txt\n>> index 3099631..17bd15e 100644\n>> --- a/Documentation/git-for-each-ref.txt\n>> +++ b/Documentation/git-for-each-ref.txt\n>> @@ -129,7 +129,7 @@ color::\n>>\n>> align::\n>> left-, middle-, or right-align the content between %(align:..)\n>> - and %(end). Followed by `:,`, where the\n>> + and %(end). Followed by `:>,`, where the\n>\n> s/>>/>/\n\nAlso: s/left-/Left-/\n"},{"id":"268160","messageId":"CAPig+cR=gCBiEnZbnPfZZs0WmjBsQyL+2BjSHggWWp_43rC9cg@mail.gmail.com","threadId":"40107","inReplyTo":"1439661643-16094-6-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v11 05/13] ref-filter: implement an `align` atom","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-17T02:07:24Z","receivedAt":"2015-08-17T02:07:24Z","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 wrote:\n> Implement an `align` atom which left-, middle-, or right-aligns the\n> content between %(align:..) and %(end).\n>\n> It is followed by `:,`, where the `` is\n> either left, right or middle and `` is the size of the area\n> into which the content will be placed. If the content between\n> %(align:) and %(end) is more than the width then no alignment is\n> performed. e.g. to align a refname atom to the middle with a total\n> width of 40 we can do: --format=\"%(align:middle,40)%(refname)%(end)\".\n>\n> This is done by calling the strbuf_utf8_align() function in utf8.c.\n>\n> Add documentation and tests for the same.\n>\n> Signed-off-by: Karthik Nayak \n> ---\n> diff --git a/ref-filter.c b/ref-filter.c\n> index 3259363..eac99d0 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -10,6 +10,7 @@\n> #include \"quote.h\"\n> #include \"ref-filter.h\"\n> #include \"revision.h\"\n> +#include \"utf8.h\"\n>\n> typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;\n>\n> @@ -53,16 +54,27 @@ static struct {\n> { \"flag\" },\n> { \"HEAD\" },\n> { \"color\" },\n> + { \"align\" },\n> + { \"end\" },\n> +};\n> +\n> +struct align {\n> + align_type position;\n> + unsigned int width;\n> };\n>\n> struct ref_formatting_state {\n> struct strbuf output;\n> struct ref_formatting_state *prev;\n> + void (*attend)(struct ref_formatting_state *state);\n\nJunio's suggestion for this member was \"at end\"; that is what to do\nwhen you are \"at\"-the-%(end), not \"attend\", which isn't meaningful\nhere. You could also call it 'end_scope', 'finish' or 'close' or\n'finalize' or something.\n\n> + void *cb_data;\n> int quote_style;\n> };\n>\n> struct atom_value {\n> const char *s;\n> + struct align *align;\n> + void (*handler)(struct atom_value *atomv, struct ref_formatting_state **state);\n> unsigned long ul; /* used for sorting when not FIELD_STR */\n> };\n>\n> @@ -137,12 +149,12 @@ int parse_ref_filter_atom(const char *atom, const char *ep)\n>\n> static struct ref_formatting_state *push_new_state(struct ref_formatting_state **state)\n> {\n> - struct ref_formatting_state *new_state = xcalloc(1, sizeof(struct ref_formatting_state));\n> - struct ref_formatting_state *tmp = *state;\n> + struct ref_formatting_state *new = xcalloc(1, sizeof(struct ref_formatting_state));\n> + struct ref_formatting_state *old = *state;\n>\n> - *state = new_state;\n> - new_state->prev = tmp;\n> - return new_state;\n> + *state = new;\n> + new->prev = old;\n> + return new;\n> }\n\nWhat are these changes about? They appear only to be renaming some\nvariables which were introduced in patch 3. It would make more sense\nto give them the desired names in the patch which introduces them.\n\n> static void pop_state(struct ref_formatting_state **state)\n> @@ -625,6 +637,34 @@ static inline char *copy_advance(char *dst, const char *src)\n> return dst;\n> }\n>\n> +static void align_handler(struct ref_formatting_state *state)\n\nThe names 'align_handler' and 'align_atom_handler' are confusingly\nsimilar. Perhaps name this end_align() or do_align() or\napply_alignment() or something?\n\n> +{\n> + struct strbuf aligned = STRBUF_INIT;\n> + struct ref_formatting_state *return_to = state->prev;\n> + struct align *align = (struct align *)state->cb_data;\n> +\n> + strbuf_utf8_align(&aligned, align->position, align->width, state->output.buf);\n> + strbuf_addbuf(&return_to->output, &aligned);\n\nA couple comments:\n\nFirst, why is 'strbuf aligned' needed? Can't you instead just invoke\nstrbuf_utf8_align(&return_to->output, ...)?\n\nSecond, I realize that Junio suggested the 'return_to' idea, but it\nseems like it could become overly painful since each handler of this\nsort is going to have to perform the same manipulation to append its\ncollected output to its parent state's output. What if you instead\nmake it the responsibility of pop_state() to append the 'output' from\nthe state being popped to the \"prev\" state's 'output'? This way, it\nhappens automatically, thus reducing code in each individual handler,\nand reducing the burden of having to keep writing the same code.\n\n> + strbuf_release(&aligned);\n> +}\n> +\n> +static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_state **state)\n> +{\n> + struct ref_formatting_state *new = push_new_state(state);\n> + strbuf_init(&new->output, 0);\n\nI think this strbuf_init() should be the responsibility of\npush_new_state(), as mentioned in my patch 3 review, otherwise every\ncaller of push_new_state() will have to remember to do this.\n\n> + new->attend = align_handler;\n> + new->cb_data = atomv->align;\n> +}\n> +\n> +static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state **state)\n> +{\n> + struct ref_formatting_state *current = *state;\n> + if (!current->attend)\n> + die(_(\"format: `end` atom used without a supporting atom\"));\n> + current->attend(current);\n> + pop_state(state);\n> +}\n> +\n> /*\n> * Parse the object referred by ref, and grab needed value.\n> */\n> @@ -653,6 +693,7 @@ static void populate_value(struct ref_array_item *ref)\n> int deref = 0;\n> const char *refname;\n> const char *formatp;\n> + const char *valp;\n> struct branch *branch = NULL;\n>\n> if (*name == '*') {\n> @@ -718,6 +759,34 @@ static void populate_value(struct ref_array_item *ref)\n> else\n> v->s = \" \";\n> continue;\n> + } else if (skip_prefix(name, \"align:\", &valp)) {\n> + struct align *align = xmalloc(sizeof(struct align));\n\nWho is responsible for freeing this memory?\n\n> + char *ep = strchr(valp, ',');\n> +\n> + if (ep)\n> + *ep = '\\0';\n> +\n> + if (strtoul_ui(valp, 10, &align->width))\n> + die(_(\"positive width expected align:%s\"), valp);\n> +\n> + if (!ep || starts_with(ep + 1, \"left\"))\n> + align->position = ALIGN_LEFT;\n> + else if (starts_with(ep + 1, \"right\"))\n> + align->position = ALIGN_RIGHT;\n> + else if (starts_with(ep + 1, \"middle\"))\n> + align->position = ALIGN_MIDDLE;\n\nShouldn't these be strcmp() rather than starts_with()? You don't want\nto match \"leftfoot\" as \"left\".\n\n> + else\n> + die(_(\"improper format entered align:%s\"), ep + 1);\n> +\n> + if (ep)\n> + *ep = ',';\n\nWhat's this conditional about? Why restore the comma?\n\n> +\n> + v->align = align;\n> + v->handler = align_atom_handler;\n\nJunio's proposal for using handlers[1] is a pretty big change which\nwould generalize atom processing overall, and which you haven't\nimplemented here. Instead, your use of handlers is just to avoid\nhaving special-case 'align' and 'end' conditionals spread throughout\nseveral functions. Am I understanding that correctly?\n\nIs the idea to leave that larger change for a later date (and possibly\na different programmer)?\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/275710\n\n> + continue;\n> + } else if (!strcmp(name, \"end\")) {\n> + v->handler = end_atom_handler;\n> + continue;\n> } else\n> continue;\n>\n> @@ -1296,6 +1365,8 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu\n> if (cp < sp)\n> append_literal(cp, sp, state);\n> get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);\n> + if (atomv->handler)\n> + atomv->handler(atomv, &state);\n> append_atom(atomv, state);\n> }\n> if (*cp) {\n> diff --git a/ref-filter.h b/ref-filter.h\n> index 45026d0..144a633 100644\n> --- a/ref-filter.h\n> +++ b/ref-filter.h\n> @@ -5,6 +5,7 @@\n> #include \"refs.h\"\n> #include \"commit.h\"\n> #include \"parse-options.h\"\n> +#include \"utf8.h\"\n\nWhy does this need to be #included here?\n\n> /* Quoting styles */\n> #define QUOTE_NONE 0\n"},{"id":"268184","messageId":"CAOLa=ZRfA-8_w6VKgWQsoL7TrdyjEq5LTHwas=_04tmx9MWhqA@mail.gmail.com","threadId":"40107","inReplyTo":"CAPig+cR=gCBiEnZbnPfZZs0WmjBsQyL+2BjSHggWWp_43rC9cg@mail.gmail.com","subject":"Re: [PATCH v11 05/13] ref-filter: implement an `align` atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-17T14:28:10Z","receivedAt":"2015-08-17T14:28:10Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Mon, Aug 17, 2015 at 7:37 AM, Eric Sunshine wrote:\n> On Sat, Aug 15, 2015 at 2:00 PM, Karthik Nayak wrote:\n>> Implement an `align` atom which left-, middle-, or right-aligns the\n>> content between %(align:..) and %(end).\n>>\n>> It is followed by `:,`, where the `` is\n>> either left, right or middle and `` is the size of the area\n>> into which the content will be placed. If the content between\n>> %(align:) and %(end) is more than the width then no alignment is\n>> performed. e.g. to align a refname atom to the middle with a total\n>> width of 40 we can do: --format=\"%(align:middle,40)%(refname)%(end)\".\n>>\n>> This is done by calling the strbuf_utf8_align() function in utf8.c.\n>>\n>> Add documentation and tests for the same.\n>>\n>> Signed-off-by: Karthik Nayak \n>> ---\n>> diff --git a/ref-filter.c b/ref-filter.c\n>> index 3259363..eac99d0 100644\n>> --- a/ref-filter.c\n>> +++ b/ref-filter.c\n>> @@ -10,6 +10,7 @@\n>> #include \"quote.h\"\n>> #include \"ref-filter.h\"\n>> #include \"revision.h\"\n>> +#include \"utf8.h\"\n>>\n>> typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;\n>>\n>> @@ -53,16 +54,27 @@ static struct {\n>> { \"flag\" },\n>> { \"HEAD\" },\n>> { \"color\" },\n>> + { \"align\" },\n>> + { \"end\" },\n>> +};\n>> +\n>> +struct align {\n>> + align_type position;\n>> + unsigned int width;\n>> };\n>>\n>> struct ref_formatting_state {\n>> struct strbuf output;\n>> struct ref_formatting_state *prev;\n>> + void (*attend)(struct ref_formatting_state *state);\n>\n> Junio's suggestion for this member was \"at end\"; that is what to do\n> when you are \"at\"-the-%(end), not \"attend\", which isn't meaningful\n> here. You could also call it 'end_scope', 'finish' or 'close' or\n> 'finalize' or something.\n>\n\nWeirdly, attend made sense to me, its a function you attend to at the end\ntypes. probably \"at_end\" would be better.\n\n>> + void *cb_data;\n>> int quote_style;\n>> };\n>>\n>> struct atom_value {\n>> const char *s;\n>> + struct align *align;\n>> + void (*handler)(struct atom_value *atomv, struct ref_formatting_state **state);\n>> unsigned long ul; /* used for sorting when not FIELD_STR */\n>> };\n>>\n>> @@ -137,12 +149,12 @@ int parse_ref_filter_atom(const char *atom, const char *ep)\n>>\n>> static struct ref_formatting_state *push_new_state(struct ref_formatting_state **state)\n>> {\n>> - struct ref_formatting_state *new_state = xcalloc(1, sizeof(struct ref_formatting_state));\n>> - struct ref_formatting_state *tmp = *state;\n>> + struct ref_formatting_state *new = xcalloc(1, sizeof(struct ref_formatting_state));\n>> + struct ref_formatting_state *old = *state;\n>>\n>> - *state = new_state;\n>> - new_state->prev = tmp;\n>> - return new_state;\n>> + *state = new;\n>> + new->prev = old;\n>> + return new;\n>> }\n>\n> What are these changes about? They appear only to be renaming some\n> variables which were introduced in patch 3. It would make more sense\n> to give them the desired names in the patch which introduces them.\n>\n\nAgreed, will scrap them off.\n\n>> static void pop_state(struct ref_formatting_state **state)\n>> @@ -625,6 +637,34 @@ static inline char *copy_advance(char *dst, const char *src)\n>> return dst;\n>> }\n>>\n>> +static void align_handler(struct ref_formatting_state *state)\n>\n> The names 'align_handler' and 'align_atom_handler' are confusingly\n> similar. Perhaps name this end_align() or do_align() or\n> apply_alignment() or something?\n>\n>> +{\n>> + struct strbuf aligned = STRBUF_INIT;\n>> + struct ref_formatting_state *return_to = state->prev;\n>> + struct align *align = (struct align *)state->cb_data;\n>> +\n>> + strbuf_utf8_align(&aligned, align->position, align->width, state->output.buf);\n>> + strbuf_addbuf(&return_to->output, &aligned);\n>\n> A couple comments:\n>\n> First, why is 'strbuf aligned' needed? Can't you instead just invoke\n> strbuf_utf8_align(&return_to->output, ...)?\n\nYeah, will remove it.\n\n>\n> Second, I realize that Junio suggested the 'return_to' idea, but it\n> seems like it could become overly painful since each handler of this\n> sort is going to have to perform the same manipulation to append its\n> collected output to its parent state's output. What if you instead\n> make it the responsibility of pop_state() to append the 'output' from\n> the state being popped to the \"prev\" state's 'output'? This way, it\n> happens automatically, thus reducing code in each individual handler,\n> and reducing the burden of having to keep writing the same code.\n>\n\nGood question, what if we don't want to append to strbuf at all?\nFor e.g., We were discussing an \"%(if).....%(then)......%(end)\"\natom structure, here if the if condition isn't met we wouldn't want to\nappend to the prev strbuf, hence I thought it's better if the handler\ndecided whether or not to append to prev strbuf.\n\n>> + strbuf_release(&aligned);\n>> +}\n>> +\n>> +static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_state **state)\n>> +{\n>> + struct ref_formatting_state *new = push_new_state(state);\n>> + strbuf_init(&new->output, 0);\n>\n> I think this strbuf_init() should be the responsibility of\n> push_new_state(), as mentioned in my patch 3 review, otherwise every\n> caller of push_new_state() will have to remember to do this.\n>\n\nAs mentioned in your previous review, this will be changed.\n\n>> + new->attend = align_handler;\n>> + new->cb_data = atomv->align;\n>> +}\n>> +\n>> +static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state **state)\n>> +{\n>> + struct ref_formatting_state *current = *state;\n>> + if (!current->attend)\n>> + die(_(\"format: `end` atom used without a supporting atom\"));\n>> + current->attend(current);\n>> + pop_state(state);\n>> +}\n>> +\n>> /*\n>> * Parse the object referred by ref, and grab needed value.\n>> */\n>> @@ -653,6 +693,7 @@ static void populate_value(struct ref_array_item *ref)\n>> int deref = 0;\n>> const char *refname;\n>> const char *formatp;\n>> + const char *valp;\n>> struct branch *branch = NULL;\n>>\n>> if (*name == '*') {\n>> @@ -718,6 +759,34 @@ static void populate_value(struct ref_array_item *ref)\n>> else\n>> v->s = \" \";\n>> continue;\n>> + } else if (skip_prefix(name, \"align:\", &valp)) {\n>> + struct align *align = xmalloc(sizeof(struct align));\n>\n> Who is responsible for freeing this memory?\n>\n\nShould have been done in align_handler().\n\n>> + char *ep = strchr(valp, ',');\n>> +\n>> + if (ep)\n>> + *ep = '\\0';\n>> +\n>> + if (strtoul_ui(valp, 10, &align->width))\n>> + die(_(\"positive width expected align:%s\"), valp);\n>> +\n>> + if (!ep || starts_with(ep + 1, \"left\"))\n>> + align->position = ALIGN_LEFT;\n>> + else if (starts_with(ep + 1, \"right\"))\n>> + align->position = ALIGN_RIGHT;\n>> + else if (starts_with(ep + 1, \"middle\"))\n>> + align->position = ALIGN_MIDDLE;\n>\n> Shouldn't these be strcmp() rather than starts_with()? You don't want\n> to match \"leftfoot\" as \"left\".\n>\n\nMakes sense.\n\n>> + else\n>> + die(_(\"improper format entered align:%s\"), ep + 1);\n>> +\n>> + if (ep)\n>> + *ep = ',';\n>\n> What's this conditional about? Why restore the comma?\n\nbecause we go through the atoms, once when we verify the format\nusing verify_format() and again when we call show_ref_array_item()\nand this if the atom is changed this would cause a segfault in the second\npho\n\n>\n>> +\n>> + v->align = align;\n>> + v->handler = align_atom_handler;\n>\n> Junio's proposal for using handlers[1] is a pretty big change which\n> would generalize atom processing overall, and which you haven't\n> implemented here. Instead, your use of handlers is just to avoid\n> having special-case 'align' and 'end' conditionals spread throughout\n> several functions. Am I understanding that correctly?\n>\n> Is the idea to leave that larger change for a later date (and possibly\n> a different programmer)?\n>\n> [1]: http://article.gmane.org/gmane.comp.version-control.git/275710\n>\n\nIf you're referring to how he suggested everything could just use the\nhandler and\nwork on that, yes, I'm leaving that out for the moment, probably me or\nsomeone else\ncould work on that after this.\n\n>> + continue;\n>> + } else if (!strcmp(name, \"end\")) {\n>> + v->handler = end_atom_handler;\n>> + continue;\n>> } else\n>> continue;\n>>\n>> @@ -1296,6 +1365,8 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu\n>> if (cp < sp)\n>> append_literal(cp, sp, state);\n>> get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);\n>> + if (atomv->handler)\n>> + atomv->handler(atomv, &state);\n>> append_atom(atomv, state);\n>> }\n>> if (*cp) {\n>> diff --git a/ref-filter.h b/ref-filter.h\n>> index 45026d0..144a633 100644\n>> --- a/ref-filter.h\n>> +++ b/ref-filter.h\n>> @@ -5,6 +5,7 @@\n>> #include \"refs.h\"\n>> #include \"commit.h\"\n>> #include \"parse-options.h\"\n>> +#include \"utf8.h\"\n>\n> Why does this need to be #included here?\n>\n\nI'll remove that.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"268197","messageId":"CAPig+cQiPKv15dGFskwxwoJceYevnNy9j-6oW7A3UySadOdBBw@mail.gmail.com","threadId":"40107","inReplyTo":"CAOLa=ZRfA-8_w6VKgWQsoL7TrdyjEq5LTHwas=_04tmx9MWhqA@mail.gmail.com","subject":"Re: [PATCH v11 05/13] ref-filter: implement an `align` atom","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-17T18:22:16Z","receivedAt":"2015-08-17T18:22:16Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Aug 17, 2015 at 10:28 AM, Karthik Nayak wrote:\n> On Mon, Aug 17, 2015 at 7:37 AM, Eric Sunshine wrote:\n>> On Sat, Aug 15, 2015 at 2:00 PM, Karthik Nayak wrote:\n>>> +{\n>>> + struct strbuf aligned = STRBUF_INIT;\n>>> + struct ref_formatting_state *return_to = state->prev;\n>>> + struct align *align = (struct align *)state->cb_data;\n>>> +\n>>> + strbuf_utf8_align(&aligned, align->position, align->width, state->output.buf);\n>>> + strbuf_addbuf(&return_to->output, &aligned);\n>>\n>> Second, I realize that Junio suggested the 'return_to' idea, but it\n>> seems like it could become overly painful since each handler of this\n>> sort is going to have to perform the same manipulation to append its\n>> collected output to its parent state's output. What if you instead\n>> make it the responsibility of pop_state() to append the 'output' from\n>> the state being popped to the \"prev\" state's 'output'? This way, it\n>> happens automatically, thus reducing code in each individual handler,\n>> and reducing the burden of having to keep writing the same code.\n>\n> Good question, what if we don't want to append to strbuf at all?\n> For e.g., We were discussing an \"%(if).....%(then)......%(end)\"\n> atom structure, here if the if condition isn't met we wouldn't want to\n> append to the prev strbuf, hence I thought it's better if the handler\n> decided whether or not to append to prev strbuf.\n\nAn %(if)/%(then)/%(end) construct should not present a problem. As\nlong as the processing of the conditional ensures that the current\nstate's 'output' contains no content, when pop_state() appends that\n(empty) content to the parent state's 'output', then the net result is\nexactly the desired behavior.\n\nThe question of \"how\" the conditional processing ensures that the\ncurrent state's 'output' is empty when the %(if) condition is false is\nunimportant (for this discussion). It might be the case that it just\ndoesn't collect any content at all for a false condition, or it\ncollects it but throws it away before the state is popped. Either way,\nthat's an implementation detail which needn't impact the decision to\nretire 'return_to' and instead make pop_state() responsible for\nappending the current state's output to the parent state's.\n"},{"id":"268202","messageId":"xmqqa8tpep3s.fsf@gitster.dls.corp.google.com","threadId":"40107","inReplyTo":"CAOLa=ZRfA-8_w6VKgWQsoL7TrdyjEq5LTHwas=_04tmx9MWhqA@mail.gmail.com","subject":"Re: [PATCH v11 05/13] ref-filter: implement an `align` atom","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-17T18:39:35Z","receivedAt":"2015-08-17T18:39:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> On Mon, Aug 17, 2015 at 7:37 AM, Eric Sunshine wrote:\n> ...\n>> Second, I realize that Junio suggested the 'return_to' idea, but it\n>> seems like it could become overly painful since each handler of this\n>> sort is going to have to perform the same manipulation to append its\n>> collected output to its parent state's output. What if you instead\n>> make it the responsibility of pop_state() to append the 'output' from\n>> the state being popped to the \"prev\" state's 'output'? This way, it\n>> happens automatically, thus reducing code in each individual handler,\n>> and reducing the burden of having to keep writing the same code.\n>\n> Good question, what if we don't want to append to strbuf at all?\n> For e.g., We were discussing an \"%(if).....%(then)......%(end)\"\n> atom structure, here if the if condition isn't met we wouldn't want to\n> append to the prev strbuf, hence I thought it's better if the handler\n> decided whether or not to append to prev strbuf.\n\nI'd imagine the implementation of these to be along the lines of\n(thinking aloud):\n\n - \"%(if:[nonempty|empty|...])\" pushes a new stack, and sets its\n attend/at_end/end_scope function to signal a syntax error. It\n also records what condition (e.g. \"nonempty\") to use in the new\n stack.\n\n - \"%(then)\" inspects the top-of-stack output and uses the condition\n recorded by the %(if) that created the stack to decide true or\n false. The stack element pushed by %(if) is then removed.\n Notice that the end_scope function prepared by %(if) is never\n called.\n\n Then (no pun intended):\n\n - If true, that means we would want the (expansion of) text up to\n \"%(end)\" or \"%(else)\", whichever comes first, appended to the\n surrounding output. Push a new stack and set its end_scope\n function to the one that appends the top-of-stack output to the\n surrounding output, expecting %(end) will follow without\n %(else).\n\n - If false, that means we would want the (expansion of) text up\n to \"%(end)\" or \"%(else)\", whichever comes first, discarded.\n Push a new stack and set its end_scope function to the one that\n discards the top-of-stack output, expecting %(end) will follow\n without %(else).\n\n - \"%(else)\" inspects the top of the stack, and if it is not left by\n \"%(then)\", signal a syntax error.\n\n Else (no pun intended), it runs the end_scope function left by\n \"%(then)\" on the top-of-stack output (e.g. if \"%(then)\" found\n that the condition holds true, the accumulated output at this\n point should be appended to the surrounding output, and it was\n expected to be done by \"%(end)\" if this \"%(else)\" weren't\n present. We do it here before waiting for \"%(end)\").\n\n Truncate the top-of-stack output, flip the end_scope function to\n the one opposite from the one left by \"%(then)\". And let \"%(end)\"\n take care of it.\n\n - \"%(end)\" just unconditionally runs the end_scope function on the\n top of the stack output.\n\nEric's suggestion is let the caller of the end_scope function to\nalways append the output of the top-of-stack, and I think it makes\nsense. It makes a common \"%(atom)\" implementation simpler. Things\nlike %(then) and %(else) above need to make sure that they reset the\ntop-of-stack output to empty as necessary, but that is not such a\nhuge implementation burden---their operation is already unusual and\nneeds to be more complex than the plain-vanilla %(atom)s anyway.\n"},{"id":"268209","messageId":"CAOLa=ZQqE4xF_NDahy5T+d5B0k0yNPFjY=qrmLoi3URekZ+QmA@mail.gmail.com","threadId":"40107","inReplyTo":"xmqqa8tpep3s.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v11 05/13] ref-filter: implement an `align` atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-17T19:12:27Z","receivedAt":"2015-08-17T19:12:27Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Mon, Aug 17, 2015 at 11:52 PM, Eric Sunshine wrote:\n> On Mon, Aug 17, 2015 at 10:28 AM, Karthik Nayak wrote:\n>> On Mon, Aug 17, 2015 at 7:37 AM, Eric Sunshine wrote:\n>>> On Sat, Aug 15, 2015 at 2:00 PM, Karthik Nayak wrote:\n>>>> +{\n>>>> + struct strbuf aligned = STRBUF_INIT;\n>>>> + struct ref_formatting_state *return_to = state->prev;\n>>>> + struct align *align = (struct align *)state->cb_data;\n>>>> +\n>>>> + strbuf_utf8_align(&aligned, align->position, align->width, state->output.buf);\n>>>> + strbuf_addbuf(&return_to->output, &aligned);\n>>>\n>>> Second, I realize that Junio suggested the 'return_to' idea, but it\n>>> seems like it could become overly painful since each handler of this\n>>> sort is going to have to perform the same manipulation to append its\n>>> collected output to its parent state's output. What if you instead\n>>> make it the responsibility of pop_state() to append the 'output' from\n>>> the state being popped to the \"prev\" state's 'output'? This way, it\n>>> happens automatically, thus reducing code in each individual handler,\n>>> and reducing the burden of having to keep writing the same code.\n>>\n>> Good question, what if we don't want to append to strbuf at all?\n>> For e.g., We were discussing an \"%(if).....%(then)......%(end)\"\n>> atom structure, here if the if condition isn't met we wouldn't want to\n>> append to the prev strbuf, hence I thought it's better if the handler\n>> decided whether or not to append to prev strbuf.\n>\n> An %(if)/%(then)/%(end) construct should not present a problem. As\n> long as the processing of the conditional ensures that the current\n> state's 'output' contains no content, when pop_state() appends that\n> (empty) content to the parent state's 'output', then the net result is\n> exactly the desired behavior.\n>\n> The question of \"how\" the conditional processing ensures that the\n> current state's 'output' is empty when the %(if) condition is false is\n> unimportant (for this discussion). It might be the case that it just\n> doesn't collect any content at all for a false condition, or it\n> collects it but throws it away before the state is popped. Either way,\n> that's an implementation detail which needn't impact the decision to\n> retire 'return_to' and instead make pop_state() responsible for\n> appending the current state's output to the parent state's.\n\nI guess what you're saying also makes sense. We could make it common\nfor pop to push the strbuf into the prev state.\n\nOn Tue, Aug 18, 2015 at 12:09 AM, Junio C Hamano wrote:\n> Karthik Nayak writes:\n>\n>> On Mon, Aug 17, 2015 at 7:37 AM, Eric Sunshine wrote:\n>> ...\n>>> Second, I realize that Junio suggested the 'return_to' idea, but it\n>>> seems like it could become overly painful since each handler of this\n>>> sort is going to have to perform the same manipulation to append its\n>>> collected output to its parent state's output. What if you instead\n>>> make it the responsibility of pop_state() to append the 'output' from\n>>> the state being popped to the \"prev\" state's 'output'? This way, it\n>>> happens automatically, thus reducing code in each individual handler,\n>>> and reducing the burden of having to keep writing the same code.\n>>\n>> Good question, what if we don't want to append to strbuf at all?\n>> For e.g., We were discussing an \"%(if).....%(then)......%(end)\"\n>> atom structure, here if the if condition isn't met we wouldn't want to\n>> append to the prev strbuf, hence I thought it's better if the handler\n>> decided whether or not to append to prev strbuf.\n>\n> I'd imagine the implementation of these to be along the lines of\n> (thinking aloud):\n>\n> - \"%(if:[nonempty|empty|...])\" pushes a new stack, and sets its\n> attend/at_end/end_scope function to signal a syntax error. It\n> also records what condition (e.g. \"nonempty\") to use in the new\n> stack.\n>\n> - \"%(then)\" inspects the top-of-stack output and uses the condition\n> recorded by the %(if) that created the stack to decide true or\n> false. The stack element pushed by %(if) is then removed.\n> Notice that the end_scope function prepared by %(if) is never\n> called.\n>\n> Then (no pun intended):\n>\n> - If true, that means we would want the (expansion of) text up to\n> \"%(end)\" or \"%(else)\", whichever comes first, appended to the\n> surrounding output. Push a new stack and set its end_scope\n> function to the one that appends the top-of-stack output to the\n> surrounding output, expecting %(end) will follow without\n> %(else).\n>\n> - If false, that means we would want the (expansion of) text up\n> to \"%(end)\" or \"%(else)\", whichever comes first, discarded.\n> Push a new stack and set its end_scope function to the one that\n> discards the top-of-stack output, expecting %(end) will follow\n> without %(else).\n>\n> - \"%(else)\" inspects the top of the stack, and if it is not left by\n> \"%(then)\", signal a syntax error.\n>\n> Else (no pun intended), it runs the end_scope function left by\n> \"%(then)\" on the top-of-stack output (e.g. if \"%(then)\" found\n> that the condition holds true, the accumulated output at this\n> point should be appended to the surrounding output, and it was\n> expected to be done by \"%(end)\" if this \"%(else)\" weren't\n> present. We do it here before waiting for \"%(end)\").\n>\n> Truncate the top-of-stack output, flip the end_scope function to\n> the one opposite from the one left by \"%(then)\". And let \"%(end)\"\n> take care of it.\n>\n> - \"%(end)\" just unconditionally runs the end_scope function on the\n> top of the stack output.\n>\n\nThat seems like the way to go, thanks for summing it up. Will follow this\nwhen implementing the %(if) %(then) %(else) %(end) atom series.\n\n> Eric's suggestion is let the caller of the end_scope function to\n> always append the output of the top-of-stack, and I think it makes\n> sense. It makes a common \"%(atom)\" implementation simpler. Things\n> like %(then) and %(else) above need to make sure that they reset the\n> top-of-stack output to empty as necessary, but that is not such a\n> huge implementation burden---their operation is already unusual and\n> needs to be more complex than the plain-vanilla %(atom)s anyway.\n\nYea, I get why you're suggesting it too, It'd remove that extra work each\nhandler has to do and make sure is done. By moving it to pop_state()\nwe don't have to leave it to the handler to be taken care of.\n\n-- \nRegards,\nKarthik Nayak\n"}]}