{"thread":{"id":"40021","subject":"[PATCH v9 03/11] ref-filter: implement an `align` atom","startedAt":"2015-08-05T18:54:14Z","lastAt":"2015-08-09T12:54:34Z","messageCount":13,"participants":["Karthik Nayak","Eric Sunshine","Matthieu Moy"],"isPatch":true,"patchVersion":9,"patchTotal":11},"messages":[{"id":"267530","messageId":"1438800854-19901-1-git-send-email-Karthik.188@gmail.com","threadId":"40021","inReplyTo":"CAOLa=ZRnnMBKpsq1ANBVgF2=xwK=A2EsPKKrGS0R4mZ8iATKfA@mail.gmail.com","subject":"[PATCH v9 03/11] ref-filter: implement an `align` atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-05T18:54:14Z","receivedAt":"2015-08-05T18:54:14Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Implement an `align` atom which will act as a modifier atom and align\nany string with or without an %(atom) appearing before a %(end) atom\nto the right, left or middle.\n\nIt is followed by `:,`, where the `` is\neither left, right or middle and `` is the total length\nof the padding to be performed. If the atom length is more than the\npadding length then no padding is performed. e.g. to pad a succeeding\natom to the middle with a total padding size of 40 we can do a\n--format=\"%(align:middle,40)..\"\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 | 84 +++++++++++++++++++++++++++++++++++---\n ref-filter.h | 18 +++++++-\n t/t6302-for-each-ref-filter.sh | 48 ++++++++++++++++++++++\n 4 files changed, 151 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex e49d578..d865f98 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+\tAlign any string with or without %(atom) before the %(end)\n+\tatom to the right, left or middle. Followed by\n+\t`:,`, where the `` is either left,\n+\tright or middle and `` is the total length of\n+\tthe padding to be performed. If the string length is more than\n+\tthe padding length then no padding is performed.\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 2c074a1..d123299 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,6 +54,8 @@ static struct {\n \t{ \"flag\" },\n \t{ \"HEAD\" },\n \t{ \"color\" },\n+\t{ \"align\" },\n+\t{ \"end\" },\n };\n \n /*\n@@ -620,7 +623,7 @@ static void populate_value(struct ref_array_item *ref)\n \t\tconst char *name = used_atom[i];\n \t\tstruct atom_value *v = &ref->value[i];\n \t\tint deref = 0;\n-\t\tconst char *refname;\n+\t\tconst char *refname = NULL;\n \t\tconst char *formatp;\n \t\tstruct branch *branch = NULL;\n \n@@ -687,6 +690,29 @@ 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 (starts_with(name, \"align:\")) {\n+\t\t\tconst char *valp = NULL;\n+\t\t\tstruct align *align = xmalloc(sizeof(struct align));\n+\n+\t\t\tskip_prefix(name, \"align:\", &valp);\n+\n+\t\t\tif (skip_prefix(valp, \"left,\", &valp))\n+\t\t\t\talign->align_type = ALIGN_LEFT;\n+\t\t\telse if (skip_prefix(valp, \"right,\", &valp))\n+\t\t\t\talign->align_type = ALIGN_RIGHT;\n+\t\t\telse if (skip_prefix(valp, \"middle,\", &valp))\n+\t\t\t\talign->align_type = ALIGN_MIDDLE;\n+\t\t\telse\n+\t\t\t\tdie(_(\"align: improper format\"));\n+\t\t\tif (strtoul_ui(valp, 10, &align->align_value))\n+\t\t\t\tdie(_(\"align: positive value expected\"));\n+\t\t\tv->align = align;\n+\t\t\tv->modifier_atom = 1;\n+\t\t\tcontinue;\n+\t\t} else if (starts_with(name, \"end\")) {\n+\t\t\tv->end = 1;\n+\t\t\tv->modifier_atom = 1;\n+\t\t\tcontinue;\n \t\t} else\n \t\t\tcontinue;\n \n@@ -1251,12 +1277,48 @@ static void emit(const char *cp, const char *ep, struct ref_formatting_state *st\n \n static void process_formatting_state(struct atom_value *atomv, struct ref_formatting_state *state)\n {\n-\t/* Based on the atomv values, the formatting state is set */\n+\tif (atomv->align) {\n+\t\tstate->align = atomv->align;\n+\t\tatomv->align = NULL;\n+\t}\n+\tif (atomv->end)\n+\t\tstate->end = 1;\n }\n \n static void apply_formatting_state(struct ref_formatting_state *state, struct strbuf *final)\n {\n-\t/* More formatting options to be evetually added */\n+\tif (state->align && state->end) {\n+\t\tstruct strbuf *value = state->output;\n+\t\tint len = 0, buf_len = value->len;\n+\t\tstruct align *align = state->align;\n+\n+\t\tif (!value->buf)\n+\t\t\treturn;\n+\t\tif (!is_utf8(value->buf)) {\n+\t\t\tlen = value->len - utf8_strwidth(value->buf);\n+\t\t\tbuf_len -= len;\n+\t\t}\n+\n+\t\tif (align->align_value < buf_len) {\n+\t\t\tstate->align = NULL;\n+\t\t\tstrbuf_addbuf(final, value);\n+\t\t\tstrbuf_release(value);\n+\t\t\treturn;\n+\t\t}\n+\n+\t\tif (align->align_type == ALIGN_LEFT)\n+\t\t\tstrbuf_addf(final, \"%-*s\", len + align->align_value, value->buf);\n+\t\telse if (align->align_type == ALIGN_MIDDLE) {\n+\t\t\tint right = (align->align_value - buf_len)/2;\n+\t\t\tstrbuf_addf(final, \"%*s%-*s\", align->align_value - right + len,\n+\t\t\t\t value->buf, right, \"\");\n+\t\t} else if (align->align_type == ALIGN_RIGHT)\n+\t\t\tstrbuf_addf(final, \"%*s\", align->align_value, value->buf);\n+\t\tstrbuf_release(value);\n+\t\tstate->align = NULL;\n+\t\treturn;\n+\t} else if (state->align)\n+\t\treturn;\n \tstrbuf_addbuf(final, state->output);\n \tstrbuf_release(state->output);\n }\n@@ -1282,10 +1344,19 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu\n \t\t\tapply_formatting_state(&state, &final_buf);\n \t\t}\n \t\tget_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv);\n-\t\tprocess_formatting_state(atomv, &state);\n-\t\tprint_value(atomv, &state);\n-\t\tapply_formatting_state(&state, &final_buf);\n+\t\tif (atomv->modifier_atom)\n+\t\t\tprocess_formatting_state(atomv, &state);\n+\t\telse {\n+\t\t\tprint_value(atomv, &state);\n+\t\t\tapply_formatting_state(&state, &final_buf);\n+\t\t}\n+\t\tif (atomv->end) {\n+\t\t\tprint_value(atomv, &state);\n+\t\t\tapply_formatting_state(&state, &final_buf);\n+\t\t}\n \t}\n+\tif (state.align)\n+\t\tdie(_(\"format: align used without an `end` atom\"));\n \tif (*cp) {\n \t\tsp = cp + strlen(cp);\n \t\temit(cp, sp, &state);\n@@ -1301,6 +1372,7 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu\n \t\tprint_value(&resetv, &state);\n \t\tapply_formatting_state(&state, &final_buf);\n \t}\n+\n \tfor (i = 0; i < final_buf.len; i++)\n \t\tprintf(\"%c\", final_buf.buf[i]);\n \tputchar('\\n');\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 9e6c2d4..5575fe9 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -16,14 +16,30 @@\n #define FILTER_REFS_INCLUDE_BROKEN 0x1\n #define FILTER_REFS_ALL 0x2\n \n+typedef enum {\n+\tALIGN_LEFT,\n+\tALIGN_MIDDLE,\n+\tALIGN_RIGHT\n+} align_type;\n+\n struct ref_formatting_state {\n-\tint quote_style;\n \tstruct strbuf *output;\n+\tstruct align *align;\n+\tint quote_style;\n+\tunsigned int end : 1;\n+};\n+\n+struct align {\n+\talign_type align_type;\n+\tunsigned int align_value;\n };\n \n struct atom_value {\n \tconst char *s;\n+\tstruct align *align;\n \tunsigned long ul; /* used for sorting when not FIELD_STR */\n+\tunsigned int modifier_atom : 1,\n+\t\tend : 1;\n };\n \n struct ref_sorting {\ndiff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\nindex 505a360..76041a2 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:left,30)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:middle,30)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:right,30)refname is %(refname)%(end)|%(refname)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.5.0\n"},{"id":"267600","messageId":"CAPig+cSTssDihF5cBwu=2uKX1y6GqH-5EhKnb92Fpp30JA7pwA@mail.gmail.com","threadId":"40021","inReplyTo":"1438800854-19901-1-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v9 03/11] ref-filter: implement an `align` atom","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-07T03:27:20Z","receivedAt":"2015-08-07T03:27:20Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Aug 5, 2015 at 2:54 PM, Karthik Nayak wrote:\n> Implement an `align` atom which will act as a modifier atom and align\n> any string with or without an %(atom) appearing before a %(end) atom\n> to the right, left or middle.\n\nFor someone not familiar with the evolution of this patch series,\n\"align any string with or without an %(atom)\" is confusing and\ndistracting and seems to imply that something extra mysterious is\ngoing on behind the scenes. Keeping it simple makes it easier to\nunderstand:\n\n Add 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\n'type' may not be the best name. Perhaps 'style' or 'position' or\nsomething else would be better?\n\n> either left, right or middle and `` is the total length\n\n'paddinglength' is misleading. You're really describing the container\nwidth in which alignment takes places, so perhaps call it 'width' or\n'fieldwidth' or something.\n\n> of the padding to be performed. If the atom length is more than the\n> padding length then no padding is performed. e.g. to pad a succeeding\n> atom to the middle with a total padding size of 40 we can do a\n\nIt's odd to have alignment described in terms of \"padding\" and\n\"padding length\", especially in the case of \"center\" alignment. It\nmight be better to rephrase the discussion in terms of field width or\nsuch.\n\n> --format=\"%(align:middle,40)..\"\n\nI may have missed the discussion, but why was comma chosen as a\nseparator here, rather than, say, colon? For instance:\n\n %(align:middle:40)\n\n> Add documentation and tests for the same.\n>\n> Signed-off-by: Karthik Nayak \n> ---\n> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\n> index e49d578..d865f98 100644\n> --- a/Documentation/git-for-each-ref.txt\n> +++ b/Documentation/git-for-each-ref.txt\n> @@ -127,6 +127,14 @@ color::\n> +align::\n> + Align any string with or without %(atom) before the %(end)\n> + atom to the right, left or middle. Followed by\n\nDitto regarding \"any string with or without %(atom)\" being confusing\nto someone not familiar with the evolution of this patch series.\nInstead, perhaps:\n\n Left-, middle-, or right-align content between %(align:...)\n and %(end).\n\n> + `:,`, where the `` is either left,\n> + right or middle and `` is the total length of\n> + the padding to be performed. If the string length is more than\n> + the padding length then no padding is performed.\n\nDitto regarding above observations about 'type', 'paddinglength', and \"padding\".\n\n> diff --git a/ref-filter.c b/ref-filter.c\n> index 2c074a1..d123299 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -620,7 +623,7 @@ static void populate_value(struct ref_array_item *ref)\n> const char *name = used_atom[i];\n> struct atom_value *v = &ref->value[i];\n> int deref = 0;\n> - const char *refname;\n> + const char *refname = NULL;\n\nWhat is this change about?\n\n> const char *formatp;\n> struct branch *branch = NULL;\n>\n> @@ -687,6 +690,29 @@ static void populate_value(struct ref_array_item *ref)\n> else\n> v->s = \" \";\n> continue;\n> + } else if (starts_with(name, \"align:\")) {\n> + const char *valp = NULL;\n\nUnnecessary NULL assignment.\n\n> + struct align *align = xmalloc(sizeof(struct align));\n> +\n> + skip_prefix(name, \"align:\", &valp);\n\nYou could simplify the code by combining this skip_prefix() with the\nearlier starts_with() in the conditional:\n\n } else if (skip_prefix(name, \"align:\", &valp)) {\n struct align *align = xmalloc(sizeof(struct align));\n ...\n\n> + if (skip_prefix(valp, \"left,\", &valp))\n> + align->align_type = ALIGN_LEFT;\n> + else if (skip_prefix(valp, \"right,\", &valp))\n> + align->align_type = ALIGN_RIGHT;\n> + else if (skip_prefix(valp, \"middle,\", &valp))\n> + align->align_type = ALIGN_MIDDLE;\n> + else\n> + die(_(\"align: improper format\"));\n\nYou could do a better job of helping the user track down the offending\n\"improper format\" by actually including it in the error message.\n\n> + if (strtoul_ui(valp, 10, &align->align_value))\n> + die(_(\"align: positive value expected\"));\n\nDitto.\n\n> + v->align = align;\n> + v->modifier_atom = 1;\n> + continue;\n> + } else if (starts_with(name, \"end\")) {\n> + v->end = 1;\n> + v->modifier_atom = 1;\n> + continue;\n> } else\n> continue;\n>\n> @@ -1251,12 +1277,48 @@ static void emit(const char *cp, const char *ep, struct ref_formatting_state *st\n> static void apply_formatting_state(struct ref_formatting_state *state, struct strbuf *final)\n> {\n> - /* More formatting options to be evetually added */\n> + if (state->align && state->end) {\n> + struct strbuf *value = state->output;\n> + int len = 0, buf_len = value->len;\n> + struct align *align = state->align;\n> +\n> + if (!value->buf)\n> + return;\n> + if (!is_utf8(value->buf)) {\n> + len = value->len - utf8_strwidth(value->buf);\n\nWhat is this doing, exactly? If the string is *not* utf-8, then you're\nasking it for its utf-8 width. Am I reading that correctly?\n\nAlso, what is 'len' supposed to represent? I guess you want it to be\nthe difference between the byte length and the display length, but the\nname 'len' doesn't convey that at all, nor does it help the reader\nunderstand the code below where you do the actual formatting.\n\nIn fact, if I'm reading this correctly, then 'len' is always zero in\nyour tests (because the tests never trigger this conditional), so this\nfunctionality is never being exercised.\n\n> + buf_len -= len;\n> + }\n> +\n> + if (align->align_value < buf_len) {\n\nShouldn't this be '<=' rather than '<'? There's no point going through\nthe formatting gyrations below if the string fills the alignment width\nexactly.\n\nAlso, what's the purpose of 'buf_len'? It's value is always\n(value->len - len), so you could just as easily say:\n\n if (align->align_value <= value->len - len) {\n\nIn fact, the misleading name 'len', along with 'buf_len' and\nvalue->len tend to make this code difficult to comprehend. If,\ninstead, you had a variable named 'display_len', then its meaning\nwould be obvious, and computations involving it would be more easily\nunderstood. The value of 'display_len' could be computed via:\n\n display_len = utf8_strnwidth(value->buf, value->len, 0);\n\nwhich would give you the utf-8 width if utf-8, or value->len if not.\n\nAnd, the above conditional would become the more readable:\n\n if (align->align_value <= display_len) {\n\nalthough, I find it easier to understand the logic with the condition\nswitched around:\n\n if (display_len >= align->align_value) {\n ...don't bother with alignment gyrations...\n\n(but that's just subjective)\n\n> + state->align = NULL;\n> + strbuf_addbuf(final, value);\n> + strbuf_release(value);\n> + return;\n> + }\n> +\n> + if (align->align_type == ALIGN_LEFT)\n> + strbuf_addf(final, \"%-*s\", len + align->align_value, value->buf);\n> + else if (align->align_type == ALIGN_MIDDLE) {\n> + int right = (align->align_value - buf_len)/2;\n> + strbuf_addf(final, \"%*s%-*s\", align->align_value - right + len,\n> + value->buf, right, \"\");\n\nWhy do you use the left-justifying format \"%-*s\" when interpolating\nthe zero-length string considering that \"%*s\" works just as well?\n\nAn aesthetic aside: When (align_value - buf_len) is an odd number,\nthis implementation favors placing more whitespace to the left of the\nstring, and less to the right. In practice, this often tends to look a\nbit more awkward than the inverse of placing more whitespace to the\nright, and less to the left (but that again is subjective).\n\n> + } else if (align->align_type == ALIGN_RIGHT)\n> + strbuf_addf(final, \"%*s\", align->align_value, value->buf);\n\nWhy doesn't this case need to take 'len' into account like the other cases?\n\nThis is a tangent, but I could easily see all of the code from 'if\n(align->align_value < buf_len)' down to this point being placed in\nutf8.c as a general alignment utility function. Doing so would make\nthis function shorter, and the patch easier to review overall (which\nmight be an important consideration -- especially given that I've\nalready spent several hours reviewing this one patch).\n\n> + strbuf_release(value);\n> + state->align = NULL;\n> + return;\n> + } else if (state->align)\n> + return;\n\nDo you expect additional types of state processing in the future? If\nso, this function is likely to get very long. It might make sense to\nbreak the alignment logic out into its own function which is called\nfrom this one.\n\n> strbuf_addbuf(final, state->output);\n> strbuf_release(state->output);\n> }\n> @@ -1301,6 +1372,7 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu\n> print_value(&resetv, &state);\n> apply_formatting_state(&state, &final_buf);\n> }\n> +\n\nSneaking in a whitespace change which an earlier patch perhaps should\nhave formatted correctly in the first place?\n\n> for (i = 0; i < final_buf.len; i++)\n> printf(\"%c\", final_buf.buf[i]);\n> putchar('\\n');\n> diff --git a/ref-filter.h b/ref-filter.h\n> index 9e6c2d4..5575fe9 100644\n> --- a/ref-filter.h\n> +++ b/ref-filter.h\n> @@ -16,14 +16,30 @@\n> struct ref_formatting_state {\n> - int quote_style;\n> struct strbuf *output;\n> + struct align *align;\n> + int quote_style;\n\nPerhaps decide where you'd like 'quote_style' to reside from the start\nso that you don't have to add it at one location in its introductory\npatch and then move it in a later patch. Also, why move it here?\n\n> + unsigned int end : 1;\n> +};\n> +\n> +struct align {\n> + align_type align_type;\n\nNo need for an \"align_\" prefix on the members of 'struct align'.\nThat's just unneeded verbosity.\n\nAs mentioned above, 'type' may not be the best name. Perhaps 'style'\nor 'position' or something better.\n\n> + unsigned int align_value;\n\nDitto. 'value' doesn't say much, whereas 'width' or 'fieldwidth' would\nbe more meaningful.\n\n> };\n> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\n> index 505a360..76041a2 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> test_cmp expect actual\n> '\n>\n> +test_expect_success 'left alignment' '\n> + cat >expect <<-\\EOF &&\n> + refname is refs/heads/master |refs/heads/master\n> + refname is refs/heads/side |refs/heads/side\n> + refname is refs/odd/spot |refs/odd/spot\n> + refname is refs/tags/double-tag|refs/tags/double-tag\n> + refname is refs/tags/four |refs/tags/four\n> + refname is refs/tags/one |refs/tags/one\n> + refname is refs/tags/signed-tag|refs/tags/signed-tag\n> + refname is refs/tags/three |refs/tags/three\n> + refname is refs/tags/two |refs/tags/two\n> + EOF\n> + git for-each-ref --format=\"%(align:left,30)refname is %(refname)%(end)|%(refname)\" >actual &&\n> + test_cmp expect actual\n> +'\n\nAlluding to a previous review comment, for this left-alignment test,\nperhaps add a third column to prove that the %(align:) atom isn't\nleaking from column 1 to column 2 since it's not obvious to the\nreader, given that trailing whitespace is not otherwise visible.\n\n> +test_expect_success 'middle alignment' '\n> + cat >expect <<-\\EOF &&\n> + | refname is refs/heads/master |refs/heads/master\n> + | refname is refs/heads/side |refs/heads/side\n> + | refname is refs/odd/spot |refs/odd/spot\n> + |refname is refs/tags/double-tag|refs/tags/double-tag\n> + | refname is refs/tags/four |refs/tags/four\n> + | refname is refs/tags/one |refs/tags/one\n> + |refname is refs/tags/signed-tag|refs/tags/signed-tag\n> + | refname is refs/tags/three |refs/tags/three\n> + | refname is refs/tags/two |refs/tags/two\n> + EOF\n> + git for-each-ref --format=\"|%(align:middle,30)refname is %(refname)%(end)|%(refname)\" >actual &&\n> + test_cmp expect actual\n> +'\n> +\n> +test_expect_success 'right alignment' '\n> + cat >expect <<-\\EOF &&\n> + | refname is refs/heads/master|refs/heads/master\n> + | refname is refs/heads/side|refs/heads/side\n> + | refname is refs/odd/spot|refs/odd/spot\n> + |refname is refs/tags/double-tag|refs/tags/double-tag\n> + | refname is refs/tags/four|refs/tags/four\n> + | refname is refs/tags/one|refs/tags/one\n> + |refname is refs/tags/signed-tag|refs/tags/signed-tag\n> + | refname is refs/tags/three|refs/tags/three\n> + | refname is refs/tags/two|refs/tags/two\n> + EOF\n> + git for-each-ref --format=\"|%(align:right,30)refname is %(refname)%(end)|%(refname)\" >actual &&\n> + test_cmp expect actual\n> +'\n> +\n> test_done\n> --\n> 2.5.0\n"},{"id":"267603","messageId":"CAPig+cTOzZ0RFd2eu43rYQDnsXmfswbRx3=vN41BJf_BHON7tA@mail.gmail.com","threadId":"40021","inReplyTo":"1438800854-19901-1-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH v9 03/11] ref-filter: implement an `align` atom","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-07T04:04:38Z","receivedAt":"2015-08-07T04:04:38Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Aug 5, 2015 at 2:54 PM, Karthik Nayak wrote:\n> Implement an `align` atom which will act as a modifier atom and align\n> any string with or without an %(atom) appearing before a %(end) atom\n> to the right, left or middle.\n>\n> It is followed by `:,`, where the `` is\n> either left, right or middle and `` is the total length\n> of the padding to be performed. If the atom length is more than the\n> padding length then no padding is performed. e.g. to pad a succeeding\n> atom to the middle with a total padding size of 40 we can do a\n> --format=\"%(align:middle,40)..\"\n>\n> Add documentation and tests for the same.\n\nI forgot to mention in my earlier review of this patch that you should\nexplain in the commit message, and probably the documentation, this\nthis implementation (assuming I'm understanding the code) does not\ncorrectly support nested %(foo)...%(end) constructs, where %(foo)\nmight be %(if:), %(truncate:), %(cut:), or even a nested %(align:), or\nsome as yet unimagined modifier. Supporting nesting of these\nconstructs will require pushing the formatting states onto a stack (or\ninvoking the parser recursively).\n\n> Signed-off-by: Karthik Nayak \n"},{"id":"267651","messageId":"CAOLa=ZSkmkPpiEfDrRXNQ6Tz5GT1+7vef3TOrj1t9aZ_3wm2Lw@mail.gmail.com","threadId":"40021","inReplyTo":"CAPig+cSTssDihF5cBwu=2uKX1y6GqH-5EhKnb92Fpp30JA7pwA@mail.gmail.com","subject":"Re: [PATCH v9 03/11] ref-filter: implement an `align` atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-08T06:35:24Z","receivedAt":"2015-08-08T06:35:24Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Fri, Aug 7, 2015 at 8:57 AM, Eric Sunshine wrote:\n> On Wed, Aug 5, 2015 at 2:54 PM, Karthik Nayak wrote:\n>> Implement an `align` atom which will act as a modifier atom and align\n>> any string with or without an %(atom) appearing before a %(end) atom\n>> to the right, left or middle.\n>\n> For someone not familiar with the evolution of this patch series,\n> \"align any string with or without an %(atom)\" is confusing and\n> distracting and seems to imply that something extra mysterious is\n> going on behind the scenes. Keeping it simple makes it easier to\n> understand:\n>\n> Add an `align` atom which left-, middle-, or right-aligns the\n> content between %(align:...) and %(end).\n>\n\nOk will change this.\n\n>> It is followed by `:,`, where the `` is\n>\n> 'type' may not be the best name. Perhaps 'style' or 'position' or\n> something else would be better?\n>\n\nposition is a better term.\n\n>> either left, right or middle and `` is the total length\n>\n> 'paddinglength' is misleading. You're really describing the container\n> width in which alignment takes places, so perhaps call it 'width' or\n> 'fieldwidth' or something.\n>\n\nwidth seems good to go.\n\n>> of the padding to be performed. If the atom length is more than the\n>> padding length then no padding is performed. e.g. to pad a succeeding\n>> atom to the middle with a total padding size of 40 we can do a\n>\n> It's odd to have alignment described in terms of \"padding\" and\n> \"padding length\", especially in the case of \"center\" alignment. It\n> might be better to rephrase the discussion in terms of field width or\n> such.\n>\n>> --format=\"%(align:middle,40)..\"\n\nOk this makes sense,\nI'll rephrase as :\n\n`` is the total length of the content with alignment.\nIf the atom length is more than the width then no alignment is performed.\ne.g. to align a succeeding atom to the middle with a total width of 40\nwe can do:\n--format=\"%(align:middle,40)..\"\n\n>\n> I may have missed the discussion, but why was comma chosen as a\n> separator here, rather than, say, colon? For instance:\n>\n> %(align:middle:40)\n>\n\nI think it's based of this:\nhttp://thread.gmane.org/gmane.comp.version-control.git/275119\n\n>> Add documentation and tests for the same.\n>>\n>> Signed-off-by: Karthik Nayak \n>> ---\n>> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\n>> index e49d578..d865f98 100644\n>> --- a/Documentation/git-for-each-ref.txt\n>> +++ b/Documentation/git-for-each-ref.txt\n>> @@ -127,6 +127,14 @@ color::\n>> +align::\n>> + Align any string with or without %(atom) before the %(end)\n>> + atom to the right, left or middle. Followed by\n>\n> Ditto regarding \"any string with or without %(atom)\" being confusing\n> to someone not familiar with the evolution of this patch series.\n> Instead, perhaps:\n>\n> Left-, middle-, or right-align content between %(align:...)\n> and %(end).\n>\n\nWill change as per change in the commit message.\n\n>> + `:,`, where the `` is either left,\n>> + right or middle and `` is the total length of\n>> + the padding to be performed. If the string length is more than\n>> + the padding length then no padding is performed.\n>\n> Ditto regarding above observations about 'type', 'paddinglength', and \"padding\".\n>\n\nNoted.\n\n>> diff --git a/ref-filter.c b/ref-filter.c\n>> index 2c074a1..d123299 100644\n>> --- a/ref-filter.c\n>> +++ b/ref-filter.c\n>> @@ -620,7 +623,7 @@ static void populate_value(struct ref_array_item *ref)\n>> const char *name = used_atom[i];\n>> struct atom_value *v = &ref->value[i];\n>> int deref = 0;\n>> - const char *refname;\n>> + const char *refname = NULL;\n>\n> What is this change about?\n>\n\nWas to remove the compiler error, can be removed now.\n\n>> const char *formatp;\n>> struct branch *branch = NULL;\n>>\n>> @@ -687,6 +690,29 @@ static void populate_value(struct ref_array_item *ref)\n>> else\n>> v->s = \" \";\n>> continue;\n>> + } else if (starts_with(name, \"align:\")) {\n>> + const char *valp = NULL;\n>\n> Unnecessary NULL assignment.\n>\n\nThats required for the second skip_prefix and so on.\nElse we get: \"warning: ‘valp’ may be used uninitialized in this\nfunction [-Wmaybe-uninitialized]\"\n\n>> + struct align *align = xmalloc(sizeof(struct align));\n>> +\n>> + skip_prefix(name, \"align:\", &valp);\n>\n> You could simplify the code by combining this skip_prefix() with the\n> earlier starts_with() in the conditional:\n>\n> } else if (skip_prefix(name, \"align:\", &valp)) {\n> struct align *align = xmalloc(sizeof(struct align));\n> ...\n>\n\nThat would require valp to be previously defined. Hence the split.\n\n>> + if (skip_prefix(valp, \"left,\", &valp))\n>> + align->align_type = ALIGN_LEFT;\n>> + else if (skip_prefix(valp, \"right,\", &valp))\n>> + align->align_type = ALIGN_RIGHT;\n>> + else if (skip_prefix(valp, \"middle,\", &valp))\n>> + align->align_type = ALIGN_MIDDLE;\n>> + else\n>> + die(_(\"align: improper format\"));\n>\n> You could do a better job of helping the user track down the offending\n> \"improper format\" by actually including it in the error message.\n>\n\nOk will do.\n\n>> + if (strtoul_ui(valp, 10, &align->align_value))\n>> + die(_(\"align: positive value expected\"));\n>\n> Ditto.\n>\n\nWill change.\n\n>> + v->align = align;\n>> + v->modifier_atom = 1;\n>> + continue;\n>> + } else if (starts_with(name, \"end\")) {\n>> + v->end = 1;\n>> + v->modifier_atom = 1;\n>> + continue;\n>> } else\n>> continue;\n>>\n>> @@ -1251,12 +1277,48 @@ static void emit(const char *cp, const char *ep, struct ref_formatting_state *st\n>> static void apply_formatting_state(struct ref_formatting_state *state, struct strbuf *final)\n>> {\n>> - /* More formatting options to be evetually added */\n>> + if (state->align && state->end) {\n>> + struct strbuf *value = state->output;\n>> + int len = 0, buf_len = value->len;\n>> + struct align *align = state->align;\n>> +\n>> + if (!value->buf)\n>> + return;\n>> + if (!is_utf8(value->buf)) {\n>> + len = value->len - utf8_strwidth(value->buf);\n>\n> What is this doing, exactly? If the string is *not* utf-8, then you're\n> asking it for its utf-8 width. Am I reading that correctly?\n>\n> Also, what is 'len' supposed to represent? I guess you want it to be\n> the difference between the byte length and the display length, but the\n> name 'len' doesn't convey that at all, nor does it help the reader\n> understand the code below where you do the actual formatting.\n>\n> In fact, if I'm reading this correctly, then 'len' is always zero in\n> your tests (because the tests never trigger this conditional), so this\n> functionality is never being exercised.\n>\n\nThere shouldn't be a \"!\" there, will change.\nI guess 'utf8_compensation' would be a better name.\n\n>> + buf_len -= len;\n>> + }\n>> +\n>> + if (align->align_value < buf_len) {\n>\n> Shouldn't this be '<=' rather than '<'? There's no point going through\n> the formatting gyrations below if the string fills the alignment width\n> exactly.\n>\n\nGood point.\n\n> Also, what's the purpose of 'buf_len'? It's value is always\n> (value->len - len), so you could just as easily say:\n>\n> if (align->align_value <= value->len - len) {\n>\n> In fact, the misleading name 'len', along with 'buf_len' and\n> value->len tend to make this code difficult to comprehend. If,\n> instead, you had a variable named 'display_len', then its meaning\n> would be obvious, and computations involving it would be more easily\n> understood. The value of 'display_len' could be computed via:\n>\n> display_len = utf8_strnwidth(value->buf, value->len, 0);\n\nThis is brilliant, would also remove the if statement for checking if it's\nutf8.\n\n>\n> which would give you the utf-8 width if utf-8, or value->len if not.\n>\n> And, the above conditional would become the more readable:\n>\n> if (align->align_value <= display_len) {\n>\n> although, I find it easier to understand the logic with the condition\n> switched around:\n>\n> if (display_len >= align->align_value) {\n> ...don't bother with alignment gyrations...\n>\n> (but that's just subjective)\n>\n\nYea with the name diplay_len that'd make sense, as you're saying is the length\nis greater than alignment value, don't bother with it.\n\n>> + state->align = NULL;\n>> + strbuf_addbuf(final, value);\n>> + strbuf_release(value);\n>> + return;\n>> + }\n>> +\n>> + if (align->align_type == ALIGN_LEFT)\n>> + strbuf_addf(final, \"%-*s\", len + align->align_value, value->buf);\n>> + else if (align->align_type == ALIGN_MIDDLE) {\n>> + int right = (align->align_value - buf_len)/2;\n>> + strbuf_addf(final, \"%*s%-*s\", align->align_value - right + len,\n>> + value->buf, right, \"\");\n>\n> Why do you use the left-justifying format \"%-*s\" when interpolating\n> the zero-length string considering that \"%*s\" works just as well?\n>\n\nYes that also would work.\n\n> An aesthetic aside: When (align_value - buf_len) is an odd number,\n> this implementation favors placing more whitespace to the left of the\n> string, and less to the right. In practice, this often tends to look a\n> bit more awkward than the inverse of placing more whitespace to the\n> right, and less to the left (but that again is subjective).\n>\n\nI know that, maybe we could add an additional padding to even out the value\ngiven?\n\n>> + } else if (align->align_type == ALIGN_RIGHT)\n>> + strbuf_addf(final, \"%*s\", align->align_value, value->buf);\n>\n> Why doesn't this case need to take 'len' into account like the other cases?\n>\n\nThis needs to be changed.\n\n> This is a tangent, but I could easily see all of the code from 'if\n> (align->align_value < buf_len)' down to this point being placed in\n> utf8.c as a general alignment utility function. Doing so would make\n> this function shorter, and the patch easier to review overall (which\n> might be an important consideration -- especially given that I've\n> already spent several hours reviewing this one patch).\n>\n\nThat's a valid suggestion, will do that, thanks, but that'd mean we need to\nsend an align struct to utf8.c which is only defined in ref-filter.h.\nEither this\nis fine or we need to move the definition of struct align to utf8.h.\n\nI think personally move the align structure and enum over to utf8.h\n\n>> + strbuf_release(value);\n>> + state->align = NULL;\n>> + return;\n>> + } else if (state->align)\n>> + return;\n>\n> Do you expect additional types of state processing in the future? If\n> so, this function is likely to get very long. It might make sense to\n> break the alignment logic out into its own function which is called\n> from this one.\n>\n\nMakes sense, will do.\n\n>> strbuf_addbuf(final, state->output);\n>> strbuf_release(state->output);\n>> }\n>> @@ -1301,6 +1372,7 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu\n>> print_value(&resetv, &state);\n>> apply_formatting_state(&state, &final_buf);\n>> }\n>> +\n>\n> Sneaking in a whitespace change which an earlier patch perhaps should\n> have formatted correctly in the first place?\n>\n\nwill change.\n\n>> for (i = 0; i < final_buf.len; i++)\n>> printf(\"%c\", final_buf.buf[i]);\n>> putchar('\\n');\n>> diff --git a/ref-filter.h b/ref-filter.h\n>> index 9e6c2d4..5575fe9 100644\n>> --- a/ref-filter.h\n>> +++ b/ref-filter.h\n>> @@ -16,14 +16,30 @@\n>> struct ref_formatting_state {\n>> - int quote_style;\n>> struct strbuf *output;\n>> + struct align *align;\n>> + int quote_style;\n>\n> Perhaps decide where you'd like 'quote_style' to reside from the start\n> so that you don't have to add it at one location in its introductory\n> patch and then move it in a later patch. Also, why move it here?\n>\n\nCause that'd save memory on a 64 bit processor, where the pointers would\nbe 8 bytes long and int would be 4 bytes long, this would bring in padding if\nint is placed before the pointers. Will change before hand.\n\n>> + unsigned int end : 1;\n>> +};\n>> +\n>> +struct align {\n>> + align_type align_type;\n>\n> No need for an \"align_\" prefix on the members of 'struct align'.\n> That's just unneeded verbosity.\n>\n\nNoted, will change.\n\n> As mentioned above, 'type' may not be the best name. Perhaps 'style'\n> or 'position' or something better.\n>\n\nPosition seems better choice, will change that throughout.\n\n>> + unsigned int align_value;\n>\n> Ditto. 'value' doesn't say much, whereas 'width' or 'fieldwidth' would\n> be more meaningful.\n>\n\nwidth seems good.\n\n>> };\n>> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\n>> index 505a360..76041a2 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>> test_cmp expect actual\n>> '\n>>\n>> +test_expect_success 'left alignment' '\n>> + cat >expect <<-\\EOF &&\n>> + refname is refs/heads/master |refs/heads/master\n>> + refname is refs/heads/side |refs/heads/side\n>> + refname is refs/odd/spot |refs/odd/spot\n>> + refname is refs/tags/double-tag|refs/tags/double-tag\n>> + refname is refs/tags/four |refs/tags/four\n>> + refname is refs/tags/one |refs/tags/one\n>> + refname is refs/tags/signed-tag|refs/tags/signed-tag\n>> + refname is refs/tags/three |refs/tags/three\n>> + refname is refs/tags/two |refs/tags/two\n>> + EOF\n>> + git for-each-ref --format=\"%(align:left,30)refname is %(refname)%(end)|%(refname)\" >actual &&\n>> + test_cmp expect actual\n>> +'\n>\n> Alluding to a previous review comment, for this left-alignment test,\n> perhaps add a third column to prove that the %(align:) atom isn't\n> leaking from column 1 to column 2 since it's not obvious to the\n> reader, given that trailing whitespace is not otherwise visible.\n>\n\nWell its kinda pointless here cause you need an %(end) atom for alignment,\nso if there was a possible leak its hard to tell where the leak would\nextend till\nas we need an %(end) atom to process alignment.\n\n>> +test_expect_success 'middle alignment' '\n>> + cat >expect <<-\\EOF &&\n>> + | refname is refs/heads/master |refs/heads/master\n>> + | refname is refs/heads/side |refs/heads/side\n>> + | refname is refs/odd/spot |refs/odd/spot\n>> + |refname is refs/tags/double-tag|refs/tags/double-tag\n>> + | refname is refs/tags/four |refs/tags/four\n>> + | refname is refs/tags/one |refs/tags/one\n>> + |refname is refs/tags/signed-tag|refs/tags/signed-tag\n>> + | refname is refs/tags/three |refs/tags/three\n>> + | refname is refs/tags/two |refs/tags/two\n>> + EOF\n>> + git for-each-ref --format=\"|%(align:middle,30)refname is %(refname)%(end)|%(refname)\" >actual &&\n>> + test_cmp expect actual\n>> +'\n>> +\n>> +test_expect_success 'right alignment' '\n>> + cat >expect <<-\\EOF &&\n>> + | refname is refs/heads/master|refs/heads/master\n>> + | refname is refs/heads/side|refs/heads/side\n>> + | refname is refs/odd/spot|refs/odd/spot\n>> + |refname is refs/tags/double-tag|refs/tags/double-tag\n>> + | refname is refs/tags/four|refs/tags/four\n>> + | refname is refs/tags/one|refs/tags/one\n>> + |refname is refs/tags/signed-tag|refs/tags/signed-tag\n>> + | refname is refs/tags/three|refs/tags/three\n>> + | refname is refs/tags/two|refs/tags/two\n>> + EOF\n>> + git for-each-ref --format=\"|%(align:right,30)refname is %(refname)%(end)|%(refname)\" >actual &&\n>> + test_cmp expect actual\n>> +'\n>> +\n>> test_done\n>> --\n>> 2.5.0\n\n\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"267652","messageId":"CAOLa=ZT+hCgM+mL2vQknwUjaxG9Mr4C2Qir=aa==Fv9CebYwLg@mail.gmail.com","threadId":"40021","inReplyTo":"CAPig+cTOzZ0RFd2eu43rYQDnsXmfswbRx3=vN41BJf_BHON7tA@mail.gmail.com","subject":"Re: [PATCH v9 03/11] ref-filter: implement an `align` atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-08T07:03:03Z","receivedAt":"2015-08-08T07:03:03Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Fri, Aug 7, 2015 at 9:34 AM, Eric Sunshine wrote:\n> On Wed, Aug 5, 2015 at 2:54 PM, Karthik Nayak wrote:\n>> Implement an `align` atom which will act as a modifier atom and align\n>> any string with or without an %(atom) appearing before a %(end) atom\n>> to the right, left or middle.\n>>\n>> It is followed by `:,`, where the `` is\n>> either left, right or middle and `` is the total length\n>> of the padding to be performed. If the atom length is more than the\n>> padding length then no padding is performed. e.g. to pad a succeeding\n>> atom to the middle with a total padding size of 40 we can do a\n>> --format=\"%(align:middle,40)..\"\n>>\n>> Add documentation and tests for the same.\n>\n> I forgot to mention in my earlier review of this patch that you should\n> explain in the commit message, and probably the documentation, this\n> this implementation (assuming I'm understanding the code) does not\n> correctly support nested %(foo)...%(end) constructs, where %(foo)\n> might be %(if:), %(truncate:), %(cut:), or even a nested %(align:), or\n> some as yet unimagined modifier. Supporting nesting of these\n> constructs will require pushing the formatting states onto a stack (or\n> invoking the parser recursively).\n>\n>> Signed-off-by: Karthik Nayak \n\nGood point, I have been working on this parallely and it works for now,\nI'll send that with the %(if) and %(end) feature. But for now, it should be\ndocumented and added in the commit message.\n\nUsing a linked list of sorts where whenever a new modifier atom is encountered\na new state is created, and once %(end) is encountered we can pop that state\ninto the previous state.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"267666","messageId":"CAPig+cTHKbn0oCV61n=p5o9WihsaJbvWqKt4y9eFwA0noJoPgA@mail.gmail.com","threadId":"40021","inReplyTo":"CAOLa=ZSkmkPpiEfDrRXNQ6Tz5GT1+7vef3TOrj1t9aZ_3wm2Lw@mail.gmail.com","subject":"Re: [PATCH v9 03/11] ref-filter: implement an `align` atom","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-09T03:42:09Z","receivedAt":"2015-08-09T03:42:09Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Aug 8, 2015 at 2:35 AM, Karthik Nayak wrote:\n> On Fri, Aug 7, 2015 at 8:57 AM, Eric Sunshine wrote:\n>> On Wed, Aug 5, 2015 at 2:54 PM, Karthik Nayak wrote:\n>>> of the padding to be performed. If the atom length is more than the\n>>> padding length then no padding is performed. e.g. to pad a succeeding\n>>> atom to the middle with a total padding size of 40 we can do a\n>>\n>> It's odd to have alignment described in terms of \"padding\" and\n>> \"padding length\", especially in the case of \"center\" alignment. It\n>> might be better to rephrase the discussion in terms of field width or\n>> such.\n>>\n>>> --format=\"%(align:middle,40)..\"\n>\n> Ok this makes sense,\n> I'll rephrase as :\n>\n> `` is the total length of the content with alignment.\n\nThis doesn't really make sense. isn't the content length; it's\nthe size of the area into which the content will be placed.\n\n> If the atom length is more than the width then no alignment is performed.\n\nWhat \"atom\"? I think you mean the content between %(align:) and %(end)\nrather than \"atom\". The description might be clearer if you actually\nsay \"content between %(align:) and %(end)\" to indicate specifically\nwhat is being aligned.\n\n> e.g. to align a succeeding atom to the middle with a total width of 40\n> we can do:\n> --format=\"%(align:middle,40)..\"\n>>> @@ -687,6 +690,29 @@ static void populate_value(struct ref_array_item *ref)\n>>> else\n>>> v->s = \" \";\n>>> continue;\n>>> + } else if (starts_with(name, \"align:\")) {\n>>> + const char *valp = NULL;\n>>\n>> Unnecessary NULL assignment.\n>\n> Thats required for the second skip_prefix and so on.\n> Else we get: \"warning: ‘valp’ may be used uninitialized in this\n> function [-Wmaybe-uninitialized]\"\n\nOkay, so that's because skip_prefix() is inline, and it doesn't touch\nits \"out\" argument unless it actually skips the prefix. Ugly, but\nmakes sense, although I think this issue would go away if you combined\nthe starts_with() and skips_prefix() as suggested earlier.\n\n>>> + struct align *align = xmalloc(sizeof(struct align));\n>>> +\n>>> + skip_prefix(name, \"align:\", &valp);\n>>\n>> You could simplify the code by combining this skip_prefix() with the\n>> earlier starts_with() in the conditional:\n>>\n>> } else if (skip_prefix(name, \"align:\", &valp)) {\n>> struct align *align = xmalloc(sizeof(struct align));\n>> ...\n>\n> That would require valp to be previously defined. Hence the split.\n\nYes, it would require declaring 'valp' earlier, but that seems a\nreasonable tradeoff for cleaner, simpler, less redundant code.\n\n>>> static void apply_formatting_state(struct ref_formatting_state *state, struct strbuf *final)\n>>> {\n>>> - /* More formatting options to be evetually added */\n>>> + if (state->align && state->end) {\n>>> + struct strbuf *value = state->output;\n>>> + int len = 0, buf_len = value->len;\n>>> + struct align *align = state->align;\n>>> +\n>>> + if (!value->buf)\n>>> + return;\n>>> + if (!is_utf8(value->buf)) {\n>>> + len = value->len - utf8_strwidth(value->buf);\n>>\n>> What is this doing, exactly? If the string is *not* utf-8, then you're\n>> asking it for its utf-8 width. Am I reading that correctly?\n>>\n>> Also, what is 'len' supposed to represent? I guess you want it to be\n>> the difference between the byte length and the display length, but the\n>> name 'len' doesn't convey that at all, nor does it help the reader\n>> understand the code below where you do the actual formatting.\n>>\n>> In fact, if I'm reading this correctly, then 'len' is always zero in\n>> your tests (because the tests never trigger this conditional), so this\n>> functionality is never being exercised.\n>\n> There shouldn't be a \"!\" there, will change.\n> I guess 'utf8_compensation' would be a better name.\n\nDefinitely better than 'len'.\n\n>>> + else if (align->align_type == ALIGN_MIDDLE) {\n>>> + int right = (align->align_value - buf_len)/2;\n>>> + strbuf_addf(final, \"%*s%-*s\", align->align_value - right + len,\n>>> + value->buf, right, \"\");\n>>\n>> An aesthetic aside: When (align_value - buf_len) is an odd number,\n>> this implementation favors placing more whitespace to the left of the\n>> string, and less to the right. In practice, this often tends to look a\n>> bit more awkward than the inverse of placing more whitespace to the\n>> right, and less to the left (but that again is subjective).\n>\n> I know that, maybe we could add an additional padding to even out the value\n> given?\n\nI don't understand your question. I was merely suggesting (purely\nsubjectively), for the \"odd length\" case, putting the extra space\nafter the centered text rather than before it. For instance:\n\n int left = (align->align_value - buf_len) / 2;\n strbuf_addf(final, \"%*s%-*s\", left, \"\",\n align->align_value - left + len, value->buf);\n\nor any similar variation which would give the same result.\n\n>> This is a tangent, but I could easily see all of the code from 'if\n>> (align->align_value < buf_len)' down to this point being placed in\n>> utf8.c as a general alignment utility function. Doing so would make\n>> this function shorter, and the patch easier to review overall (which\n>> might be an important consideration -- especially given that I've\n>> already spent several hours reviewing this one patch).\n>\n> That's a valid suggestion, will do that, thanks, but that'd mean we need to\n> send an align struct to utf8.c which is only defined in ref-filter.h.\n> Either this\n> is fine or we need to move the definition of struct align to utf8.h.\n> I think personally move the align structure and enum over to utf8.h\n\nNo, you don't need to move the 'struct align' to utf8.h. That\nstructure is specific to ref-filter and should stay there. Instead,\nyou only need to move the enum. For instance, you'd add something like\nthis to utf8.h:\n\n enum utf8_alignment {\n ALIGN_LEFT,\n ALIGN_MIDDLE,\n ALIGN_RIGHT\n };\n\n void strbuf_utf8_align(struct strbuf *buf,\n utf8_alignment where, int width, const char *s);\n\nBy the way, I forgot to say earlier that this should be done as a\nseparate patch (in order to make the current patch smaller).\n\nThat raises another question. Why are 'struct ref_formatting_state',\n'struct align', 'struct atom_value', etc. defined in ref-filter.h at\nall? Aren't those private implementation details of ref-filter.c, or\ndo you expect other code to be using them?\n\n>>> for (i = 0; i < final_buf.len; i++)\n>>> printf(\"%c\", final_buf.buf[i]);\n>>> putchar('\\n');\n>>> diff --git a/ref-filter.h b/ref-filter.h\n>>> index 9e6c2d4..5575fe9 100644\n>>> --- a/ref-filter.h\n>>> +++ b/ref-filter.h\n>>> @@ -16,14 +16,30 @@\n>>> struct ref_formatting_state {\n>>> - int quote_style;\n>>> struct strbuf *output;\n>>> + struct align *align;\n>>> + int quote_style;\n>>\n>> Perhaps decide where you'd like 'quote_style' to reside from the start\n>> so that you don't have to add it at one location in its introductory\n>> patch and then move it in a later patch. Also, why move it here?\n>\n> Cause that'd save memory on a 64 bit processor, where the pointers would\n> be 8 bytes long and int would be 4 bytes long, this would bring in padding if\n> int is placed before the pointers. Will change before hand.\n\nAs I understand it, you're not likely to have many\nref_fomratting_state's around at any given time, so this sounds like\npremature memory micro-optimization.\n"},{"id":"267668","messageId":"CAOLa=ZQW1HBACAb4V_jMky4=54=rxqS=nyYMBX1CUUvA2c0s9A@mail.gmail.com","threadId":"40021","inReplyTo":"CAPig+cTHKbn0oCV61n=p5o9WihsaJbvWqKt4y9eFwA0noJoPgA@mail.gmail.com","subject":"Re: [PATCH v9 03/11] ref-filter: implement an `align` atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-09T06:55:52Z","receivedAt":"2015-08-09T06:55:52Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Sun, Aug 9, 2015 at 9:12 AM, Eric Sunshine wrote:\n> On Sat, Aug 8, 2015 at 2:35 AM, Karthik Nayak wrote:\n>> On Fri, Aug 7, 2015 at 8:57 AM, Eric Sunshine wrote:\n>>> On Wed, Aug 5, 2015 at 2:54 PM, Karthik Nayak wrote:\n>>>> of the padding to be performed. If the atom length is more than the\n>>>> padding length then no padding is performed. e.g. to pad a succeeding\n>>>> atom to the middle with a total padding size of 40 we can do a\n>>>\n>>> It's odd to have alignment described in terms of \"padding\" and\n>>> \"padding length\", especially in the case of \"center\" alignment. It\n>>> might be better to rephrase the discussion in terms of field width or\n>>> such.\n>>>\n>>>> --format=\"%(align:middle,40)..\"\n>>\n>> Ok this makes sense,\n>> I'll rephrase as :\n>>\n>> `` is the total length of the content with alignment.\n>\n> This doesn't really make sense. isn't the content length; it's\n> the size of the area into which the content will be placed.\n>\n\nWill change this.\n\n>> If the atom length is more than the width then no alignment is performed.\n>\n> What \"atom\"? I think you mean the content between %(align:) and %(end)\n> rather than \"atom\". The description might be clearer if you actually\n> say \"content between %(align:) and %(end)\" to indicate specifically\n> what is being aligned.\n\nYes, that's what I meant.\n\n>\n>> e.g. to align a succeeding atom to the middle with a total width of 40\n>> we can do:\n>> --format=\"%(align:middle,40)..\"\n>>>> @@ -687,6 +690,29 @@ static void populate_value(struct ref_array_item *ref)\n>>>> else\n>>>> v->s = \" \";\n>>>> continue;\n>>>> + } else if (starts_with(name, \"align:\")) {\n>>>> + const char *valp = NULL;\n>>>\n>>> Unnecessary NULL assignment.\n>>\n>> Thats required for the second skip_prefix and so on.\n>> Else we get: \"warning: ‘valp’ may be used uninitialized in this\n>> function [-Wmaybe-uninitialized]\"\n>\n> Okay, so that's because skip_prefix() is inline, and it doesn't touch\n> its \"out\" argument unless it actually skips the prefix. Ugly, but\n> makes sense, although I think this issue would go away if you combined\n> the starts_with() and skips_prefix() as suggested earlier.\n>\n\nOkay then I'll declare valp prehand to get rid of this and also to\nremove redundant, starts_with() and skip_prefix().\n\n>>>> + struct align *align = xmalloc(sizeof(struct align));\n>>>> +\n>>>> + skip_prefix(name, \"align:\", &valp);\n>>>\n>>> You could simplify the code by combining this skip_prefix() with the\n>>> earlier starts_with() in the conditional:\n>>>\n>>> } else if (skip_prefix(name, \"align:\", &valp)) {\n>>> struct align *align = xmalloc(sizeof(struct align));\n>>> ...\n>>\n>> That would require valp to be previously defined. Hence the split.\n>\n> Yes, it would require declaring 'valp' earlier, but that seems a\n> reasonable tradeoff for cleaner, simpler, less redundant code.\n>\n\nYes. will do this.\n\n>>>> static void apply_formatting_state(struct ref_formatting_state *state, struct strbuf *final)\n>>>> {\n>>>> - /* More formatting options to be evetually added */\n>>>> + if (state->align && state->end) {\n>>>> + struct strbuf *value = state->output;\n>>>> + int len = 0, buf_len = value->len;\n>>>> + struct align *align = state->align;\n>>>> +\n>>>> + if (!value->buf)\n>>>> + return;\n>>>> + if (!is_utf8(value->buf)) {\n>>>> + len = value->len - utf8_strwidth(value->buf);\n>>>\n>>> What is this doing, exactly? If the string is *not* utf-8, then you're\n>>> asking it for its utf-8 width. Am I reading that correctly?\n>>>\n>>> Also, what is 'len' supposed to represent? I guess you want it to be\n>>> the difference between the byte length and the display length, but the\n>>> name 'len' doesn't convey that at all, nor does it help the reader\n>>> understand the code below where you do the actual formatting.\n>>>\n>>> In fact, if I'm reading this correctly, then 'len' is always zero in\n>>> your tests (because the tests never trigger this conditional), so this\n>>> functionality is never being exercised.\n>>\n>> There shouldn't be a \"!\" there, will change.\n>> I guess 'utf8_compensation' would be a better name.\n>\n> Definitely better than 'len'.\n>\n>>>> + else if (align->align_type == ALIGN_MIDDLE) {\n>>>> + int right = (align->align_value - buf_len)/2;\n>>>> + strbuf_addf(final, \"%*s%-*s\", align->align_value - right + len,\n>>>> + value->buf, right, \"\");\n>>>\n>>> An aesthetic aside: When (align_value - buf_len) is an odd number,\n>>> this implementation favors placing more whitespace to the left of the\n>>> string, and less to the right. In practice, this often tends to look a\n>>> bit more awkward than the inverse of placing more whitespace to the\n>>> right, and less to the left (but that again is subjective).\n>>\n>> I know that, maybe we could add an additional padding to even out the value\n>> given?\n>\n> I don't understand your question. I was merely suggesting (purely\n> subjectively), for the \"odd length\" case, putting the extra space\n> after the centered text rather than before it. For instance:\n>\n> int left = (align->align_value - buf_len) / 2;\n> strbuf_addf(final, \"%*s%-*s\", left, \"\",\n> align->align_value - left + len, value->buf);\n>\n> or any similar variation which would give the same result.\n>\n\nI get this could be done, what I was asking was, Consider given a alignment\nwidth of 25 would be better to make that 26 so that we have even padding on\nboth sides. But I don't like the adding of manipulating user given data.\n\n>>> This is a tangent, but I could easily see all of the code from 'if\n>>> (align->align_value < buf_len)' down to this point being placed in\n>>> utf8.c as a general alignment utility function. Doing so would make\n>>> this function shorter, and the patch easier to review overall (which\n>>> might be an important consideration -- especially given that I've\n>>> already spent several hours reviewing this one patch).\n>>\n>> That's a valid suggestion, will do that, thanks, but that'd mean we need to\n>> send an align struct to utf8.c which is only defined in ref-filter.h.\n>> Either this\n>> is fine or we need to move the definition of struct align to utf8.h.\n>> I think personally move the align structure and enum over to utf8.h\n>\n> No, you don't need to move the 'struct align' to utf8.h. That\n> structure is specific to ref-filter and should stay there. Instead,\n> you only need to move the enum. For instance, you'd add something like\n> this to utf8.h:\n>\n> enum utf8_alignment {\n> ALIGN_LEFT,\n> ALIGN_MIDDLE,\n> ALIGN_RIGHT\n> };\n>\n> void strbuf_utf8_align(struct strbuf *buf,\n> utf8_alignment where, int width, const char *s);\n>\n\nOkay will do this.\n\n> By the way, I forgot to say earlier that this should be done as a\n> separate patch (in order to make the current patch smaller).\n>\n\nOf course, that was obvious :)\n\n> That raises another question. Why are 'struct ref_formatting_state',\n> 'struct align', 'struct atom_value', etc. defined in ref-filter.h at\n> all? Aren't those private implementation details of ref-filter.c, or\n> do you expect other code to be using them?\n>\n\nI guess struct ref_formatting_state and struct align could be moved to\nref-filter.c. About struct atom_value its referenced by ref_array_item()\nso any reader reading about this, would find it easier if atom_value()\nis at the same place.\n\n>>>> for (i = 0; i < final_buf.len; i++)\n>>>> printf(\"%c\", final_buf.buf[i]);\n>>>> putchar('\\n');\n>>>> diff --git a/ref-filter.h b/ref-filter.h\n>>>> index 9e6c2d4..5575fe9 100644\n>>>> --- a/ref-filter.h\n>>>> +++ b/ref-filter.h\n>>>> @@ -16,14 +16,30 @@\n>>>> struct ref_formatting_state {\n>>>> - int quote_style;\n>>>> struct strbuf *output;\n>>>> + struct align *align;\n>>>> + int quote_style;\n>>>\n>>> Perhaps decide where you'd like 'quote_style' to reside from the start\n>>> so that you don't have to add it at one location in its introductory\n>>> patch and then move it in a later patch. Also, why move it here?\n>>\n>> Cause that'd save memory on a 64 bit processor, where the pointers would\n>> be 8 bytes long and int would be 4 bytes long, this would bring in padding if\n>> int is placed before the pointers. Will change before hand.\n>\n> As I understand it, you're not likely to have many\n> ref_fomratting_state's around at any given time, so this sounds like\n> premature memory micro-optimization.\n\nAgreed, its a micro-optimization, but why leave it out when we only need\nto re-structure code? I'll probably change it beforehand.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"267670","messageId":"368C34E3-F245-45AD-847B-0866C61295DA@imag.fr","threadId":"40021","inReplyTo":"CAOLa=ZT+hCgM+mL2vQknwUjaxG9Mr4C2Qir=aa==Fv9CebYwLg@mail.gmail.com","subject":"Re: [PATCH v9 03/11] ref-filter: implement an `align` atom","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2015-08-09T08:00:17Z","receivedAt":"2015-08-09T08:00:17Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"\n\nLe 8 août 2015 09:03:03 GMT+02:00, Karthik Nayak a écrit :\n>On Fri, Aug 7, 2015 at 9:34 AM, Eric Sunshine \n>wrote:\n>> On Wed, Aug 5, 2015 at 2:54 PM, Karthik Nayak \n>wrote:\n>>> Implement an `align` atom which will act as a modifier atom and\n>align\n>>> any string with or without an %(atom) appearing before a %(end) atom\n>>> to the right, left or middle.\n>>>\n>>> It is followed by `:,`, where the `` is\n>>> either left, right or middle and `` is the total\n>length\n>>> of the padding to be performed. If the atom length is more than the\n>>> padding length then no padding is performed. e.g. to pad a\n>succeeding\n>>> atom to the middle with a total padding size of 40 we can do a\n>>> --format=\"%(align:middle,40)..\"\n>>>\n>>> Add documentation and tests for the same.\n>>\n>> I forgot to mention in my earlier review of this patch that you\n>should\n>> explain in the commit message, and probably the documentation, this\n>> this implementation (assuming I'm understanding the code) does not\n>> correctly support nested %(foo)...%(end) constructs, where %(foo)\n>> might be %(if:), %(truncate:), %(cut:), or even a nested %(align:),\n>or\n>> some as yet unimagined modifier. Supporting nesting of these\n>> constructs will require pushing the formatting states onto a stack\n>(or\n>> invoking the parser recursively).\n>>\n>>> Signed-off-by: Karthik Nayak \n>\n>Good point, I have been working on this parallely and it works for now,\n>I'll send that with the %(if) and %(end) feature. But for now, it\n>should be\n>documented and added in the commit message.\n>\n>Using a linked list of sorts where whenever a new modifier atom is\n>encountered\n>a new state is created, and once %(end) is encountered we can pop that\n>state\n>into the previous state.\n\nGood, but keep in mind that this is not needed to port tag/branch, and your GSoC end soon. So keep your priorities in mind... IMHO, the nestable implementation can wait. \n\nCheers,\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"267671","messageId":"CAPig+cT=uUp5T1W1-CBrx4ZMsTSdpoghg57affU3JZ3Uvq20Pw@mail.gmail.com","threadId":"40021","inReplyTo":"CAOLa=ZQW1HBACAb4V_jMky4=54=rxqS=nyYMBX1CUUvA2c0s9A@mail.gmail.com","subject":"Re: [PATCH v9 03/11] ref-filter: implement an `align` atom","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-09T08:04:00Z","receivedAt":"2015-08-09T08:04:00Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Aug 9, 2015 at 2:55 AM, Karthik Nayak wrote:\n> On Sun, Aug 9, 2015 at 9:12 AM, Eric Sunshine wrote:\n>> On Sat, Aug 8, 2015 at 2:35 AM, Karthik Nayak wrote:\n>>> On Fri, Aug 7, 2015 at 8:57 AM, Eric Sunshine wrote:\n>>>> On Wed, Aug 5, 2015 at 2:54 PM, Karthik Nayak wrote:\n>>>>> + else if (align->align_type == ALIGN_MIDDLE) {\n>>>>> + int right = (align->align_value - buf_len)/2;\n>>>>> + strbuf_addf(final, \"%*s%-*s\", align->align_value - right + len,\n>>>>> + value->buf, right, \"\");\n>>>>\n>>>> An aesthetic aside: When (align_value - buf_len) is an odd number,\n>>>> this implementation favors placing more whitespace to the left of the\n>>>> string, and less to the right. In practice, this often tends to look a\n>>>> bit more awkward than the inverse of placing more whitespace to the\n>>>> right, and less to the left (but that again is subjective).\n>>>\n>>> I know that, maybe we could add an additional padding to even out the value\n>>> given?\n>>\n>> I don't understand your question. I was merely suggesting (purely\n>> subjectively), for the \"odd length\" case, putting the extra space\n>> after the centered text rather than before it. For instance:\n>>\n>> int left = (align->align_value - buf_len) / 2;\n>> strbuf_addf(final, \"%*s%-*s\", left, \"\",\n>> align->align_value - left + len, value->buf);\n>>\n>> or any similar variation which would give the same result.\n>\n> I get this could be done, what I was asking was, Consider given a alignment\n> width of 25 would be better to make that 26 so that we have even padding on\n> both sides. But I don't like the adding of manipulating user given data.\n\nI thought you might be asking that, but wasn't certain. I do agree\nwith your conclusion that second-guessing the user is a bad idea, and\nthat you should give the user exactly what was requested.\n\n>> That raises another question. Why are 'struct ref_formatting_state',\n>> 'struct align', 'struct atom_value', etc. defined in ref-filter.h at\n>> all? Aren't those private implementation details of ref-filter.c, or\n>> do you expect other code to be using them?\n>\n> I guess struct ref_formatting_state and struct align could be moved to\n> ref-filter.c. About struct atom_value its referenced by ref_array_item()\n> so any reader reading about this, would find it easier if atom_value()\n> is at the same place.\n\nDo you expect callers ever to be manipulating or otherwise accessing\nthe atom_value of ref_array_item? If callers have no business mucking\nwith atom_value, then one option would be to simply forward declare\natom_value in the header:\n\n struct atom_value;\n\n struct ref_array_item {\n ...\n struct atom_value *value;\n ...\n };\n\nwhich makes atom_value opaque to clients of ref-filter. The actual\ndeclaration of atom_value would then be moved to ref-filter.c, thus\nkept private.\n"},{"id":"267672","messageId":"CAOLa=ZQoHuE_NV1OMQrA7zoqYo6tUvXibURVz+3UknJsNLHV_w@mail.gmail.com","threadId":"40021","inReplyTo":"CAPig+cT=uUp5T1W1-CBrx4ZMsTSdpoghg57affU3JZ3Uvq20Pw@mail.gmail.com","subject":"Re: [PATCH v9 03/11] ref-filter: implement an `align` atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-09T08:09:32Z","receivedAt":"2015-08-09T08:09:32Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Sun, Aug 9, 2015 at 1:34 PM, Eric Sunshine wrote:\n> On Sun, Aug 9, 2015 at 2:55 AM, Karthik Nayak wrote:\n>> On Sun, Aug 9, 2015 at 9:12 AM, Eric Sunshine wrote:\n>>> On Sat, Aug 8, 2015 at 2:35 AM, Karthik Nayak wrote:\n>>>> On Fri, Aug 7, 2015 at 8:57 AM, Eric Sunshine wrote:\n>>>>> On Wed, Aug 5, 2015 at 2:54 PM, Karthik Nayak wrote:\n>>>>>> + else if (align->align_type == ALIGN_MIDDLE) {\n>>>>>> + int right = (align->align_value - buf_len)/2;\n>>>>>> + strbuf_addf(final, \"%*s%-*s\", align->align_value - right + len,\n>>>>>> + value->buf, right, \"\");\n>>>>>\n>>>>> An aesthetic aside: When (align_value - buf_len) is an odd number,\n>>>>> this implementation favors placing more whitespace to the left of the\n>>>>> string, and less to the right. In practice, this often tends to look a\n>>>>> bit more awkward than the inverse of placing more whitespace to the\n>>>>> right, and less to the left (but that again is subjective).\n>>>>\n>>>> I know that, maybe we could add an additional padding to even out the value\n>>>> given?\n>>>\n>>> I don't understand your question. I was merely suggesting (purely\n>>> subjectively), for the \"odd length\" case, putting the extra space\n>>> after the centered text rather than before it. For instance:\n>>>\n>>> int left = (align->align_value - buf_len) / 2;\n>>> strbuf_addf(final, \"%*s%-*s\", left, \"\",\n>>> align->align_value - left + len, value->buf);\n>>>\n>>> or any similar variation which would give the same result.\n>>\n>> I get this could be done, what I was asking was, Consider given a alignment\n>> width of 25 would be better to make that 26 so that we have even padding on\n>> both sides. But I don't like the adding of manipulating user given data.\n>\n> I thought you might be asking that, but wasn't certain. I do agree\n> with your conclusion that second-guessing the user is a bad idea, and\n> that you should give the user exactly what was requested.\n>\n\nIn that case I'll be doing what you suggested, thanks :)\n\n>>> That raises another question. Why are 'struct ref_formatting_state',\n>>> 'struct align', 'struct atom_value', etc. defined in ref-filter.h at\n>>> all? Aren't those private implementation details of ref-filter.c, or\n>>> do you expect other code to be using them?\n>>\n>> I guess struct ref_formatting_state and struct align could be moved to\n>> ref-filter.c. About struct atom_value its referenced by ref_array_item()\n>> so any reader reading about this, would find it easier if atom_value()\n>> is at the same place.\n>\n> Do you expect callers ever to be manipulating or otherwise accessing\n> the atom_value of ref_array_item? If callers have no business mucking\n> with atom_value, then one option would be to simply forward declare\n> atom_value in the header:\n>\n> struct atom_value;\n>\n> struct ref_array_item {\n> ...\n> struct atom_value *value;\n> ...\n> };\n>\n> which makes atom_value opaque to clients of ref-filter. The actual\n> declaration of atom_value would then be moved to ref-filter.c, thus\n> kept private.\n\nAlso the code that this was done in has been excepted into `next`\nso either I send a new series for the same, or write a patch just to\nmove this from ref-filter.h to ref-filter.c. So what would you suggest?\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"267673","messageId":"CAOLa=ZS4GKkzMZvSRiK6FdL1OjJ=MdKkQt7Mxb2sT0-LPX9V-A@mail.gmail.com","threadId":"40021","inReplyTo":"368C34E3-F245-45AD-847B-0866C61295DA@imag.fr","subject":"Re: [PATCH v9 03/11] ref-filter: implement an `align` atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-09T08:10:16Z","receivedAt":"2015-08-09T08:10:16Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Sun, Aug 9, 2015 at 1:30 PM, Matthieu Moy wrote:\n>\n>\n> Le 8 août 2015 09:03:03 GMT+02:00, Karthik Nayak a écrit :\n>>On Fri, Aug 7, 2015 at 9:34 AM, Eric Sunshine \n>>wrote:\n>>> On Wed, Aug 5, 2015 at 2:54 PM, Karthik Nayak \n>>wrote:\n>>>> Implement an `align` atom which will act as a modifier atom and\n>>align\n>>>> any string with or without an %(atom) appearing before a %(end) atom\n>>>> to the right, left or middle.\n>>>>\n>>>> It is followed by `:,`, where the `` is\n>>>> either left, right or middle and `` is the total\n>>length\n>>>> of the padding to be performed. If the atom length is more than the\n>>>> padding length then no padding is performed. e.g. to pad a\n>>succeeding\n>>>> atom to the middle with a total padding size of 40 we can do a\n>>>> --format=\"%(align:middle,40)..\"\n>>>>\n>>>> Add documentation and tests for the same.\n>>>\n>>> I forgot to mention in my earlier review of this patch that you\n>>should\n>>> explain in the commit message, and probably the documentation, this\n>>> this implementation (assuming I'm understanding the code) does not\n>>> correctly support nested %(foo)...%(end) constructs, where %(foo)\n>>> might be %(if:), %(truncate:), %(cut:), or even a nested %(align:),\n>>or\n>>> some as yet unimagined modifier. Supporting nesting of these\n>>> constructs will require pushing the formatting states onto a stack\n>>(or\n>>> invoking the parser recursively).\n>>>\n>>>> Signed-off-by: Karthik Nayak \n>>\n>>Good point, I have been working on this parallely and it works for now,\n>>I'll send that with the %(if) and %(end) feature. But for now, it\n>>should be\n>>documented and added in the commit message.\n>>\n>>Using a linked list of sorts where whenever a new modifier atom is\n>>encountered\n>>a new state is created, and once %(end) is encountered we can pop that\n>>state\n>>into the previous state.\n>\n> Good, but keep in mind that this is not needed to port tag/branch, and your GSoC end soon. So keep your priorities in mind... IMHO, the nestable implementation can wait.\n>\n> Cheers,\n>\n\nAgreed, but it was just something I had already worked on. Probably\nwill push that series after GSoC :)\n\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"267674","messageId":"CAPig+cRoryoJj44D0Q_m3fVAZeYrCc0OiNA7GJ3UtQK4XVOn9g@mail.gmail.com","threadId":"40021","inReplyTo":"CAOLa=ZQoHuE_NV1OMQrA7zoqYo6tUvXibURVz+3UknJsNLHV_w@mail.gmail.com","subject":"Re: [PATCH v9 03/11] ref-filter: implement an `align` atom","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-09T08:19:26Z","receivedAt":"2015-08-09T08:19:26Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Aug 9, 2015 at 4:09 AM, Karthik Nayak wrote:\n> On Sun, Aug 9, 2015 at 1:34 PM, Eric Sunshine wrote:\n>> On Sun, Aug 9, 2015 at 2:55 AM, Karthik Nayak wrote:\n>>> On Sun, Aug 9, 2015 at 9:12 AM, Eric Sunshine wrote:\n>>>> That raises another question. Why are 'struct ref_formatting_state',\n>>>> 'struct align', 'struct atom_value', etc. defined in ref-filter.h at\n>>>> all? Aren't those private implementation details of ref-filter.c, or\n>>>> do you expect other code to be using them?\n>>>\n>>> I guess struct ref_formatting_state and struct align could be moved to\n>>> ref-filter.c. About struct atom_value its referenced by ref_array_item()\n>>> so any reader reading about this, would find it easier if atom_value()\n>>> is at the same place.\n>>\n>> Do you expect callers ever to be manipulating or otherwise accessing\n>> the atom_value of ref_array_item? If callers have no business mucking\n>> with atom_value, then one option would be to simply forward declare\n>> atom_value in the header:\n>>\n>> struct atom_value;\n>>\n>> struct ref_array_item {\n>> ...\n>> struct atom_value *value;\n>> ...\n>> };\n>>\n>> which makes atom_value opaque to clients of ref-filter. The actual\n>> declaration of atom_value would then be moved to ref-filter.c, thus\n>> kept private.\n>\n> Also the code that this was done in has been excepted into `next`\n> so either I send a new series for the same, or write a patch just to\n> move this from ref-filter.h to ref-filter.c. So what would you suggest?\n\nTo my eye, atom_value seems to encapsulate a bunch of state local to\nand only meaningful to ref-filter's internal workings, so it doesn't\nreally belong in the public header. Assuming that you don't foresee\nany callers ever needing to access the properties of atom_value, then\nit might indeed be reasonable to introduce a patch which moves it from\nthe .h file to the .c file (while leaving only a forward declaration\nin the .h file).\n"},{"id":"267676","messageId":"CAOLa=ZTJ0k9zbMK-L0sB49X2+DmnLSV-m+4MiPirTbU8r1iOqg@mail.gmail.com","threadId":"40021","inReplyTo":"CAPig+cRoryoJj44D0Q_m3fVAZeYrCc0OiNA7GJ3UtQK4XVOn9g@mail.gmail.com","subject":"Re: [PATCH v9 03/11] ref-filter: implement an `align` atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-09T12:54:34Z","receivedAt":"2015-08-09T12:54:34Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Sun, Aug 9, 2015 at 1:49 PM, Eric Sunshine wrote:\n> On Sun, Aug 9, 2015 at 4:09 AM, Karthik Nayak wrote:\n>> On Sun, Aug 9, 2015 at 1:34 PM, Eric Sunshine wrote:\n>>> On Sun, Aug 9, 2015 at 2:55 AM, Karthik Nayak wrote:\n>>>> On Sun, Aug 9, 2015 at 9:12 AM, Eric Sunshine wrote:\n>>>>> That raises another question. Why are 'struct ref_formatting_state',\n>>>>> 'struct align', 'struct atom_value', etc. defined in ref-filter.h at\n>>>>> all? Aren't those private implementation details of ref-filter.c, or\n>>>>> do you expect other code to be using them?\n>>>>\n>>>> I guess struct ref_formatting_state and struct align could be moved to\n>>>> ref-filter.c. About struct atom_value its referenced by ref_array_item()\n>>>> so any reader reading about this, would find it easier if atom_value()\n>>>> is at the same place.\n>>>\n>>> Do you expect callers ever to be manipulating or otherwise accessing\n>>> the atom_value of ref_array_item? If callers have no business mucking\n>>> with atom_value, then one option would be to simply forward declare\n>>> atom_value in the header:\n>>>\n>>> struct atom_value;\n>>>\n>>> struct ref_array_item {\n>>> ...\n>>> struct atom_value *value;\n>>> ...\n>>> };\n>>>\n>>> which makes atom_value opaque to clients of ref-filter. The actual\n>>> declaration of atom_value would then be moved to ref-filter.c, thus\n>>> kept private.\n>>\n>> Also the code that this was done in has been excepted into `next`\n>> so either I send a new series for the same, or write a patch just to\n>> move this from ref-filter.h to ref-filter.c. So what would you suggest?\n>\n> To my eye, atom_value seems to encapsulate a bunch of state local to\n> and only meaningful to ref-filter's internal workings, so it doesn't\n> really belong in the public header. Assuming that you don't foresee\n> any callers ever needing to access the properties of atom_value, then\n> it might indeed be reasonable to introduce a patch which moves it from\n> the .h file to the .c file (while leaving only a forward declaration\n> in the .h file).\n\nThanks! will add it to the series.\n\n-- \nRegards,\nKarthik Nayak\n"}]}