{"thread":{"id":"40474","subject":"[PATCH 0/9] port branch.c to use ref-filter printing APIs","startedAt":"2015-10-02T17:38:57Z","lastAt":"2015-10-07T18:02:27Z","messageCount":43,"participants":["Karthik Nayak","Junio C Hamano","Matthieu Moy"],"isPatch":true,"patchVersion":1,"patchTotal":9},"messages":[{"id":"271013","messageId":"1443807546-5985-1-git-send-email-Karthik.188@gmail.com","threadId":"40474","inReplyTo":null,"subject":"[PATCH 0/9] port branch.c to use ref-filter printing APIs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-10-02T17:38:57Z","receivedAt":"2015-10-02T17:38:57Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"This series ports over branch.c to use ref-filter APIs\nThis is the last part of unification of 'git branch -l', 'git tag -l'\nand 'git for-each-ref'.\n\nThere are a few topics worth discussing here :\n\n1. [7/9] This ensures that when we use the \"%(upstream:track)\" atom we\nprint \"[gone]\" for upstreams which are invalid. We either implement\nthis and try to mimic what branch.c does when we use the verbose\noption or we can drop this in branch.c.\n\n2. [8/9] The usage of get_format() function in branch.c which provides\nus the needed format for printing. This seems overly complex to the\nuninitiated as it uses a complex atom combinations to mimic the\nexisting branch printing format.\n\nKarthik Nayak (9):\n ref-filter: implement %(if), %(then), and %(else) atoms\n ref-filter: implement %(if:equals=) and\n %(if:notequals=)\n ref-filter: add support for %(path) atom\n ref-filter: modify \"%(objectname:short)\" to take length\n ref-filter: adopt get_head_description() from branch.c\n ref-filter: introduce format_ref_array_item()\n ref-filter: make %(upstream:track) prints \"[gone]\" for invalid\n upstreams\n branch: use ref-filter printing APIs\n branch: implement '--format' option\n\n Documentation/git-branch.txt | 7 +-\n Documentation/git-for-each-ref.txt | 32 ++++-\n builtin/branch.c | 262 ++++++++++---------------------------\n ref-filter.c | 250 +++++++++++++++++++++++++++++++----\n ref-filter.h | 3 +\n t/t3203-branch-output.sh | 11 ++\n t/t6040-tracking-info.sh | 12 +-\n t/t6300-for-each-ref.sh | 24 +++-\n t/t6302-for-each-ref-filter.sh | 105 +++++++++++++++\n 9 files changed, 480 insertions(+), 226 deletions(-)\n\n-- \n2.6.0\n"},{"id":"271021","messageId":"1443807546-5985-2-git-send-email-Karthik.188@gmail.com","threadId":"40474","inReplyTo":"1443807546-5985-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH 1/9] ref-filter: implement %(if), %(then), and %(else) atoms","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-10-02T17:38:58Z","receivedAt":"2015-10-02T17:38:58Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Implement %(if), %(then) and %(else) atoms. Used as\n%(if)..%(then)..%(end) or %(if)..%(then)..%(else)..%(end). If there is\nan atom with value or string literal after the %(if) then everything\nafter the %(then) is printed, else if the %(else) atom is used, then\neverything after %(else) is printed. If the string contains only\nwhitespaces, then it is not considered. Nesting of this construct is\npossible.\n\nThis is in preperation for porting over `git branch -l` to use\nref-filter APIs for printing.\n\nAdd Documentation and tests regarding the same.\n\nMentored-by: Christian Couder \nMentored-by: Matthieu Moy \nSigned-off-by: Karthik Nayak \n---\n Documentation/git-for-each-ref.txt | 23 +++++++-\n ref-filter.c | 116 ++++++++++++++++++++++++++++++++++---\n t/t6302-for-each-ref-filter.sh | 48 +++++++++++++++\n 3 files changed, 177 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex 16b4ac5..768a512 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -134,9 +134,14 @@ align::\n \t`` is either left, right or middle, default being\n \tleft and `` is the total length of the content with\n \talignment. If the contents length is more than the width then\n-\tno alignment is performed. If used with '--quote' everything\n-\tin between %(align:...) and %(end) is quoted, but if nested\n-\tthen only the topmost level performs quoting.\n+\tno alignment is performed.\n+\n+if::\n+\tUsed as %(if)..%(then)..(%end) or %(if)..%(then)..%(else)..%(end).\n+\tIf there is an atom with value or string literal after the\n+\t%(if) then everything after the %(then) is printed, else if\n+\tthe %(else) atom is used, then everything after %(else) is\n+\tprinted.\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@@ -169,6 +174,18 @@ the date by adding one of `:default`, `:relative`, `:short`, `:local`,\n `:iso8601`, `:rfc2822` or `:raw` to the end of the fieldname; e.g.\n `%(taggerdate:relative)`.\n \n+Some atoms like %(align) and %(if) always require a matching %(end).\n+We call them \"opening atoms\" and sometimes denote them as %($open).\n+\n+When a scripting language specific quoting is in effect, except for\n+opening atoms, replacement from every %(atom) is quoted when and only\n+when it appears at the top-level (that is, when it appears outside\n+%($open)...%(end)).\n+\n+When a scripting language specific quoting is in effect, everything\n+between a top-level opening atom and its matching %(end) is evaluated\n+according to the semantics of the opening atom and its result is\n+quoted.\n \n EXAMPLES\n --------\ndiff --git a/ref-filter.c b/ref-filter.c\nindex dbd8fce..93b07f5 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -55,6 +55,9 @@ static struct {\n \t{ \"color\" },\n \t{ \"align\" },\n \t{ \"end\" },\n+\t{ \"if\" },\n+\t{ \"then\" },\n+\t{ \"else\" },\n };\n \n #define REF_FORMATTING_STATE_INIT { 0, NULL }\n@@ -69,10 +72,17 @@ struct contents {\n \tstruct object_id oid;\n };\n \n+struct if_then_else {\n+\tunsigned int if_atom : 1,\n+\t\tthen_atom : 1,\n+\t\telse_atom : 1,\n+\t\tcondition_satisfied : 1;\n+};\n+\n struct ref_formatting_stack {\n \tstruct ref_formatting_stack *prev;\n \tstruct strbuf output;\n-\tvoid (*at_end)(struct ref_formatting_stack *stack);\n+\tvoid (*at_end)(struct ref_formatting_stack **stack);\n \tvoid *at_end_data;\n };\n \n@@ -216,13 +226,14 @@ static void pop_stack_element(struct ref_formatting_stack **stack)\n \t*stack = prev;\n }\n \n-static void end_align_handler(struct ref_formatting_stack *stack)\n+static void end_align_handler(struct ref_formatting_stack **stack)\n {\n-\tstruct align *align = (struct align *)stack->at_end_data;\n+\tstruct ref_formatting_stack *cur = *stack;\n+\tstruct align *align = (struct align *)cur->at_end_data;\n \tstruct strbuf s = STRBUF_INIT;\n \n-\tstrbuf_utf8_align(&s, align->position, align->width, stack->output.buf);\n-\tstrbuf_swap(&stack->output, &s);\n+\tstrbuf_utf8_align(&s, align->position, align->width, cur->output.buf);\n+\tstrbuf_swap(&cur->output, &s);\n \tstrbuf_release(&s);\n }\n \n@@ -236,6 +247,85 @@ static void align_atom_handler(struct atom_value *atomv, struct ref_formatting_s\n \tnew->at_end_data = &atomv->u.align;\n }\n \n+static void if_then_else_handler(struct ref_formatting_stack **stack)\n+{\n+\tstruct ref_formatting_stack *cur = *stack;\n+\tstruct ref_formatting_stack *prev = cur->prev;\n+\tstruct if_then_else *if_then_else = (struct if_then_else *)cur->at_end_data;\n+\n+\t/*\n+\t * If the 'if' condition is satisfied, then if there exists an\n+\t * 'else' atom drop the 'else' atom's state. Else we swap the\n+\t * buffer of the 'else' atom with the previous state and drop\n+\t * that state. If the condition is satisfied and no 'else' atom\n+\t * exists, then just clear the buffer.\n+\t */\n+\tif (if_then_else->condition_satisfied && if_then_else->else_atom) {\n+\t\tstrbuf_reset(&cur->output);\n+\t\tpop_stack_element(&cur);\n+\t} else if (if_then_else->else_atom) {\n+\t\tstrbuf_swap(&cur->output, &prev->output);\n+\t\tstrbuf_reset(&cur->output);\n+\t\tpop_stack_element(&cur);\n+\t} else if (!if_then_else->condition_satisfied)\n+\t\tstrbuf_reset(&cur->output);\n+\t*stack = cur;\n+\tfree(if_then_else);\n+}\n+\n+static void if_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)\n+{\n+\tstruct ref_formatting_stack *new;\n+\tstruct if_then_else *if_then_else = xcalloc(sizeof(struct if_then_else), 1);\n+\n+\tif_then_else->if_atom = 1;\n+\tpush_stack_element(&state->stack);\n+\tnew = state->stack;\n+\tnew->at_end = if_then_else_handler;\n+\tnew->at_end_data = if_then_else;\n+}\n+\n+static int is_empty(const char * s){\n+\twhile (*s != '\\0') {\n+\t\tif (!isspace(*s))\n+\t\t\treturn 0;\n+\t\ts++;\n+\t}\n+\treturn 1;\n+}\n+\n+static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)\n+{\n+\tstruct ref_formatting_stack *cur = state->stack;\n+\tstruct if_then_else *if_then_else = (struct if_then_else *)cur->at_end_data;\n+\n+\tif (!if_then_else)\n+\t\tdie(_(\"format: %%(then) atom used without an %%(if) atom\"));\n+\tif_then_else->then_atom = 1;\n+\t/*\n+\t * If there exists non-empty string between the 'if' and\n+\t * 'then' atom then the 'if' condition is satisfied.\n+\t */\n+\tif (cur->output.len && !is_empty(cur->output.buf))\n+\t\tif_then_else->condition_satisfied = 1;\n+\tstrbuf_reset(&cur->output);\n+}\n+\n+static void else_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)\n+{\n+\tstruct ref_formatting_stack *prev = state->stack;\n+\tstruct if_then_else *if_then_else = (struct if_then_else *)state->stack->at_end_data;\n+\n+\tif (!if_then_else)\n+\t\tdie(_(\"format: %%(else) atom used without an %%(if) atom\"));\n+\tif (!if_then_else->then_atom)\n+\t\tdie(_(\"format: %%(else) atom used without a %%(then) atom\"));\n+\tif_then_else->else_atom = 1;\n+\tpush_stack_element(&state->stack);\n+\tstate->stack->at_end_data = prev->at_end_data;\n+\tstate->stack->at_end = prev->at_end;\n+}\n+\n static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)\n {\n \tstruct ref_formatting_stack *current = state->stack;\n@@ -243,14 +333,17 @@ static void end_atom_handler(struct atom_value *atomv, struct ref_formatting_sta\n \n \tif (!current->at_end)\n \t\tdie(_(\"format: %%(end) atom used without corresponding atom\"));\n-\tcurrent->at_end(current);\n+\tcurrent->at_end(&state->stack);\n+\n+\t/* Stack may have been popped, hence reset the current pointer */\n+\tcurrent = state->stack;\n \n \t/*\n \t * Perform quote formatting when the stack element is that of\n \t * a supporting atom. If nested then perform quote formatting\n \t * only on the topmost supporting atom.\n \t */\n-\tif (!state->stack->prev->prev) {\n+\tif (!current->prev->prev) {\n \t\tquote_formatting(&s, current->output.buf, state->quote_style);\n \t\tstrbuf_swap(¤t->output, &s);\n \t}\n@@ -920,6 +1013,15 @@ static void populate_value(struct ref_array_item *ref)\n \t\t} else if (!strcmp(name, \"end\")) {\n \t\t\tv->handler = end_atom_handler;\n \t\t\tcontinue;\n+\t\t} else if (!strcmp(name, \"if\")) {\n+\t\t\tv->handler = if_atom_handler;\n+\t\t\tcontinue;\n+\t\t} else if (!strcmp(name, \"then\")) {\n+\t\t\tv->handler = then_atom_handler;\n+\t\t\tcontinue;\n+\t\t} else if (!strcmp(name, \"else\")) {\n+\t\t\tv->handler = else_atom_handler;\n+\t\t\tcontinue;\n \t\t} else\n \t\t\tcontinue;\n \ndiff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\nindex fe4796c..7ce1cc4 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -255,4 +255,52 @@ test_expect_success 'reverse version sort' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'improper usage of %(if), %(then), %(else) and %(end) atoms' '\n+\ttest_must_fail git for-each-ref --format=\"%(if)\" &&\n+\ttest_must_fail git for-each-ref --format=\"%(then)\" &&\n+\ttest_must_fail git for-each-ref --format=\"%(else)\" &&\n+\ttest_must_fail git for-each-ref --format=\"%(if) %(else)\" &&\n+\ttest_must_fail git for-each-ref --format=\"%(then) %(else)\" &&\n+\ttest_must_fail git for-each-ref --format=\"%(if) %(else)\" &&\n+\ttest_must_fail git for-each-ref --format=\"%(if) %(then) %(else)\"\n+'\n+\n+test_expect_success 'check %(if)...%(then)...%(end) atoms' '\n+\tgit for-each-ref --format=\"%(if)%(authorname)%(then)%(authorname): %(refname)%(end)\" >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tA U Thor: refs/heads/master\n+\tA U Thor: refs/heads/side\n+\tA U Thor: refs/odd/spot\n+\t\n+\tA U Thor: refs/tags/foo1.10\n+\tA U Thor: refs/tags/foo1.3\n+\tA U Thor: refs/tags/foo1.6\n+\tA U Thor: refs/tags/four\n+\tA U Thor: refs/tags/one\n+\t\n+\tA U Thor: refs/tags/three\n+\tA U Thor: refs/tags/two\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check %(if)...%(then)...%(else)...%(end) atoms' '\n+\tgit for-each-ref --format=\"%(if)%(authorname)%(then)%(authorname)%(else)No author%(end): %(refname)\" >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tA U Thor: refs/heads/master\n+\tA U Thor: refs/heads/side\n+\tA U Thor: refs/odd/spot\n+\tNo author: refs/tags/double-tag\n+\tA U Thor: refs/tags/foo1.10\n+\tA U Thor: refs/tags/foo1.3\n+\tA U Thor: refs/tags/foo1.6\n+\tA U Thor: refs/tags/four\n+\tA U Thor: refs/tags/one\n+\tNo author: refs/tags/signed-tag\n+\tA U Thor: refs/tags/three\n+\tA U Thor: refs/tags/two\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.6.0\n"},{"id":"271022","messageId":"1443807546-5985-3-git-send-email-Karthik.188@gmail.com","threadId":"40474","inReplyTo":"1443807546-5985-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH 2/9] ref-filter: implement %(if:equals=) and %(if:notequals=)","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-10-02T17:38:59Z","receivedAt":"2015-10-02T17:38:59Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Implement %(if:equals=) wherein the if condition is only\nsatisfied if the value obtained between the %(if:...) and %(then) atom\nis the same as the given ''.\n\nSimilarly, implement (if:notequals=) wherein the if condition\nis only satisfied if the value obtained between the %(if:...) and\n%(then) atom is differnt from the given ''.\n\nAdd tests and Documentation 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 | 4 +++-\n ref-filter.c | 28 ++++++++++++++++++++++++----\n t/t6302-for-each-ref-filter.sh | 18 ++++++++++++++++++\n 3 files changed, 45 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex 768a512..5c12c2f 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -141,7 +141,9 @@ if::\n \tIf there is an atom with value or string literal after the\n \t%(if) then everything after the %(then) is printed, else if\n \tthe %(else) atom is used, then everything after %(else) is\n-\tprinted.\n+\tprinted. Append \":equals=\" or \":notequals=\" to\n+\tcompare the value between the %(if:...) and %(then) atoms with the\n+\tgiven string.\n \n In addition to the above, for commit and tag objects, the header\n field names (`tree`, `parent`, `object`, `type`, and `tag`) can\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 93b07f5..da7723b 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -73,6 +73,8 @@ struct contents {\n };\n \n struct if_then_else {\n+\tconst char *if_equals,\n+\t\t*not_equals;\n \tunsigned int if_atom : 1,\n \t\tthen_atom : 1,\n \t\telse_atom : 1,\n@@ -277,8 +279,16 @@ static void if_atom_handler(struct atom_value *atomv, struct ref_formatting_stat\n {\n \tstruct ref_formatting_stack *new;\n \tstruct if_then_else *if_then_else = xcalloc(sizeof(struct if_then_else), 1);\n+\tconst char *valp;\n \n \tif_then_else->if_atom = 1;\n+\tif (skip_prefix(atomv->s, \"equals=\", &valp))\n+\t\tif_then_else->if_equals = valp;\n+\telse if (skip_prefix(atomv->s, \"notequals=\", &valp))\n+\t\tif_then_else->not_equals = valp;\n+\telse if (atomv->s[0])\n+\t\tdie(_(\"format: unknown format if:%s\"), atomv->s);\n+\n \tpush_stack_element(&state->stack);\n \tnew = state->stack;\n \tnew->at_end = if_then_else_handler;\n@@ -302,11 +312,19 @@ static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_st\n \tif (!if_then_else)\n \t\tdie(_(\"format: %%(then) atom used without an %%(if) atom\"));\n \tif_then_else->then_atom = 1;\n+\n \t/*\n-\t * If there exists non-empty string between the 'if' and\n-\t * 'then' atom then the 'if' condition is satisfied.\n+\t * If the 'equals' or 'notequals' attribute is used then\n+\t * perform the required comparison. If not, only non-empty\n+\t * strings satisfy the 'if' condition.\n \t */\n-\tif (cur->output.len && !is_empty(cur->output.buf))\n+\tif (if_then_else->if_equals) {\n+\t\tif (!strcmp(if_then_else->if_equals, cur->output.buf))\n+\t\t\tif_then_else->condition_satisfied = 1;\n+\t} else \tif (if_then_else->not_equals) {\n+\t\tif (strcmp(if_then_else->not_equals, cur->output.buf))\n+\t\t\tif_then_else->condition_satisfied = 1;\n+\t} else if (cur->output.len && !is_empty(cur->output.buf))\n \t\tif_then_else->condition_satisfied = 1;\n \tstrbuf_reset(&cur->output);\n }\n@@ -1013,8 +1031,10 @@ static void populate_value(struct ref_array_item *ref)\n \t\t} else if (!strcmp(name, \"end\")) {\n \t\t\tv->handler = end_atom_handler;\n \t\t\tcontinue;\n-\t\t} else if (!strcmp(name, \"if\")) {\n+\t\t} else if (match_atom_name(name, \"if\", &valp)) {\n \t\t\tv->handler = if_atom_handler;\n+\t\t\tif (valp)\n+\t\t\t\tv->s = xstrdup(valp);\n \t\t\tcontinue;\n \t\t} else if (!strcmp(name, \"then\")) {\n \t\t\tv->handler = then_atom_handler;\ndiff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\nindex 7ce1cc4..d7f7a18 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -303,4 +303,22 @@ test_expect_success 'check %(if)...%(then)...%(else)...%(end) atoms' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'check %(if:equals=)' '\n+\tgit for-each-ref --format=\"%(if:equals=master)%(refname:short)%(then)Found master%(else)Not master%(end)\" refs/heads/ >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tFound master\n+\tNot master\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check %(if:notequals=)' '\n+\tgit for-each-ref --format=\"%(if:notequals=master)%(refname:short)%(then)Not master%(else)Found master%(end)\" refs/heads/ >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tFound master\n+\tNot master\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.6.0\n"},{"id":"271016","messageId":"1443807546-5985-4-git-send-email-Karthik.188@gmail.com","threadId":"40474","inReplyTo":"1443807546-5985-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH 3/9] ref-filter: add support for %(path) atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-10-02T17:39:00Z","receivedAt":"2015-10-02T17:39:00Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"This adds %(path) and %(path:short) atoms. The %(path) atom will print\nthe path of the given ref, while %(path:short) will only print the\nsubdirectory of the given ref.\n\nAdd tests and documentation for the same.\n---\n Documentation/git-for-each-ref.txt | 5 +++++\n ref-filter.c | 17 +++++++++++++++++\n t/t6302-for-each-ref-filter.sh | 39 ++++++++++++++++++++++++++++++++++++++\n 3 files changed, 61 insertions(+)\n\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex 5c12c2f..6a476ba 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -145,6 +145,11 @@ if::\n \tcompare the value between the %(if:...) and %(then) atoms with the\n \tgiven string.\n \n+path::\n+\tThe path of the given ref. For a shortened version listing\n+\tonly the name of the directory the ref is under append\n+\t`:short`.\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 da7723b..b0e86ae 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -58,6 +58,7 @@ static struct {\n \t{ \"if\" },\n \t{ \"then\" },\n \t{ \"else\" },\n+\t{ \"path\" },\n };\n \n #define REF_FORMATTING_STATE_INIT { 0, NULL }\n@@ -1042,6 +1043,22 @@ static void populate_value(struct ref_array_item *ref)\n \t\t} else if (!strcmp(name, \"else\")) {\n \t\t\tv->handler = else_atom_handler;\n \t\t\tcontinue;\n+\t\t} else if (match_atom_name(name, \"path\", &valp)) {\n+\t\t\tconst char *sp, *ep;\n+\n+\t\t\tif (ref->kind & FILTER_REFS_DETACHED_HEAD)\n+\t\t\t\tcontinue;\n+\n+\t\t\tsp = strchr(ref->refname, '/');\n+\t\t\tep = strchr(++sp, '/');\n+\n+\t\t\tif (!valp)\n+\t\t\t\tsp = ref->refname;\n+\t\t\telse if (strcmp(valp, \"short\"))\n+\t\t\t\tdie(_(\"format: invalid value given path:%s\"), valp);\n+\n+\t\t\tv->s = xstrndup(sp, ep - sp);\n+\t\t\tcontinue;\n \t\t} else\n \t\t\tcontinue;\n \ndiff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\nindex d7f7a18..5557657 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -312,6 +312,7 @@ test_expect_success 'check %(if:equals=)' '\n \ttest_cmp expect actual\n '\n \n+\n test_expect_success 'check %(if:notequals=)' '\n \tgit for-each-ref --format=\"%(if:notequals=master)%(refname:short)%(then)Not master%(else)Found master%(end)\" refs/heads/ >actual &&\n \tcat >expect <<-\\EOF &&\n@@ -321,4 +322,42 @@ test_expect_success 'check %(if:notequals=)' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'check %(path)' '\n+\tgit for-each-ref --format=\"%(path)\" >actual &&\n+\tcat >expect <<-\\EOF &&\n+\trefs/heads\n+\trefs/heads\n+\trefs/odd\n+\trefs/tags\n+\trefs/tags\n+\trefs/tags\n+\trefs/tags\n+\trefs/tags\n+\trefs/tags\n+\trefs/tags\n+\trefs/tags\n+\trefs/tags\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check %(path:short)' '\n+\tgit for-each-ref --format=\"%(path:short)\" >actual &&\n+\tcat >expect <<-\\EOF &&\n+\theads\n+\theads\n+\todd\n+\ttags\n+\ttags\n+\ttags\n+\ttags\n+\ttags\n+\ttags\n+\ttags\n+\ttags\n+\ttags\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.6.0\n"},{"id":"271015","messageId":"1443807546-5985-5-git-send-email-Karthik.188@gmail.com","threadId":"40474","inReplyTo":"1443807546-5985-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH 4/9] ref-filter: modify \"%(objectname:short)\" to take length","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-10-02T17:39:01Z","receivedAt":"2015-10-02T17:39:01Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Add support for %(objectname:short,) which would print the\nabbreviated unique objectname of given length. When no length is\nspecified 7 is used. The minimum length is 4.\n\nAdd tests and documentation 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 | 2 ++\n ref-filter.c | 39 ++++++++++++++++++++++++++++++--------\n t/t6300-for-each-ref.sh | 22 +++++++++++++++++++++\n 3 files changed, 55 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex 6a476ba..5097915 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -103,6 +103,8 @@ objectsize::\n objectname::\n \tThe object name (aka SHA-1).\n \tFor a non-ambiguous abbreviation of the object name append `:short`.\n+\tThe length can be explicitly specified by appending either\n+\t`:short,` or `:,short`. Minimum length is 4.\n \n upstream::\n \tThe name of a local ref which can be considered ``upstream''\ndiff --git a/ref-filter.c b/ref-filter.c\nindex b0e86ae..223daeb 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -454,14 +454,37 @@ static void *get_obj(const unsigned char *sha1, struct object **obj, unsigned lo\n static int grab_objectname(const char *name, const unsigned char *sha1,\n \t\t\t struct atom_value *v)\n {\n-\tif (!strcmp(name, \"objectname\")) {\n-\t\tchar *s = xmalloc(41);\n-\t\tstrcpy(s, sha1_to_hex(sha1));\n-\t\tv->s = s;\n-\t\treturn 1;\n-\t}\n-\tif (!strcmp(name, \"objectname:short\")) {\n-\t\tv->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));\n+\tconst char *p;\n+\n+\tif (match_atom_name(name, \"objectname\", &p)) {\n+\t\tstruct strbuf **s, **to_free;\n+\t\tint length = DEFAULT_ABBREV;\n+\n+\t\t/* No arguments given, copy the entire objectname */\n+\t\tif (!p) {\n+\t\t\tchar *s = xmalloc(41);\n+\t\t\tstrcpy(s, sha1_to_hex(sha1));\n+\t\t\tv->s = s;\n+\t\t} else {\n+\t\t\ts = to_free = strbuf_split_str(p, ',', 0);\n+\t\t\twhile (*s) {\n+\t\t\t\t/* Strip trailing comma */\n+\t\t\t\tif (s[1])\n+\t\t\t\t\tstrbuf_setlen(s[0], s[0]->len - 1);\n+\t\t\t\tif (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&length))\n+\t\t\t\t\t;\n+\t\t\t\t/* The `short` argument uses the default length */\n+\t\t\t\telse if (!strcmp(\"short\", s[0]->buf))\n+\t\t\t\t\t;\n+\t\t\t\telse\n+\t\t\t\t\tdie(_(\"format: unknown option entered with objectname:%s\"), s[0]->buf);\n+\t\t\t\ts++;\n+\t\t\t}\n+\t\t\tif (length < MINIMUM_ABBREV)\n+\t\t\t\tlength = MINIMUM_ABBREV;\n+\t\t\tv->s = xstrdup(find_unique_abbrev(sha1, length));\n+\t\t\tfree(to_free);\n+\t\t}\n \t\treturn 1;\n \t}\n \treturn 0;\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 7c9bec7..7f675d2 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -385,6 +385,28 @@ test_expect_success 'Check short objectname format' '\n \ttest_cmp expected actual\n '\n \n+cat >expected <actual &&\n+\ttest_cmp expected actual\n+'\n+\n+cat >expected <actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'Check objectname:short format for invalid input' '\n+\ttest_must_fail git for-each-ref --format=\"%(objectname:short,-1)\" refs/heads\n+'\n+\n test_expect_success 'Check for invalid refname format' '\n \ttest_must_fail git for-each-ref --format=\"%(refname:INVALID)\"\n '\n-- \n2.6.0\n"},{"id":"271014","messageId":"1443807546-5985-6-git-send-email-Karthik.188@gmail.com","threadId":"40474","inReplyTo":"1443807546-5985-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH 5/9] ref-filter: adopt get_head_description() from branch.c","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-10-02T17:39:02Z","receivedAt":"2015-10-02T17:39:02Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Copy the implementation of get_head_description() from branch.c.\nThis gives a description of the HEAD ref if called. This is used as\nthe refname for the HEAD ref whenever the FILTER_REFS_DETACHED_HEAD\noption is used.\n\nMentored-by: Christian Couder \nMentored-by: Matthieu Moy \nSigned-off-by: Karthik Nayak \n---\n builtin/branch.c | 4 ++++\n ref-filter.c | 38 ++++++++++++++++++++++++++++++++++++--\n 2 files changed, 40 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex b7a60f4..67ef9f1 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -355,6 +355,10 @@ static void add_verbose_info(struct strbuf *out, struct ref_array_item *item,\n \tstrbuf_release(&subject);\n }\n \n+/*\n+ * This is duplicated in ref-filter.c, will be removed when we adopt\n+ * ref-filter's printing APIs.\n+ */\n static char *get_head_description(void)\n {\n \tstruct strbuf desc = STRBUF_INIT;\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 223daeb..15df421 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -13,6 +13,7 @@\n #include \"utf8.h\"\n #include \"git-compat-util.h\"\n #include \"version.h\"\n+#include \"wt-status.h\"\n \n typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;\n \n@@ -915,6 +916,37 @@ static inline char *copy_advance(char *dst, const char *src)\n \treturn dst;\n }\n \n+static char *get_head_description(void)\n+{\n+\tstruct strbuf desc = STRBUF_INIT;\n+\tstruct wt_status_state state;\n+\tmemset(&state, 0, sizeof(state));\n+\twt_status_get_state(&state, 1);\n+\tif (state.rebase_in_progress ||\n+\t state.rebase_interactive_in_progress)\n+\t\tstrbuf_addf(&desc, _(\"(no branch, rebasing %s)\"),\n+\t\t\t state.branch);\n+\telse if (state.bisect_in_progress)\n+\t\tstrbuf_addf(&desc, _(\"(no branch, bisect started on %s)\"),\n+\t\t\t state.branch);\n+\telse if (state.detached_from) {\n+\t\t/* TRANSLATORS: make sure these match _(\"HEAD detached at \")\n+\t\t and _(\"HEAD detached from \") in wt-status.c */\n+\t\tif (state.detached_at)\n+\t\t\tstrbuf_addf(&desc, _(\"(HEAD detached at %s)\"),\n+\t\t\t\tstate.detached_from);\n+\t\telse\n+\t\t\tstrbuf_addf(&desc, _(\"(HEAD detached from %s)\"),\n+\t\t\t\tstate.detached_from);\n+\t}\n+\telse\n+\t\tstrbuf_addstr(&desc, _(\"(no branch)\"));\n+\tfree(state.branch);\n+\tfree(state.onto);\n+\tfree(state.detached_from);\n+\treturn strbuf_detach(&desc, NULL);\n+}\n+\n /*\n * Parse the object referred by ref, and grab needed value.\n */\n@@ -953,9 +985,11 @@ static void populate_value(struct ref_array_item *ref)\n \t\t\tname++;\n \t\t}\n \n-\t\tif (starts_with(name, \"refname\"))\n+\t\tif (starts_with(name, \"refname\")) {\n \t\t\trefname = ref->refname;\n-\t\telse if (starts_with(name, \"symref\"))\n+\t\t\tif (ref->kind & FILTER_REFS_DETACHED_HEAD)\n+\t\t\t\trefname = get_head_description();\n+\t\t} else if (starts_with(name, \"symref\"))\n \t\t\trefname = ref->symref ? ref->symref : \"\";\n \t\telse if (starts_with(name, \"upstream\")) {\n \t\t\tconst char *branch_name;\n-- \n2.6.0\n"},{"id":"271018","messageId":"1443807546-5985-7-git-send-email-Karthik.188@gmail.com","threadId":"40474","inReplyTo":"1443807546-5985-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH 6/9] ref-filter: introduce format_ref_array_item()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-10-02T17:39:03Z","receivedAt":"2015-10-02T17:39:03Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Introduce format_ref_array_item() which will output the details of a\ngiven ref_array_item as per the given format and quote_style to the\ngiven strbuf.\n\nThis is derived from show_ref_array_item() which is now reduced to a\nwrapper around format_ref_array_item(). show_ref_array_item() obtains\nthe strbuf and prints it the standard output.\n\nThis is needed when we port over the printing options of ref-filter to\nbranch.c.\n\nMentored-by: Christian Couder \nMentored-by: Matthieu Moy \nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 16 ++++++++++++----\n ref-filter.h | 3 +++\n 2 files changed, 15 insertions(+), 4 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 15df421..099acd6 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1748,10 +1748,10 @@ static void append_literal(const char *cp, const char *ep, struct ref_formatting\n \t}\n }\n \n-void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)\n+void format_ref_array_item(struct ref_array_item *info, const char *format,\n+\t\t\t int quote_style, struct strbuf *final_buf)\n {\n \tconst char *cp, *sp, *ep;\n-\tstruct strbuf *final_buf;\n \tstruct ref_formatting_state state = REF_FORMATTING_STATE_INIT;\n \n \tstate.quote_style = quote_style;\n@@ -1781,9 +1781,17 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu\n \t}\n \tif (state.stack->prev)\n \t\tdie(_(\"format: %%(end) atom missing\"));\n-\tfinal_buf = &state.stack->output;\n-\tfwrite(final_buf->buf, 1, final_buf->len, stdout);\n+\tstrbuf_addbuf(final_buf, &state.stack->output);\n \tpop_stack_element(&state.stack);\n+}\n+\n+void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style)\n+{\n+\tstruct strbuf final_buf = STRBUF_INIT;\n+\n+\tformat_ref_array_item(info, format, quote_style, &final_buf);\n+\tfwrite(final_buf.buf, 1, final_buf.len, stdout);\n+\tstrbuf_release(&final_buf);\n \tputchar('\\n');\n }\n \ndiff --git a/ref-filter.h b/ref-filter.h\nindex 14d435e..e0b26e8 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -98,6 +98,9 @@ int parse_ref_filter_atom(const char *atom, const char *ep);\n int verify_ref_format(const char *format);\n /* Sort the given ref_array as per the ref_sorting provided */\n void ref_array_sort(struct ref_sorting *sort, struct ref_array *array);\n+/* Based on the given format and quote_style, fill the strbuf */\n+void format_ref_array_item(struct ref_array_item *info, const char *format,\n+\t\t\t int quote_style, struct strbuf *final_buf);\n /* Print the ref using the given format and quote_style */\n void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style);\n /* Callback function for parsing the sort option */\n-- \n2.6.0\n"},{"id":"271017","messageId":"1443807546-5985-8-git-send-email-Karthik.188@gmail.com","threadId":"40474","inReplyTo":"1443807546-5985-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH 7/9] ref-filter: make %(upstream:track) prints \"[gone]\" for invalid upstreams","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-10-02T17:39:04Z","receivedAt":"2015-10-02T17:39:04Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Borrowing from branch.c's implementation print \"[gone]\" whenever an\nunknown upstream ref is encountered instead of just ignoring it.\n\nThis makes sure that when branch.c is ported over to using ref-filter\nAPIs for printing, this feature is not lost.\n\nMake changes to t/t6300-for-each-ref.sh to reflect this change.\n\nMentored-by: Christian Couder \nMentored-by: Matthieu Moy \nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 4 +++-\n t/t6300-for-each-ref.sh | 2 +-\n 2 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 099acd6..48b06e3 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1133,8 +1133,10 @@ static void populate_value(struct ref_array_item *ref)\n \t\t\t\tchar buf[40];\n \n \t\t\t\tif (stat_tracking_info(branch, &num_ours,\n-\t\t\t\t\t\t &num_theirs, NULL))\n+\t\t\t\t\t\t &num_theirs, NULL)) {\n+\t\t\t\t\tv->s = xstrdup(\"[gone]\");\n \t\t\t\t\tcontinue;\n+\t\t\t\t}\n \n \t\t\t\tif (!num_ours && !num_theirs)\n \t\t\t\t\tv->s = \"\";\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 7f675d2..2a01645 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -359,7 +359,7 @@ test_expect_success 'Check that :track[short] cannot be used with other atoms' '\n \n test_expect_success 'Check that :track[short] works when upstream is invalid' '\n \tcat >expected <<-\\EOF &&\n-\n+\t[gone]\n \n \tEOF\n \ttest_when_finished \"git config branch.master.merge refs/heads/master\" &&\n-- \n2.6.0\n"},{"id":"271020","messageId":"1443807546-5985-9-git-send-email-Karthik.188@gmail.com","threadId":"40474","inReplyTo":"1443807546-5985-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH 8/9] branch: use ref-filter printing APIs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-10-02T17:39:05Z","receivedAt":"2015-10-02T17:39:05Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Port branch.c to use ref-filter APIs for printing. This clears out\nmost of the code used in branch.c for printing and replaces them with\ncalls made to the ref-filter library.\n\nIntroduce get_format() which gets the format required for printing of\nrefs. Make amendments to print_ref_list() to reflect these changes.\n\nSince branch.c is being ported to use ref-filter APIs to print the\nrequired data, it is constricted to the constraints of printing as per\nref-filter. Which means branch.c can only print as per the atoms\navailable in ref-filter. Hence the \"-vv\" option of 'git branch' now\nprints the upstream and its tracking details separately as\n\"[] []\" instead of \"[: ]\".\n\nMake changes in /t/t6040-tracking-info.sh to reflect the change.\n\nMentored-by: Christian Couder \nMentored-by: Matthieu Moy \nSigned-off-by: Karthik Nayak \n---\n builtin/branch.c | 258 ++++++++++++-----------------------------------\n t/t6040-tracking-info.sh | 12 +--\n 2 files changed, 69 insertions(+), 201 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 67ef9f1..48fbca1 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -35,12 +35,12 @@ static unsigned char head_sha1[20];\n \n static int branch_use_color = -1;\n static char branch_colors[][COLOR_MAXLEN] = {\n-\tGIT_COLOR_RESET,\n-\tGIT_COLOR_NORMAL,\t/* PLAIN */\n-\tGIT_COLOR_RED,\t\t/* REMOTE */\n-\tGIT_COLOR_NORMAL,\t/* LOCAL */\n-\tGIT_COLOR_GREEN,\t/* CURRENT */\n-\tGIT_COLOR_BLUE,\t\t/* UPSTREAM */\n+\t\"%(color:reset)\",\n+\t\"%(color:reset)\",\t/* PLAIN */\n+\t\"%(color:red)\",\t\t/* REMOTE */\n+\t\"%(color:reset)\",\t/* LOCAL */\n+\t\"%(color:green)\",\t/* CURRENT */\n+\t\"%(color:blue)\",\t/* UPSTREAM */\n };\n enum color_branch {\n \tBRANCH_COLOR_RESET = 0,\n@@ -271,192 +271,6 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \treturn(ret);\n }\n \n-static void fill_tracking_info(struct strbuf *stat, const char *branch_name,\n-\t\tint show_upstream_ref)\n-{\n-\tint ours, theirs;\n-\tchar *ref = NULL;\n-\tstruct branch *branch = branch_get(branch_name);\n-\tconst char *upstream;\n-\tstruct strbuf fancy = STRBUF_INIT;\n-\tint upstream_is_gone = 0;\n-\tint added_decoration = 1;\n-\n-\tif (stat_tracking_info(branch, &ours, &theirs, &upstream) < 0) {\n-\t\tif (!upstream)\n-\t\t\treturn;\n-\t\tupstream_is_gone = 1;\n-\t}\n-\n-\tif (show_upstream_ref) {\n-\t\tref = shorten_unambiguous_ref(upstream, 0);\n-\t\tif (want_color(branch_use_color))\n-\t\t\tstrbuf_addf(&fancy, \"%s%s%s\",\n-\t\t\t\t\tbranch_get_color(BRANCH_COLOR_UPSTREAM),\n-\t\t\t\t\tref, branch_get_color(BRANCH_COLOR_RESET));\n-\t\telse\n-\t\t\tstrbuf_addstr(&fancy, ref);\n-\t}\n-\n-\tif (upstream_is_gone) {\n-\t\tif (show_upstream_ref)\n-\t\t\tstrbuf_addf(stat, _(\"[%s: gone]\"), fancy.buf);\n-\t\telse\n-\t\t\tadded_decoration = 0;\n-\t} else if (!ours && !theirs) {\n-\t\tif (show_upstream_ref)\n-\t\t\tstrbuf_addf(stat, _(\"[%s]\"), fancy.buf);\n-\t\telse\n-\t\t\tadded_decoration = 0;\n-\t} else if (!ours) {\n-\t\tif (show_upstream_ref)\n-\t\t\tstrbuf_addf(stat, _(\"[%s: behind %d]\"), fancy.buf, theirs);\n-\t\telse\n-\t\t\tstrbuf_addf(stat, _(\"[behind %d]\"), theirs);\n-\n-\t} else if (!theirs) {\n-\t\tif (show_upstream_ref)\n-\t\t\tstrbuf_addf(stat, _(\"[%s: ahead %d]\"), fancy.buf, ours);\n-\t\telse\n-\t\t\tstrbuf_addf(stat, _(\"[ahead %d]\"), ours);\n-\t} else {\n-\t\tif (show_upstream_ref)\n-\t\t\tstrbuf_addf(stat, _(\"[%s: ahead %d, behind %d]\"),\n-\t\t\t\t fancy.buf, ours, theirs);\n-\t\telse\n-\t\t\tstrbuf_addf(stat, _(\"[ahead %d, behind %d]\"),\n-\t\t\t\t ours, theirs);\n-\t}\n-\tstrbuf_release(&fancy);\n-\tif (added_decoration)\n-\t\tstrbuf_addch(stat, ' ');\n-\tfree(ref);\n-}\n-\n-static void add_verbose_info(struct strbuf *out, struct ref_array_item *item,\n-\t\t\t struct ref_filter *filter, const char *refname)\n-{\n-\tstruct strbuf subject = STRBUF_INIT, stat = STRBUF_INIT;\n-\tconst char *sub = _(\" **** invalid ref ****\");\n-\tstruct commit *commit = item->commit;\n-\n-\tif (!parse_commit(commit)) {\n-\t\tpp_commit_easy(CMIT_FMT_ONELINE, commit, &subject);\n-\t\tsub = subject.buf;\n-\t}\n-\n-\tif (item->kind == FILTER_REFS_BRANCHES)\n-\t\tfill_tracking_info(&stat, refname, filter->verbose > 1);\n-\n-\tstrbuf_addf(out, \" %s %s%s\",\n-\t\tfind_unique_abbrev(item->commit->object.sha1, filter->abbrev),\n-\t\tstat.buf, sub);\n-\tstrbuf_release(&stat);\n-\tstrbuf_release(&subject);\n-}\n-\n-/*\n- * This is duplicated in ref-filter.c, will be removed when we adopt\n- * ref-filter's printing APIs.\n- */\n-static char *get_head_description(void)\n-{\n-\tstruct strbuf desc = STRBUF_INIT;\n-\tstruct wt_status_state state;\n-\tmemset(&state, 0, sizeof(state));\n-\twt_status_get_state(&state, 1);\n-\tif (state.rebase_in_progress ||\n-\t state.rebase_interactive_in_progress)\n-\t\tstrbuf_addf(&desc, _(\"(no branch, rebasing %s)\"),\n-\t\t\t state.branch);\n-\telse if (state.bisect_in_progress)\n-\t\tstrbuf_addf(&desc, _(\"(no branch, bisect started on %s)\"),\n-\t\t\t state.branch);\n-\telse if (state.detached_from) {\n-\t\t/* TRANSLATORS: make sure these match _(\"HEAD detached at \")\n-\t\t and _(\"HEAD detached from \") in wt-status.c */\n-\t\tif (state.detached_at)\n-\t\t\tstrbuf_addf(&desc, _(\"(HEAD detached at %s)\"),\n-\t\t\t\tstate.detached_from);\n-\t\telse\n-\t\t\tstrbuf_addf(&desc, _(\"(HEAD detached from %s)\"),\n-\t\t\t\tstate.detached_from);\n-\t}\n-\telse\n-\t\tstrbuf_addstr(&desc, _(\"(no branch)\"));\n-\tfree(state.branch);\n-\tfree(state.onto);\n-\tfree(state.detached_from);\n-\treturn strbuf_detach(&desc, NULL);\n-}\n-\n-static void format_and_print_ref_item(struct ref_array_item *item, int maxwidth,\n-\t\t\t\t struct ref_filter *filter, const char *remote_prefix)\n-{\n-\tchar c;\n-\tint current = 0;\n-\tint color;\n-\tstruct strbuf out = STRBUF_INIT, name = STRBUF_INIT;\n-\tconst char *prefix = \"\";\n-\tconst char *desc = item->refname;\n-\tchar *to_free = NULL;\n-\n-\tswitch (item->kind) {\n-\tcase FILTER_REFS_BRANCHES:\n-\t\tskip_prefix(desc, \"refs/heads/\", &desc);\n-\t\tif (!filter->detached && !strcmp(desc, head))\n-\t\t\tcurrent = 1;\n-\t\telse\n-\t\t\tcolor = BRANCH_COLOR_LOCAL;\n-\t\tbreak;\n-\tcase FILTER_REFS_REMOTES:\n-\t\tskip_prefix(desc, \"refs/remotes/\", &desc);\n-\t\tcolor = BRANCH_COLOR_REMOTE;\n-\t\tprefix = remote_prefix;\n-\t\tbreak;\n-\tcase FILTER_REFS_DETACHED_HEAD:\n-\t\tdesc = to_free = get_head_description();\n-\t\tcurrent = 1;\n-\t\tbreak;\n-\tdefault:\n-\t\tcolor = BRANCH_COLOR_PLAIN;\n-\t\tbreak;\n-\t}\n-\n-\tc = ' ';\n-\tif (current) {\n-\t\tc = '*';\n-\t\tcolor = BRANCH_COLOR_CURRENT;\n-\t}\n-\n-\tstrbuf_addf(&name, \"%s%s\", prefix, desc);\n-\tif (filter->verbose) {\n-\t\tint utf8_compensation = strlen(name.buf) - utf8_strwidth(name.buf);\n-\t\tstrbuf_addf(&out, \"%c %s%-*s%s\", c, branch_get_color(color),\n-\t\t\t maxwidth + utf8_compensation, name.buf,\n-\t\t\t branch_get_color(BRANCH_COLOR_RESET));\n-\t} else\n-\t\tstrbuf_addf(&out, \"%c %s%s%s\", c, branch_get_color(color),\n-\t\t\t name.buf, branch_get_color(BRANCH_COLOR_RESET));\n-\n-\tif (item->symref) {\n-\t\tskip_prefix(item->symref, \"refs/remotes/\", &desc);\n-\t\tstrbuf_addf(&out, \" -> %s\", desc);\n-\t}\n-\telse if (filter->verbose)\n-\t\t/* \" f7c0c00 [ahead 58, behind 197] vcs-svn: drop obj_pool.h\" */\n-\t\tadd_verbose_info(&out, item, filter, desc);\n-\tif (column_active(colopts)) {\n-\t\tassert(!filter->verbose && \"--column and --verbose are incompatible\");\n-\t\tstring_list_append(&output, out.buf);\n-\t} else {\n-\t\tprintf(\"%s\\n\", out.buf);\n-\t}\n-\tstrbuf_release(&name);\n-\tstrbuf_release(&out);\n-\tfree(to_free);\n-}\n-\n static int calc_maxwidth(struct ref_array *refs, int remote_bonus)\n {\n \tint i, max = 0;\n@@ -477,12 +291,54 @@ static int calc_maxwidth(struct ref_array *refs, int remote_bonus)\n \treturn max;\n }\n \n+static char *get_format(struct ref_filter *filter, int maxwidth, const char *remote_prefix)\n+{\n+\tchar *remote = NULL;\n+\tchar *local = NULL;\n+\tchar *final = NULL;\n+\n+\tif (filter->verbose) {\n+\t\tif (filter->verbose > 1)\n+\t\t\tlocal = xstrfmt(\"%%(if)%%(HEAD)%%(then)* %s%%(else) %%(end)%%(align:%d,left)%%(refname:short)%%(end)%s\"\n+\t\t\t\t\t\" %%(objectname:short,7) %%(if)%%(upstream)%%(then)[%s%%(upstream:short)%s] %%(end)\"\n+\t\t\t\t\t\"%%(if)%%(upstream:track)%%(then)%%(upstream:track) %%(end)%%(contents:subject)\",\n+\t\t\t\t\tbranch_get_color(BRANCH_COLOR_CURRENT), maxwidth, branch_get_color(BRANCH_COLOR_RESET),\n+\t\t\t\t\tbranch_get_color(BRANCH_COLOR_UPSTREAM), branch_get_color(BRANCH_COLOR_RESET));\n+\n+\t\telse\n+\t\t\tlocal = xstrfmt(\"%%(if)%%(HEAD)%%(then)* %s%%(else) %%(end)%%(align:%d,left)%\"\n+\t\t\t\t\t\"%(refname:short)%%(end)%s %%(objectname:short,7) %%(if)%%(upstream:track)%\"\n+\t\t\t\t\t\"%(then)%%(upstream:track) %%(end)%%(contents:subject)\",\n+\t\t\t\t\tbranch_get_color(BRANCH_COLOR_CURRENT), maxwidth, branch_get_color(BRANCH_COLOR_RESET));\n+\n+\t\tremote = xstrfmt(\" %s%%(align:%d,left)%s%%(refname:short)%%(end)%s%%(if)%%(symref)%%(then) -> %%(symref:short)\"\n+\t\t\t\t \"%%(else) %%(objectname:short,7) %%(contents:subject)%%(end)\",\n+\t\t\t\t branch_get_color(BRANCH_COLOR_REMOTE), maxwidth,\n+\t\t\t\t remote_prefix, branch_get_color(BRANCH_COLOR_RESET));\n+\t\tfinal = xstrfmt(\"%%(if:notequals=remotes)%%(path:short)%%(then)%s%%(else)%s%%(end)\", local, remote);\n+\n+\t} else {\n+\t\tlocal = xstrfmt(\"%%(if)%%(HEAD)%%(then)* %s%%(else) %%(end)%%(refname:short)%s\",\n+\t\t\t\tbranch_get_color(BRANCH_COLOR_CURRENT), branch_get_color(BRANCH_COLOR_RESET));\n+\t\tremote = xstrfmt(\" %s%s%%(refname:short)%s%%(if)%%(symref)%%(then) -> %%(symref:short)%%(end)\",\n+\t\t\t\t branch_get_color(BRANCH_COLOR_REMOTE), remote_prefix, branch_get_color(BRANCH_COLOR_RESET));\n+\t\tfinal = xstrfmt(\"%%(if:notequals=remotes)%%(path:short)%%(then)%s%%(else)%s%%(end)\", local, remote);\n+\t}\n+\n+\tfree(local);\n+\tfree(remote);\n+\n+\treturn final;\n+}\n+\n static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sorting)\n {\n \tint i;\n \tstruct ref_array array;\n \tint maxwidth = 0;\n \tconst char *remote_prefix = \"\";\n+\tstruct strbuf out = STRBUF_INIT;\n+\tchar *format;\n \n \t/*\n \t * If we are listing more than just remote branches,\n@@ -494,12 +350,14 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin\n \n \tmemset(&array, 0, sizeof(array));\n \n-\tverify_ref_format(\"%(refname)%(symref)\");\n \tfilter_refs(&array, filter, filter->kind | FILTER_REFS_INCLUDE_BROKEN);\n \n \tif (filter->verbose)\n \t\tmaxwidth = calc_maxwidth(&array, strlen(remote_prefix));\n \n+\tformat = get_format(filter, maxwidth, remote_prefix);\n+\tverify_ref_format(format);\n+\n \t/*\n \t * If no sorting parameter is given then we default to sorting\n \t * by 'refname'. This would give us an alphabetically sorted\n@@ -511,10 +369,20 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin\n \t\tsorting = ref_default_sorting();\n \tref_array_sort(sorting, &array);\n \n-\tfor (i = 0; i < array.nr; i++)\n-\t\tformat_and_print_ref_item(array.items[i], maxwidth, filter, remote_prefix);\n+\tfor (i = 0; i < array.nr; i++) {\n+\t\tformat_ref_array_item(array.items[i], format, 0, &out);\n+\t\tif (column_active(colopts)) {\n+\t\t\tassert(!filter->verbose && \"--column and --verbose are incompatible\");\n+\t\t\tstring_list_append(&output, out.buf);\n+\t\t} else {\n+\t\t\tfwrite(out.buf, 1, out.len, stdout);\n+\t\t\tputchar('\\n');\n+\t\t}\n+\t\tstrbuf_release(&out);\n+\t}\n \n \tref_array_clear(&array);\n+\tfree(format);\n }\n \n static void rename_branch(const char *oldname, const char *newname, int force)\ndiff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh\nindex 3d5c238..d110f26 100755\n--- a/t/t6040-tracking-info.sh\n+++ b/t/t6040-tracking-info.sh\n@@ -44,7 +44,7 @@ b1 [ahead 1, behind 1] d\n b2 [ahead 1, behind 1] d\n b3 [behind 1] b\n b4 [ahead 2] f\n-b5 g\n+b5 [gone] g\n b6 c\n EOF\n \n@@ -58,11 +58,11 @@ test_expect_success 'branch -v' '\n '\n \n cat >expect <<\\EOF\n-b1 [origin/master: ahead 1, behind 1] d\n-b2 [origin/master: ahead 1, behind 1] d\n-b3 [origin/master: behind 1] b\n-b4 [origin/master: ahead 2] f\n-b5 [brokenbase: gone] g\n+b1 [origin/master] [ahead 1, behind 1] d\n+b2 [origin/master] [ahead 1, behind 1] d\n+b3 [origin/master] [behind 1] b\n+b4 [origin/master] [ahead 2] f\n+b5 [brokenbase] [gone] g\n b6 [origin/master] c\n EOF\n \n-- \n2.6.0\n"},{"id":"271019","messageId":"1443807546-5985-10-git-send-email-Karthik.188@gmail.com","threadId":"40474","inReplyTo":"1443807546-5985-1-git-send-email-Karthik.188@gmail.com","subject":"[PATCH 9/9] branch: implement '--format' option","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-10-02T17:39:06Z","receivedAt":"2015-10-02T17:39:06Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Implement the '--format' option provided by 'ref-filter'.\nThis lets the user list tags as per desired format similar\nto the implementation in 'git for-each-ref'.\n\nAdd tests and documentation for the same.\n\nMentored-by: Christian Couder \nMentored-by: Matthieu Moy \nSigned-off-by: Karthik Nayak \n---\n Documentation/git-branch.txt | 7 ++++++-\n builtin/branch.c | 14 +++++++++-----\n t/t3203-branch-output.sh | 11 +++++++++++\n 3 files changed, 26 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt\nindex 03c7af1..5227206 100644\n--- a/Documentation/git-branch.txt\n+++ b/Documentation/git-branch.txt\n@@ -12,7 +12,7 @@ SYNOPSIS\n \t[--list] [-v [--abbrev= | --no-abbrev]]\n \t[--column[=] | --no-column]\n \t[(--merged | --no-merged | --contains) []] [--sort=]\n-\t[--points-at ] [...]\n+\t[--points-at ] [--format=] [...]\n 'git branch' [--set-upstream | --track | --no-track] [-l] [-f] []\n 'git branch' (--set-upstream-to= | -u ) []\n 'git branch' --unset-upstream []\n@@ -244,6 +244,11 @@ start-point is either a local or remote-tracking branch.\n --points-at ::\n \tOnly list branches of the given object.\n \n+--format ::\n+\tA string that interpolates `%(fieldname)` from the object\n+\tpointed at by a ref being shown. The format is the same as\n+\tthat of linkgit:git-for-each-ref[1].\n+\n Examples\n --------\n \ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 48fbca1..ae3ecfb 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -27,6 +27,7 @@ static const char * const builtin_branch_usage[] = {\n \tN_(\"git branch [] [-r] (-d | -D) ...\"),\n \tN_(\"git branch [] (-m | -M) [] \"),\n \tN_(\"git branch [] [-r | -a] [--points-at]\"),\n+\tN_(\"git branch [] [-r | -a] [--format]\"),\n \tNULL\n };\n \n@@ -331,14 +332,14 @@ static char *get_format(struct ref_filter *filter, int maxwidth, const char *rem\n \treturn final;\n }\n \n-static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sorting)\n+static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sorting, const char *format)\n {\n \tint i;\n \tstruct ref_array array;\n \tint maxwidth = 0;\n \tconst char *remote_prefix = \"\";\n \tstruct strbuf out = STRBUF_INIT;\n-\tchar *format;\n+\tchar *to_free = NULL;\n \n \t/*\n \t * If we are listing more than just remote branches,\n@@ -355,7 +356,8 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin\n \tif (filter->verbose)\n \t\tmaxwidth = calc_maxwidth(&array, strlen(remote_prefix));\n \n-\tformat = get_format(filter, maxwidth, remote_prefix);\n+\tif (!format)\n+\t\tformat = to_free = get_format(filter, maxwidth, remote_prefix);\n \tverify_ref_format(format);\n \n \t/*\n@@ -382,7 +384,7 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin\n \t}\n \n \tref_array_clear(&array);\n-\tfree(format);\n+\tfree(to_free);\n }\n \n static void rename_branch(const char *oldname, const char *newname, int force)\n@@ -483,6 +485,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \tenum branch_track track;\n \tstruct ref_filter filter;\n \tstatic struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;\n+\tconst char *format = NULL;\n \n \tstruct option options[] = {\n \t\tOPT_GROUP(N_(\"Generic options\")),\n@@ -523,6 +526,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\t\tOPTION_CALLBACK, 0, \"points-at\", &filter.points_at, N_(\"object\"),\n \t\t\tN_(\"print only branches of the object\"), 0, parse_opt_object_name\n \t\t},\n+\t\tOPT_STRING( 0 , \"format\", &format, N_(\"format\"), N_(\"format to use for the output\")),\n \t\tOPT_END(),\n \t};\n \n@@ -581,7 +585,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tif ((filter.kind & FILTER_REFS_BRANCHES) && filter.detached)\n \t\t\tfilter.kind |= FILTER_REFS_DETACHED_HEAD;\n \t\tfilter.name_patterns = argv;\n-\t\tprint_ref_list(&filter, sorting);\n+\t\tprint_ref_list(&filter, sorting, format);\n \t\tprint_columns(&output, colopts, NULL);\n \t\tstring_list_clear(&output, 0);\n \t\treturn 0;\ndiff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh\nindex f1ae5ff..a475ff1 100755\n--- a/t/t3203-branch-output.sh\n+++ b/t/t3203-branch-output.sh\n@@ -163,4 +163,15 @@ test_expect_success 'git branch --points-at option' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'git branch --format option' '\n+\tcat >expect <<-\\EOF &&\n+\tRefname is (HEAD detached from fromtag)\n+\tRefname is refs/heads/branch-one\n+\tRefname is refs/heads/branch-two\n+\tRefname is refs/heads/master\n+\tEOF\n+\tgit branch --format=\"Refname is %(refname)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.6.0\n"},{"id":"271033","messageId":"xmqqpp0x3spo.fsf@gitster.mtv.corp.google.com","threadId":"40474","inReplyTo":"1443807546-5985-2-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH 1/9] ref-filter: implement %(if), %(then), and %(else) atoms","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-02T20:45:23Z","receivedAt":"2015-10-02T20:45:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> +static int is_empty(const char * s){\n> +\twhile (*s != '\\0') {\n> +\t\tif (!isspace(*s))\n> +\t\t\treturn 0;\n> +\t\ts++;\n> +\t}\n> +\treturn 1;\n> +}\n\nMy knee-jerk reaction was \"why is space so special?\", but if a\ncaller really cared, it can do \"%(if:not_equal=)%(something)%(then)\"\nto unignore spaces in %(something), so it is not a huge deal. It\nmay be that ignoring spaces when checking if something is empty is\nso common that this default is useful---I cannot tell offhand.\n\n\n> +if::\n> +\tUsed as %(if)..%(then)..(%end) or %(if)..%(then)..%(else)..%(end).\n> +\tIf there is an atom with value or string literal after the\n> +\t%(if) then everything after the %(then) is printed, else if\n> +\tthe %(else) atom is used, then everything after %(else) is\n> +\tprinted.\n\nI notice that \"we ignore space when evaluating the string before\n%(then)\" is not mentioned here. That fact, and an example or two\nthat illustrates the situation where this \"ignore spaces\" behaviour\nis useful, would be a good thing to document here.\n\nThanks.\n"},{"id":"271034","messageId":"xmqqlhbl3sb6.fsf@gitster.mtv.corp.google.com","threadId":"40474","inReplyTo":"1443807546-5985-3-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH 2/9] ref-filter: implement %(if:equals=) and %(if:notequals=)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-02T20:54:05Z","receivedAt":"2015-10-02T20:54:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> Implement %(if:equals=) wherein the if condition is only\n> satisfied if the value obtained between the %(if:...) and %(then) atom\n> is the same as the given ''.\n>\n> Similarly, implement (if:notequals=) wherein the if condition\n> is only satisfied if the value obtained between the %(if:...) and\n> %(then) atom is differnt from the given ''.\n>\n> Add tests and Documentation for the same.\n>\n> Mentored-by: Christian Couder \n> Mentored-by: Matthieu Moy \n> Signed-off-by: Karthik Nayak \n> ---\n\nThe fast that the patch touches only the narrow parts that are\nspecific to %(if),%(then) and %(else) and does not have to touch any\ngeneric part (other than the populate_value() parser for obvious\nreasons) is a good signal that tells us that the basic structure of\nthe code is very sound. I very much like the direction in which\nthis series is going ;-)\n"},{"id":"271036","messageId":"xmqqd1wx3rxt.fsf@gitster.mtv.corp.google.com","threadId":"40474","inReplyTo":"1443807546-5985-5-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH 4/9] ref-filter: modify \"%(objectname:short)\" to take length","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-02T21:02:06Z","receivedAt":"2015-10-02T21:02:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> Add support for %(objectname:short,) which would print the\n> abbreviated unique objectname of given length. When no length is\n> specified 7 is used. The minimum length is 4.\n\nIt would have to be \"short=\", not \"short,\", if I\nrecall the previous discussion on width=, etc., on the %(align)\natom.\n"},{"id":"271046","messageId":"CAOLa=ZQ31+gEktXh=QJJL+_McRMHhQynHAg__DpfuUsdc-5uWg@mail.gmail.com","threadId":"40474","inReplyTo":"xmqqpp0x3spo.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/9] ref-filter: implement %(if), %(then), and %(else) atoms","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-10-03T07:05:59Z","receivedAt":"2015-10-03T07:05:59Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Sat, Oct 3, 2015 at 2:15 AM, Junio C Hamano wrote:\n> Karthik Nayak writes:\n>\n>> +static int is_empty(const char * s){\n>> + while (*s != '\\0') {\n>> + if (!isspace(*s))\n>> + return 0;\n>> + s++;\n>> + }\n>> + return 1;\n>> +}\n>\n> My knee-jerk reaction was \"why is space so special?\", but if a\n> caller really cared, it can do \"%(if:not_equal=)%(something)%(then)\"\n> to unignore spaces in %(something), so it is not a huge deal. It\n> may be that ignoring spaces when checking if something is empty is\n> so common that this default is useful---I cannot tell offhand.\n>\n>\n\nMy reason for doing this was the \"HEAD\" atom which prints \"*\" or \" \",\nhence I thought strings with only spaces in general shouldn't be accepted.\n\n>> +if::\n>> + Used as %(if)..%(then)..(%end) or %(if)..%(then)..%(else)..%(end).\n>> + If there is an atom with value or string literal after the\n>> + %(if) then everything after the %(then) is printed, else if\n>> + the %(else) atom is used, then everything after %(else) is\n>> + printed.\n>\n> I notice that \"we ignore space when evaluating the string before\n> %(then)\" is not mentioned here. That fact, and an example or two\n> that illustrates the situation where this \"ignore spaces\" behaviour\n> is useful, would be a good thing to document here.\n>\n\nOh Yes! Will do that :)\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"271047","messageId":"CAOLa=ZQ3tJQ6VD6RBBOU_41=75WB1vrD6ASk5HecnnJse0cJ+Q@mail.gmail.com","threadId":"40474","inReplyTo":"xmqqlhbl3sb6.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/9] ref-filter: implement %(if:equals=) and %(if:notequals=)","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-10-03T07:14:39Z","receivedAt":"2015-10-03T07:14:39Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Sat, Oct 3, 2015 at 2:24 AM, Junio C Hamano wrote:\n> Karthik Nayak writes:\n>\n>> Implement %(if:equals=) wherein the if condition is only\n>> satisfied if the value obtained between the %(if:...) and %(then) atom\n>> is the same as the given ''.\n>>\n>> Similarly, implement (if:notequals=) wherein the if condition\n>> is only satisfied if the value obtained between the %(if:...) and\n>> %(then) atom is differnt from the given ''.\n>>\n>> Add tests and Documentation for the same.\n>>\n>> Mentored-by: Christian Couder \n>> Mentored-by: Matthieu Moy \n>> Signed-off-by: Karthik Nayak \n>> ---\n>\n> The fast that the patch touches only the narrow parts that are\n> specific to %(if),%(then) and %(else) and does not have to touch any\n> generic part (other than the populate_value() parser for obvious\n> reasons) is a good signal that tells us that the basic structure of\n> the code is very sound. I very much like the direction in which\n> this series is going ;-)\n>\n\nThats nice to hear :)\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"271048","messageId":"CAOLa=ZQVXwQzv8io=Z+4eCrpArF4t=qwwaw+=59G4xRTPmhF_A@mail.gmail.com","threadId":"40474","inReplyTo":"xmqqd1wx3rxt.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 4/9] ref-filter: modify \"%(objectname:short)\" to take length","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-10-03T07:28:53Z","receivedAt":"2015-10-03T07:28:53Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Sat, Oct 3, 2015 at 2:32 AM, Junio C Hamano wrote:\n> Karthik Nayak writes:\n>\n>> Add support for %(objectname:short,) which would print the\n>> abbreviated unique objectname of given length. When no length is\n>> specified 7 is used. The minimum length is 4.\n>\n> It would have to be \"short=\", not \"short,\", if I\n> recall the previous discussion on width=, etc., on the %(align)\n> atom.\n\nYea, I got confused by \"The \"align:\" is followed by `` and\n`` in any order\nseparated by a comma\". Will change, Thanks.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"271051","messageId":"vpq8u7kb89o.fsf@grenoble-inp.fr","threadId":"40474","inReplyTo":"1443807546-5985-2-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH 1/9] ref-filter: implement %(if), %(then), and %(else) atoms","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-10-03T09:39:47Z","receivedAt":"2015-10-03T09:39:47Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Karthik Nayak writes:\n\n> Implement %(if), %(then) and %(else) atoms. Used as\n> %(if)..%(then)..%(end) or %(if)..%(then)..%(else)..%(end).\n\nI prefer ... to .., which often means \"interval\" as in HEAD^^..HEAD.\n\n> If there is an atom with value or string literal after the %(if)\n\nI find this explanation hard to read, and ambiguous: what does \"atom\nwith value\" mean?\n\n> then everything after the %(then) is printed, else if the %(else) atom\n> is used, then everything after %(else) is printed. If the string\n> contains only whitespaces, then it is not considered.\n\n\"the string\" is ambiguous again. I guess it's \"what's between %(if) and\n%(then)\", but it could be clearer. And it's not clear what \"not\nconsidered\" means.\n\nMy take on it:\n\nImplement %(if), %(then) and %(else) atoms. Used as\n%(if)...%(then)...%(end) or %(if)...%(then)...%(else)...%(end). If the\nformat string between %(if) and %(then) expands to an empty string, or\nto only whitespaces, then the string following %(then) is printed.\nOtherwise, the string following %(else), if any, is printed.\n\n> +When a scripting language specific quoting is in effect,\n\nThis may not be immediately clear to the reader. I'd add explicitly:\n\nWhen a scripting language specific quoting is in effect (i.e. one of\n`--shell`, `--perl`, `--python`, `--tcl` is used), ...\n\n> EXAMPLES\n> --------\n\nThis is just the context of the patch, but I read it as a hint that we\ncould add some examples with complex --format usage to illustrate the\ntheory above.\n\n> +\tif (if_then_else->condition_satisfied && if_then_else->else_atom) {\n// cs && else\n> +\t\tstrbuf_reset(&cur->output);\n> +\t\tpop_stack_element(&cur);\n> +\t} else if (if_then_else->else_atom) {\n// !cs && else\n> +\t\tstrbuf_swap(&cur->output, &prev->output);\n> +\t\tstrbuf_reset(&cur->output);\n> +\t\tpop_stack_element(&cur);\n> +\t} else if (!if_then_else->condition_satisfied)\n// !cs && !else\n> +\t\tstrbuf_reset(&cur->output);\n\nThis if/else if/else if looks hard to read to me. I had to add the\ncomments above as a note to myself to get the actual full condition for\n3 branches.\n\nThe reasoning would be clearer to me as:\n\nif (if_then_else->else_atom) {\n\t/*\n\t * There is an %(else) atom: we need to drop one state from the\n\t * stack, either the %(else) branch if the condition is satisfied, or\n\t * the %(then) branch if it isn't.\n\t */\n\tif (if_then_else->condition_satisfied) {\n\t\tstrbuf_reset(&cur->output);\n\t\tpop_stack_element(&cur);\n\t} else {\n\t\tstrbuf_swap(&cur->output, &prev->output);\n\t\tstrbuf_reset(&cur->output);\n\t\tpop_stack_element(&cur);\n\t}\n} else if (if_then_else->condition_satisfied)\n\t/*\n * No %(else) atom: just drop the %(then) branch if the\n\t * condition is not satisfied.\n */\n\tstrbuf_reset(&cur->output);\n\n> +static void if_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)\n> +{\n> +\tstruct ref_formatting_stack *new;\n> +\tstruct if_then_else *if_then_else = xcalloc(sizeof(struct if_then_else), 1);\n> +\n> +\tif_then_else->if_atom = 1;\n\nDo you ever use this \"if_atom\"? It doesn't seem so in the current patch,\nand it seems like a tautology to me: if you have a struct if_then_else,\nthen you have seen the %(if).\n\n> +static int is_empty(const char * s){\n\nchar * s -> char *s\n\n> +static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)\n> +{\n> +\tstruct ref_formatting_stack *cur = state->stack;\n> +\tstruct if_then_else *if_then_else = (struct if_then_else *)cur->at_end_data;\n> +\n> +\tif (!if_then_else)\n> +\t\tdie(_(\"format: %%(then) atom used without an %%(if) atom\"));\n\nYou've just casted at_end_data to if_then_else. if_then_else being not\nNULL does not mean that it is properly typed. It can be the at_end_data\nof another opening atom. What happens if you use\n%(align)foo%(then)bar%(end)?\n\nOne way to be safer would be to check that cur->at_end does point to\nif_then_else_handler. Or add information to struct ref_formatting_stack\n(a Boolean is_if_then_else or an enum).\n\nAlso, you need to check that if_then_else->then_atom is not 1.\n\n> +static void else_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)\n> +{\n> +\tstruct ref_formatting_stack *prev = state->stack;\n> +\tstruct if_then_else *if_then_else = (struct if_then_else *)state->stack->at_end_data;\n> +\n> +\tif (!if_then_else)\n> +\t\tdie(_(\"format: %%(else) atom used without an %%(if) atom\"));\n\nSame as above, I guess (not tested) %(align)...%(else) is accepted.\n\n> +\tif (!if_then_else->then_atom)\n> +\t\tdie(_(\"format: %%(else) atom used without a %%(then) atom\"));\n> +\tif_then_else->else_atom = 1;\n> +\tpush_stack_element(&state->stack);\n\nSo, while parsing the %(else)...%(end), the stack contains both the\n%(then)...%(else) part, and the %(else)...%(end).\n\nI'm wondering if we can simplify this. We already know if the condition\nis satisfied, and if it's not, we can just drop the %(then) part right\nnow, and write to the top of the stack normally (the %(end) atom will\nonly have to pop the string normally). But if the condition is not\nsatisfied, we need to preserve the %(then) part and need to do something\nabout the %(else).\n\n> -\tcurrent->at_end(current);\n> +\tcurrent->at_end(&state->stack);\n> +\n> +\t/* Stack may have been popped, hence reset the current pointer */\n\nI'd say explicitly \"... may have been popped within at_end, hence ...\"\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"271052","messageId":"vpqwpv49so7.fsf@grenoble-inp.fr","threadId":"40474","inReplyTo":"1443807546-5985-4-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH 3/9] ref-filter: add support for %(path) atom","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-10-03T10:02:00Z","receivedAt":"2015-10-03T10:02:00Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Karthik Nayak writes:\n\n> This adds %(path) and %(path:short) atoms. The %(path) atom will print\n> the path of the given ref, while %(path:short) will only print the\n> subdirectory of the given ref.\n\nWhat does \"path\" mean in this context? How is it different from\n%(refname)?\n\nI found the answer below, but I could not guess from the doc and commit\nmessage. Actually, I'm not sure %(path) is the right name. If you want\nthe \"file/directory\" analogy, then %(dirname) would be better.\n\n> +\t\t} else if (match_atom_name(name, \"path\", &valp)) {\n> +\t\t\tconst char *sp, *ep;\n> +\n> +\t\t\tif (ref->kind & FILTER_REFS_DETACHED_HEAD)\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tsp = strchr(ref->refname, '/');\n> +\t\t\tep = strchr(++sp, '/');\n\nThis assumes you have two / in the fullrefname. It is normally the case,\nbut one can also create eg. refs/foo references. AFAIK, in this case sp\nwill be NULL, and you're then calling strchr(++NULL) which may segfault.\n\n> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\n> index d7f7a18..5557657 100755\n> --- a/t/t6302-for-each-ref-filter.sh\n> +++ b/t/t6302-for-each-ref-filter.sh\n> @@ -312,6 +312,7 @@ test_expect_success 'check %(if:equals=)' '\n> \ttest_cmp expect actual\n> '\n> \n> +\n> test_expect_success 'check %(if:notequals=)' '\n\nUseless new blank line.\n\n> +test_expect_success 'check %(path)' '\n> +\tgit for-each-ref --format=\"%(path)\" >actual &&\n> +\tcat >expect <<-\\EOF &&\n> +\trefs/heads\n\nYou should add eg.\n\ngit update-ref refs/foo HEAD\ngit update-ref refs/foodir/bar/boz HEAD\n\nbefore the test to check and document the behavior for such refnames.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"271054","messageId":"vpqoagg9n2l.fsf@grenoble-inp.fr","threadId":"40474","inReplyTo":"1443807546-5985-7-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH 6/9] ref-filter: introduce format_ref_array_item()","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-10-03T12:02:58Z","receivedAt":"2015-10-03T12:02:58Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Karthik Nayak writes:\n\n> Introduce format_ref_array_item() which will output the details of a\n> given ref_array_item as per the given format and quote_style to the\n> given strbuf.\n\nWhy do you need it in this series and you could do without for tag?\n\nGoing through PATCH 8/9, I guess there's something related to --column,\nbut tag also has --column so I'm puzzled.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"271055","messageId":"vpqio6o9mu1.fsf@grenoble-inp.fr","threadId":"40474","inReplyTo":"1443807546-5985-8-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH 7/9] ref-filter: make %(upstream:track) prints \"[gone]\" for invalid upstreams","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-10-03T12:08:06Z","receivedAt":"2015-10-03T12:08:06Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Karthik Nayak writes:\n\n> diff --git a/ref-filter.c b/ref-filter.c\n> index 099acd6..48b06e3 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -1133,8 +1133,10 @@ static void populate_value(struct ref_array_item *ref)\n> \t\t\t\tchar buf[40];\n> \n> \t\t\t\tif (stat_tracking_info(branch, &num_ours,\n> -\t\t\t\t\t\t &num_theirs, NULL))\n> +\t\t\t\t\t\t &num_theirs, NULL)) {\n> +\t\t\t\t\tv->s = xstrdup(\"[gone]\");\n\nI think just \"[gone]\" without the xstrdup is OK, and avoids leaking one\nstring.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"271056","messageId":"vpqvbao86pj.fsf@grenoble-inp.fr","threadId":"40474","inReplyTo":"1443807546-5985-9-git-send-email-Karthik.188@gmail.com","subject":"Re: [PATCH 8/9] branch: use ref-filter printing APIs","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-10-03T12:41:44Z","receivedAt":"2015-10-03T12:41:44Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Karthik Nayak writes:\n\n> -\tif (upstream_is_gone) {\n> -\t\tif (show_upstream_ref)\n> -\t\t\tstrbuf_addf(stat, _(\"[%s: gone]\"), fancy.buf);\n\nThe old string was translated, and you're replacing it with one which\nisn't.\n\nI'm not a big fan of translation, so that change doesn't disturb me, but\npeople used to having \"git branch\" talk their native language here may\ncare.\n\nBe careful: translation should happen only in porcelain. Typicall,\n\"for-each-ref\" should anyway continue saying \"gone\" in english whatever\nthe configuration is.\n\nSee e.g. 7a76c28 (status: disable translation when --porcelain is used,\n2014-03-20) for an example of translation only for porcelain (that was\nfor status, but also for ahead/behind/gone).\n\n> +static char *get_format(struct ref_filter *filter, int maxwidth, const char *remote_prefix)\n\nI'd call that \"build_format\", since \"get_...\" usually implies that the\nvalue exists already and you're retrieving it.\n\n> +{\n> +\tchar *remote = NULL;\n> +\tchar *local = NULL;\n> +\tchar *final = NULL;\n> +\n> +\tif (filter->verbose) {\n> +\t\tif (filter->verbose > 1)\n> +\t\t\tlocal = xstrfmt(\"%%(if)%%(HEAD)%%(then)* %s%%(else) %%(end)%%(align:%d,left)%%(refname:short)%%(end)%s\"\n> +\t\t\t\t\t\" %%(objectname:short,7) %%(if)%%(upstream)%%(then)[%s%%(upstream:short)%s] %%(end)\"\n> +\t\t\t\t\t\"%%(if)%%(upstream:track)%%(then)%%(upstream:track) %%(end)%%(contents:subject)\",\n> +\t\t\t\t\tbranch_get_color(BRANCH_COLOR_CURRENT), maxwidth, branch_get_color(BRANCH_COLOR_RESET),\n> +\t\t\t\t\tbranch_get_color(BRANCH_COLOR_UPSTREAM), branch_get_color(BRANCH_COLOR_RESET));\n\nOK, that is a hell of a block of code ;-).\n\nYou should factor the common portions out of these if/else. C allows you\nto write several string litteral next to each other, so you can eg.\n\n#define STAR_IF_HEAD \"%%(if)%%(HEAD)%%(then)* %s%%(else) %%(end)\"\n#define ALIGNED_REFNAME \"%%(align:%d,left)%%(refname:short)%%(end)\"\n\nand then\n\nSTAR_IF_HEAD ALIGNED_REFNAME ...\n\nActually, this is not a performance-cricical piece of code at all, so I\nthink it's even better to build an strbuf little by little using\nrepeated strbuf_addf calls. This way you can do things like\n\nstrbuf_addf(fmt, \"%%(if)%%(HEAD)%%(then)* %s%%(else) %%(end)\",\n\t branch_get_color(BRANCH_COLOR_CURRENT));\nstrbuf_addf(fmt, \"%%(align:%d,left)%%(refname:short)%%(end)\", maxwidth);\n\nwhich makes it much easier to pair the %s and the corresponding\nbranch_get_color() or the %d and maxwidth.\n\nBut fundamentally, I think this function is doing the right thing. The\nfunction is a bit complex (I think it will be much nicer to read when\nbetter factored), but 1) it allows getting rid of a lot of specific code\nand 2) it's a proof that --format is expressive enough.\n\n> @@ -58,11 +58,11 @@ test_expect_success 'branch -v' '\n> '\n> \n> cat >expect <<\\EOF\n> -b1 [origin/master: ahead 1, behind 1] d\n> -b2 [origin/master: ahead 1, behind 1] d\n> -b3 [origin/master: behind 1] b\n> -b4 [origin/master: ahead 2] f\n> -b5 [brokenbase: gone] g\n> +b1 [origin/master] [ahead 1, behind 1] d\n> +b2 [origin/master] [ahead 1, behind 1] d\n> +b3 [origin/master] [behind 1] b\n> +b4 [origin/master] [ahead 2] f\n> +b5 [brokenbase] [gone] g\n\nThis corresponds to this part of the commit message:\n\n> Since branch.c is being ported to use ref-filter APIs to print the\n> required data, it is constricted to the constraints of printing as per\n> ref-filter. Which means branch.c can only print as per the atoms\n> available in ref-filter. Hence the \"-vv\" option of 'git branch' now\n> prints the upstream and its tracking details separately as\n> \"[] []\" instead of \"[: info>]\".\n> \n> Make changes in /t/t6040-tracking-info.sh to reflect the change.\n\nnit: /t/t... -> t/t... (remove leading /)\n\nThat is a behavior change, and I actually prefered the previous one.\n\nI actually don't understand why you can't get the old syntax: the []\naround the refname come from the format string it get_format(), so you\ncould decide to change \"[%s%%(upstream:short)%s]\" to\n\"[%s%%(upstream:short)%s: \". The brackets around the status are a bit\nmore tricky, since they were here before your series. But we could have\na %(upstream:track,nobracket) to display just the status without the\nbrackets around.\n\nThen, the code must deal with up-to-date branches for which no\n\" :ahead/behind\" must be displayed. Probably one more if/then/else\nnested in the first one.\n\nSo, it seems feasible to me to keep the old behavior with the new\nsystem, even though it's going to be a bit tricky.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"271062","messageId":"xmqqpp0v3mzo.fsf@gitster.mtv.corp.google.com","threadId":"40474","inReplyTo":"vpq8u7kb89o.fsf@grenoble-inp.fr","subject":"Re: [PATCH 1/9] ref-filter: implement %(if), %(then), and %(else) atoms","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-03T17:01:15Z","receivedAt":"2015-10-03T17:01:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n> My take on it:\n>\n> Implement %(if), %(then) and %(else) atoms. Used as\n> %(if)...%(then)...%(end) or %(if)...%(then)...%(else)...%(end). If the\n> format string between %(if) and %(then) expands to an empty string, or\n> to only whitespaces, then the string following %(then) is printed.\n> Otherwise, the string following %(else), if any, is printed.\n>\n>> +When a scripting language specific quoting is in effect,\n>\n> This may not be immediately clear to the reader. I'd add explicitly:\n>\n> When a scripting language specific quoting is in effect (i.e. one of\n> `--shell`, `--perl`, `--python`, `--tcl` is used), ...\n\nI found all the suggestions very good, except that the distinction\nbetween \"expands to\" and \"is printed\" bothers me a bit, as they want\nto mean exactly the same thing (imagine this whole thing were inside\nanother %(if)...%(then)).\n\n>> EXAMPLES\n>> --------\n>\n> This is just the context of the patch, but I read it as a hint that we\n> could add some examples with complex --format usage to illustrate the\n> theory above.\n\nYes ;-)\n\n>> +static int is_empty(const char * s){\n>\n> char * s -> char *s\n\nAlso \"{\" must sit alone on the next line.\n\n>> +static void then_atom_handler(struct atom_value *atomv, struct\n>> ref_formatting_state *state)\n>> +{\n>> +\tstruct ref_formatting_stack *cur = state->stack;\n>> +\tstruct if_then_else *if_then_else = (struct if_then_else *)cur->at_end_data;\n>> +\n>> +\tif (!if_then_else)\n>> +\t\tdie(_(\"format: %%(then) atom used without an %%(if) atom\"));\n>\n> You've just casted at_end_data to if_then_else. if_then_else being not\n> NULL does not mean that it is properly typed. It can be the at_end_data\n> of another opening atom. What happens if you use\n> %(align)foo%(then)bar%(end)?\n>\n> One way to be safer would be to check that cur->at_end does point to\n> if_then_else_handler.\n\nGood eyes. Thanks.\n\n> So, while parsing the %(else)...%(end), the stack contains both the\n> %(then)...%(else) part, and the %(else)...%(end).\n>\n> I'm wondering if we can simplify this. We already know if the condition\n> is satisfied, and if it's not, we can just drop the %(then) part right\n> now, and write to the top of the stack normally (the %(end) atom will\n> only have to pop the string normally). But if the condition is not\n> satisfied, we need to preserve the %(then) part and need to do something\n> about the %(else).\n\nGood suggestion.\n\nThanks.\n"},{"id":"271082","messageId":"vpqr3lb6oy7.fsf@grenoble-inp.fr","threadId":"40474","inReplyTo":"xmqqpp0v3mzo.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/9] ref-filter: implement %(if), %(then), and %(else) atoms","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-10-04T08:02:56Z","receivedAt":"2015-10-04T08:02:56Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano writes:\n\n> Matthieu Moy writes:\n>\n>> My take on it:\n>>\n>> Implement %(if), %(then) and %(else) atoms. Used as\n>> %(if)...%(then)...%(end) or %(if)...%(then)...%(else)...%(end). If the\n>> format string between %(if) and %(then) expands to an empty string, or\n>> to only whitespaces, then the string following %(then) is printed.\n>> Otherwise, the string following %(else), if any, is printed.\n>\n> I found all the suggestions very good, except that the distinction\n> between \"expands to\" and \"is printed\" bothers me a bit, as they want\n> to mean exactly the same thing (imagine this whole thing were inside\n> another %(if)...%(then)).\n\nTrue. Then let me try again:\n\nImplement %(if), %(then) and %(else) atoms. Used as\n%(if)...%(then)...%(end) or %(if)...%(then)...%(else)...%(end). If the\nformat string between %(if) and %(then) expands to an empty string, or\nto only whitespaces, then the whole %(if)...%(end) expands to the string\nfollowing %(then). Otherwise, it expands to the string following\n%(else), if any.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"271085","messageId":"CAOLa=ZRXvz+sAtMOmreri7QUCfwrusDQKNme4ke_8uaTkAoqiQ@mail.gmail.com","threadId":"40474","inReplyTo":"vpq8u7kb89o.fsf@grenoble-inp.fr","subject":"Re: [PATCH 1/9] ref-filter: implement %(if), %(then), and %(else) atoms","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-10-04T13:59:23Z","receivedAt":"2015-10-04T13:59:23Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Sat, Oct 3, 2015 at 3:09 PM, Matthieu Moy\n wrote:\n> Karthik Nayak writes:\n>\n>> Implement %(if), %(then) and %(else) atoms. Used as\n>> %(if)..%(then)..%(end) or %(if)..%(then)..%(else)..%(end).\n>\n> I prefer ... to .., which often means \"interval\" as in HEAD^^..HEAD.\n>\n\nSeems good, will change.\n\n>> If there is an atom with value or string literal after the %(if)\n>\n> I find this explanation hard to read, and ambiguous: what does \"atom\n> with value\" mean?\n>\n>> then everything after the %(then) is printed, else if the %(else) atom\n>> is used, then everything after %(else) is printed. If the string\n>> contains only whitespaces, then it is not considered.\n>\n> \"the string\" is ambiguous again. I guess it's \"what's between %(if) and\n> %(then)\", but it could be clearer. And it's not clear what \"not\n> considered\" means.\n>\n> My take on it:\n>\n> Implement %(if), %(then) and %(else) atoms. Used as\n> %(if)...%(then)...%(end) or %(if)...%(then)...%(else)...%(end). If the\n> format string between %(if) and %(then) expands to an empty string, or\n> to only whitespaces, then the string following %(then) is printed.\n> Otherwise, the string following %(else), if any, is printed.\n>\n\nThis (the one you sent again after Junio's suggestion, looks better),\nI'll use this thanks.\n\n>> +When a scripting language specific quoting is in effect,\n>\n> This may not be immediately clear to the reader. I'd add explicitly:\n>\n> When a scripting language specific quoting is in effect (i.e. one of\n> `--shell`, `--perl`, `--python`, `--tcl` is used), ...\n>\n\nMakes sense.\n\n>> EXAMPLES\n>> --------\n>\n> This is just the context of the patch, but I read it as a hint that we\n> could add some examples with complex --format usage to illustrate the\n> theory above.\n>\n\nI was thinking about adding :\n\nAn example to show the usage of %(if)...%(then)...%(else)...%(end).\nThis prefixes the current branch with a star.\n\n------------\n#!/bin/sh\n\ngit for-each-ref --format=\"%(if)%(HEAD)%(then)* %(else)\n%(end)%(refname:short)\" refs/heads/\n------------\n\n\nAn example to show the usage of %(if)...%(then)...%(end).\nThis adds a red color to authorname, if present\n\n------------\n#!/bin/sh\n\ngit for-each-ref\n--format=\"%(refname)%(if)%(authorname)%(then)%(color:red)%(end)\n%(authorname)\"\n------------\n\n>> + if (if_then_else->condition_satisfied && if_then_else->else_atom) {\n> // cs && else\n>> + strbuf_reset(&cur->output);\n>> + pop_stack_element(&cur);\n>> + } else if (if_then_else->else_atom) {\n> // !cs && else\n>> + strbuf_swap(&cur->output, &prev->output);\n>> + strbuf_reset(&cur->output);\n>> + pop_stack_element(&cur);\n>> + } else if (!if_then_else->condition_satisfied)\n> // !cs && !else\n>> + strbuf_reset(&cur->output);\n>\n> This if/else if/else if looks hard to read to me. I had to add the\n> comments above as a note to myself to get the actual full condition for\n> 3 branches.\n>\n> The reasoning would be clearer to me as:\n>\n> if (if_then_else->else_atom) {\n> /*\n> * There is an %(else) atom: we need to drop one state from the\n> * stack, either the %(else) branch if the condition is satisfied, or\n> * the %(then) branch if it isn't.\n> */\n> if (if_then_else->condition_satisfied) {\n> strbuf_reset(&cur->output);\n> pop_stack_element(&cur);\n> } else {\n> strbuf_swap(&cur->output, &prev->output);\n> strbuf_reset(&cur->output);\n> pop_stack_element(&cur);\n> }\n> } else if (if_then_else->condition_satisfied)\n> /*\n> * No %(else) atom: just drop the %(then) branch if the\n> * condition is not satisfied.\n> */\n> strbuf_reset(&cur->output);\n>\n\nThis looks neater thanks.\n\n>> +static void if_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)\n>> +{\n>> + struct ref_formatting_stack *new;\n>> + struct if_then_else *if_then_else = xcalloc(sizeof(struct if_then_else), 1);\n>> +\n>> + if_then_else->if_atom = 1;\n>\n> Do you ever use this \"if_atom\"? It doesn't seem so in the current patch,\n> and it seems like a tautology to me: if you have a struct if_then_else,\n> then you have seen the %(if).\n>\n\nYea I'll remove that.\n\n>> +static int is_empty(const char * s){\n>\n> char * s -> char *s\n>\n\nwill do.\n\n>> +static void then_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)\n>> +{\n>> + struct ref_formatting_stack *cur = state->stack;\n>> + struct if_then_else *if_then_else = (struct if_then_else *)cur->at_end_data;\n>> +\n>> + if (!if_then_else)\n>> + die(_(\"format: %%(then) atom used without an %%(if) atom\"));\n>\n> You've just casted at_end_data to if_then_else. if_then_else being not\n> NULL does not mean that it is properly typed. It can be the at_end_data\n> of another opening atom. What happens if you use\n> %(align)foo%(then)bar%(end)?\n>\n\nNice catch, didn't see that possibility.\n\n> One way to be safer would be to check that cur->at_end does point to\n> if_then_else_handler. Or add information to struct ref_formatting_stack\n> (a Boolean is_if_then_else or an enum).\n>\n\nChecking cur->at_end with if_then_else_handler seems good to me.\n\n> Also, you need to check that if_then_else->then_atom is not 1.\n>\n\nAh! multiple usage of the same atom.\n\n>> +static void else_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state)\n>> +{\n>> + struct ref_formatting_stack *prev = state->stack;\n>> + struct if_then_else *if_then_else = (struct if_then_else *)state->stack->at_end_data;\n>> +\n>> + if (!if_then_else)\n>> + die(_(\"format: %%(else) atom used without an %%(if) atom\"));\n>\n> Same as above, I guess (not tested) %(align)...%(else) is accepted.\n>\n\nWill change.\n\n>> + if (!if_then_else->then_atom)\n>> + die(_(\"format: %%(else) atom used without a %%(then) atom\"));\n>> + if_then_else->else_atom = 1;\n>> + push_stack_element(&state->stack);\n>\n> So, while parsing the %(else)...%(end), the stack contains both the\n> %(then)...%(else) part, and the %(else)...%(end).\n>\n> I'm wondering if we can simplify this. We already know if the condition\n> is satisfied, and if it's not, we can just drop the %(then) part right\n> now, and write to the top of the stack normally (the %(end) atom will\n> only have to pop the string normally). But if the condition is not\n> satisfied, we need to preserve the %(then) part and need to do something\n> about the %(else).\n>\n\nI wanted to do something like this the problem is append_atom() and\nappend_literal()\nwould need to be informed about which part to ignore, and this moves\nthe code's logic\nfrom the current handlers to append_atom() and append_literal(). Which I didn't\nthink was a nice way of doing this.\n\n>> - current->at_end(current);\n>> + current->at_end(&state->stack);\n>> +\n>> + /* Stack may have been popped, hence reset the current pointer */\n>\n> I'd say explicitly \"... may have been popped within at_end, hence ...\"\n>\n\nWill do.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"271088","messageId":"CAOLa=ZSdM34_J+gb_=K4-9fgNsMfseAYxUYGm_ncWZstcW+SYw@mail.gmail.com","threadId":"40474","inReplyTo":"vpqwpv49so7.fsf@grenoble-inp.fr","subject":"Re: [PATCH 3/9] ref-filter: add support for %(path) atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-10-04T17:07:21Z","receivedAt":"2015-10-04T17:07:21Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Sat, Oct 3, 2015 at 3:32 PM, Matthieu Moy\n wrote:\n> Karthik Nayak writes:\n>\n>> This adds %(path) and %(path:short) atoms. The %(path) atom will print\n>> the path of the given ref, while %(path:short) will only print the\n>> subdirectory of the given ref.\n>\n> What does \"path\" mean in this context? How is it different from\n> %(refname)?\n>\n> I found the answer below, but I could not guess from the doc and commit\n> message. Actually, I'm not sure %(path) is the right name. If you want\n> the \"file/directory\" analogy, then %(dirname) would be better.\n>\n\nNoted will change.\n\n>> + } else if (match_atom_name(name, \"path\", &valp)) {\n>> + const char *sp, *ep;\n>> +\n>> + if (ref->kind & FILTER_REFS_DETACHED_HEAD)\n>> + continue;\n>> +\n>> + sp = strchr(ref->refname, '/');\n>> + ep = strchr(++sp, '/');\n>\n> This assumes you have two / in the fullrefname. It is normally the case,\n> but one can also create eg. refs/foo references. AFAIK, in this case sp\n> will be NULL, and you're then calling strchr(++NULL) which may segfault.\n>\n\nNot really, but you made me think of another possible issue.\n\nAssume refs/foo \"sp = strchr(ref->refname, '/');\" would set sp to point at\n'/' and ++sp will now point at 'f'.\n\nThe problem now is refs/foo is supposed to print just \"refs\" whereas it'll\nprint \"refs/foo\". and %(dirname:short) is supposed to print \"\" whereas it'll\nprint \"foo\". Will look into this :)\n\n>> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\n>> index d7f7a18..5557657 100755\n>> --- a/t/t6302-for-each-ref-filter.sh\n>> +++ b/t/t6302-for-each-ref-filter.sh\n>> @@ -312,6 +312,7 @@ test_expect_success 'check %(if:equals=)' '\n>> test_cmp expect actual\n>> '\n>>\n>> +\n>> test_expect_success 'check %(if:notequals=)' '\n>\n> Useless new blank line.\n>\n\nWill remove.\n\n>> +test_expect_success 'check %(path)' '\n>> + git for-each-ref --format=\"%(path)\" >actual &&\n>> + cat >expect <<-\\EOF &&\n>> + refs/heads\n>\n> You should add eg.\n>\n> git update-ref refs/foo HEAD\n> git update-ref refs/foodir/bar/boz HEAD\n>\n> before the test to check and document the behavior for such refnames.\n\nYeah makes sense.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"271090","messageId":"xmqqsi5q1rd8.fsf@gitster.mtv.corp.google.com","threadId":"40474","inReplyTo":"vpqr3lb6oy7.fsf@grenoble-inp.fr","subject":"Re: [PATCH 1/9] ref-filter: implement %(if), %(then), and %(else) atoms","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-04T17:21:55Z","receivedAt":"2015-10-04T17:21:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n>> I found all the suggestions very good, except that the distinction\n>> between \"expands to\" and \"is printed\" bothers me a bit, as they want\n>> to mean exactly the same thing (imagine this whole thing were inside\n>> another %(if)...%(then)).\n>\n> True. Then let me try again:\n>\n> Implement %(if), %(then) and %(else) atoms. Used as\n> %(if)...%(then)...%(end) or %(if)...%(then)...%(else)...%(end). If the\n> format string between %(if) and %(then) expands to an empty string, or\n> to only whitespaces, then the whole %(if)...%(end) expands to the string\n> following %(then). Otherwise, it expands to the string following\n> %(else), if any.\n\nNice. Thanks.\n"},{"id":"271092","messageId":"vpqfv1q5xop.fsf@grenoble-inp.fr","threadId":"40474","inReplyTo":"CAOLa=ZSdM34_J+gb_=K4-9fgNsMfseAYxUYGm_ncWZstcW+SYw@mail.gmail.com","subject":"Re: [PATCH 3/9] ref-filter: add support for %(path) atom","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-10-04T17:51:50Z","receivedAt":"2015-10-04T17:51:50Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Karthik Nayak writes:\n\n> On Sat, Oct 3, 2015 at 3:32 PM, Matthieu Moy\n> wrote:\n>> Karthik Nayak writes:\n>>\n>>> This adds %(path) and %(path:short) atoms. The %(path) atom will print\n>>> the path of the given ref, while %(path:short) will only print the\n>>> subdirectory of the given ref.\n>>\n>> What does \"path\" mean in this context? How is it different from\n>> %(refname)?\n>>\n>> I found the answer below, but I could not guess from the doc and commit\n>> message. Actually, I'm not sure %(path) is the right name. If you want\n>> the \"file/directory\" analogy, then %(dirname) would be better.\n>>\n>\n> Noted will change.\n\nNote: I don't completely like %(dirname) either. I'm convinced it's\nbetter than %(path), but there may be a better option.\n\n>>> + } else if (match_atom_name(name, \"path\", &valp)) {\n>>> + const char *sp, *ep;\n>>> +\n>>> + if (ref->kind & FILTER_REFS_DETACHED_HEAD)\n>>> + continue;\n>>> +\n>>> + sp = strchr(ref->refname, '/');\n>>> + ep = strchr(++sp, '/');\n>>\n>> This assumes you have two / in the fullrefname. It is normally the case,\n>> but one can also create eg. refs/foo references. AFAIK, in this case sp\n>> will be NULL, and you're then calling strchr(++NULL) which may segfault.\n>\n> Not really,\n\nOops, indeed, for refs/foo, sp points to / and ep is NULL. It would\nsegfault for any ref without a /. Currently, the only such ref is HEAD\nand it is dealt with by the 'if' above, but that still sounds a bit\nfragile to me. Ifever we allow listing refs like FETCH_HEAD, I'm not\nsure we'll remember that and test %(path) on it.\n\nAdding something like\n\nif (!sp)\n\tcontinue;\n\nafter the first strchr would make me feel safer.\n\n> but you made me think of another possible issue.\n>\n> Assume refs/foo \"sp = strchr(ref->refname, '/');\" would set sp to point at\n> '/' and ++sp will now point at 'f'.\n>\n> The problem now is refs/foo is supposed to print just \"refs\" whereas it'll\n> print \"refs/foo\". and %(dirname:short) is supposed to print \"\" whereas it'll\n> print \"foo\". Will look into this :)\n\nI think it's worse than that because ep will be NULL, and then you call\n\n\tv->s = xstrndup(sp, ep - sp);\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"271105","messageId":"xmqq1tda1njq.fsf@gitster.mtv.corp.google.com","threadId":"40474","inReplyTo":"vpqfv1q5xop.fsf@grenoble-inp.fr","subject":"Re: [PATCH 3/9] ref-filter: add support for %(path) atom","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-04T18:44:25Z","receivedAt":"2015-10-04T18:44:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n>>>> This adds %(path) and %(path:short) atoms. The %(path) atom will print\n>>>> the path of the given ref, while %(path:short) will only print the\n>>>> subdirectory of the given ref.\n>>>\n>>> What does \"path\" mean in this context? How is it different from\n>>> %(refname)?\n>>>\n>>> I found the answer below, but I could not guess from the doc and commit\n>>> message. Actually, I'm not sure %(path) is the right name. If you want\n>>> the \"file/directory\" analogy, then %(dirname) would be better.\n>>>\n>>\n>> Noted will change.\n>\n> Note: I don't completely like %(dirname) either. I'm convinced it's\n> better than %(path), but there may be a better option.\n\nIs that a derived form of the refname, just like %(refname:short)\nthat is 'master' for a ref whose %(refname) is 'refs/heads/master'\nis a derived form of %(refname), and \":short\" is what tells the\nformatting machinery what kind of derivation is desired?\n\nIf so would %(refname:dir) & %(refname:base) be more in line with\nthe overall structure?\n"},{"id":"271109","messageId":"CAOLa=ZS2oWfjrx12q8nRxbquwFhtydqAFZ5zc4t2rMAk68RLYw@mail.gmail.com","threadId":"40474","inReplyTo":"vpqfv1q5xop.fsf@grenoble-inp.fr","subject":"Re: [PATCH 3/9] ref-filter: add support for %(path) atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-10-04T19:12:43Z","receivedAt":"2015-10-04T19:12:43Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Mon, Oct 5, 2015 at 12:14 AM, Junio C Hamano wrote:\n>\n> Is that a derived form of the refname, just like %(refname:short)\n> that is 'master' for a ref whose %(refname) is 'refs/heads/master'\n> is a derived form of %(refname), and \":short\" is what tells the\n> formatting machinery what kind of derivation is desired?\n>\n> If so would %(refname:dir) & %(refname:base) be more in line with\n> the overall structure?\n\nThis seems way better, I like these names more.\n\nOn Sun, Oct 4, 2015 at 11:21 PM, Matthieu Moy\n wrote:\n> Karthik Nayak writes:\n>\n>> On Sat, Oct 3, 2015 at 3:32 PM, Matthieu Moy\n>> wrote:\n>>> Karthik Nayak writes:\n>>>\n>>>> This adds %(path) and %(path:short) atoms. The %(path) atom will print\n>>>> the path of the given ref, while %(path:short) will only print the\n>>>> subdirectory of the given ref.\n>>>\n>>> What does \"path\" mean in this context? How is it different from\n>>> %(refname)?\n>>>\n>>> I found the answer below, but I could not guess from the doc and commit\n>>> message. Actually, I'm not sure %(path) is the right name. If you want\n>>> the \"file/directory\" analogy, then %(dirname) would be better.\n>>>\n>>\n>> Noted will change.\n>\n> Note: I don't completely like %(dirname) either. I'm convinced it's\n> better than %(path), but there may be a better option.\n>\n\nI like Junio's suggestion, stick with that?\n\n>>>> + } else if (match_atom_name(name, \"path\", &valp)) {\n>>>> + const char *sp, *ep;\n>>>> +\n>>>> + if (ref->kind & FILTER_REFS_DETACHED_HEAD)\n>>>> + continue;\n>>>> +\n>>>> + sp = strchr(ref->refname, '/');\n>>>> + ep = strchr(++sp, '/');\n>>>\n>>> This assumes you have two / in the fullrefname. It is normally the case,\n>>> but one can also create eg. refs/foo references. AFAIK, in this case sp\n>>> will be NULL, and you're then calling strchr(++NULL) which may segfault.\n>>\n>> Not really,\n>\n> Oops, indeed, for refs/foo, sp points to / and ep is NULL. It would\n> segfault for any ref without a /. Currently, the only such ref is HEAD\n> and it is dealt with by the 'if' above, but that still sounds a bit\n> fragile to me. Ifever we allow listing refs like FETCH_HEAD, I'm not\n> sure we'll remember that and test %(path) on it.\n>\n> Adding something like\n>\n> if (!sp)\n> continue;\n>\n> after the first strchr would make me feel safer.\n>\n\nGood point! I'll add that.\n\n>> but you made me think of another possible issue.\n>>\n>> Assume refs/foo \"sp = strchr(ref->refname, '/');\" would set sp to point at\n>> '/' and ++sp will now point at 'f'.\n>>\n>> The problem now is refs/foo is supposed to print just \"refs\" whereas it'll\n>> print \"refs/foo\". and %(dirname:short) is supposed to print \"\" whereas it'll\n>> print \"foo\". Will look into this :)\n>\n> I think it's worse than that because ep will be NULL, and then you call\n>\n> v->s = xstrndup(sp, ep - sp);\n>\n\nAh yes, weirdly though my test didn't yield a problem.\n\n~ git update-ref refs/foo master\n~ git for-each-ref --format=\"%(refname) %(color:red)%(dirname:short)\"\nrefs/foo foo\n~ git for-each-ref --format=\"%(refname) %(color:red)%(dirname)\"\nrefs/foo refs/foo\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"271121","messageId":"vpqbncd6coq.fsf@grenoble-inp.fr","threadId":"40474","inReplyTo":"xmqq1tda1njq.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/9] ref-filter: add support for %(path) atom","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-10-05T06:40:05Z","receivedAt":"2015-10-05T06:40:05Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano writes:\n\n> Matthieu Moy writes:\n>\n>>>>> This adds %(path) and %(path:short) atoms. The %(path) atom will print\n>>>>> the path of the given ref, while %(path:short) will only print the\n>>>>> subdirectory of the given ref.\n>>>>\n>>>> What does \"path\" mean in this context? How is it different from\n>>>> %(refname)?\n>>>>\n>>>> I found the answer below, but I could not guess from the doc and commit\n>>>> message. Actually, I'm not sure %(path) is the right name. If you want\n>>>> the \"file/directory\" analogy, then %(dirname) would be better.\n>>>>\n>>>\n>>> Noted will change.\n>>\n>> Note: I don't completely like %(dirname) either. I'm convinced it's\n>> better than %(path), but there may be a better option.\n>\n> Is that a derived form of the refname, just like %(refname:short)\n> that is 'master' for a ref whose %(refname) is 'refs/heads/master'\n> is a derived form of %(refname), and \":short\" is what tells the\n> formatting machinery what kind of derivation is desired?\n>\n> If so would %(refname:dir) & %(refname:base) be more in line with\n> the overall structure?\n\nYes, indeed much better. It's still about the refnames, so a specialized\nversion of %(refname) makes much more sense than a new atom.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"271124","messageId":"CAOLa=ZTMf9i=+9Rgi9oO==vr_JGtG3J9DvQWk18Zx-yd5_35XA@mail.gmail.com","threadId":"40474","inReplyTo":"vpqoagg9n2l.fsf@grenoble-inp.fr","subject":"Re: [PATCH 6/9] ref-filter: introduce format_ref_array_item()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-10-05T07:49:13Z","receivedAt":"2015-10-05T07:49:13Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Sat, Oct 3, 2015 at 5:32 PM, Matthieu Moy\n wrote:\n> Karthik Nayak writes:\n>\n>> Introduce format_ref_array_item() which will output the details of a\n>> given ref_array_item as per the given format and quote_style to the\n>> given strbuf.\n>\n> Why do you need it in this series and you could do without for tag?\n>\n> Going through PATCH 8/9, I guess there's something related to --column,\n> but tag also has --column so I'm puzzled.\n>\n\nThe problem is with colors which tag.c doesn't use by default.\nHere we need to print colors only if we're printing to a tty which supports\ncolors. which does not play well with the implementation of --column as\ndone in tag.c. Where, If I'm not wrong the --column option captures all\noutput, formats it and then prints it to stdout. Hence when using colors, we're\ntold that the printing isn't done to a tty which supports colors, hence we lose\nout on colors.\n\nI was trying something where we have a variable which knows when the column\noption is used and hence explicitly lets us use colors, but seemed like a mess.\nThis was a clean of going about it.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"271125","messageId":"CAOLa=ZRF6tVg6DSjXZb65pJz_D5_H5aix7JFx9pSW49iyFN+iQ@mail.gmail.com","threadId":"40474","inReplyTo":"vpqio6o9mu1.fsf@grenoble-inp.fr","subject":"Re: [PATCH 7/9] ref-filter: make %(upstream:track) prints \"[gone]\" for invalid upstreams","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-10-05T07:49:43Z","receivedAt":"2015-10-05T07:49:43Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Sat, Oct 3, 2015 at 5:38 PM, Matthieu Moy\n wrote:\n> Karthik Nayak writes:\n>\n>> diff --git a/ref-filter.c b/ref-filter.c\n>> index 099acd6..48b06e3 100644\n>> --- a/ref-filter.c\n>> +++ b/ref-filter.c\n>> @@ -1133,8 +1133,10 @@ static void populate_value(struct ref_array_item *ref)\n>> char buf[40];\n>>\n>> if (stat_tracking_info(branch, &num_ours,\n>> - &num_theirs, NULL))\n>> + &num_theirs, NULL)) {\n>> + v->s = xstrdup(\"[gone]\");\n>\n> I think just \"[gone]\" without the xstrdup is OK, and avoids leaking one\n> string.\n\nWill do.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"271131","messageId":"vpq1td9zolv.fsf@grenoble-inp.fr","threadId":"40474","inReplyTo":"CAOLa=ZTMf9i=+9Rgi9oO==vr_JGtG3J9DvQWk18Zx-yd5_35XA@mail.gmail.com","subject":"Re: [PATCH 6/9] ref-filter: introduce format_ref_array_item()","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-10-05T08:49:48Z","receivedAt":"2015-10-05T08:49:48Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Karthik Nayak writes:\n\n> The problem is with colors which tag.c doesn't use by default.\n> Here we need to print colors only if we're printing to a tty which supports\n> colors.\n\nAh, and tag --format '%(color:...)' does not do the tty autodetection.\n\n> which does not play well with the implementation of --column as done\n> in tag.c. Where, If I'm not wrong the --column option captures all\n> output, formats it and then prints it to stdout. Hence when using\n> colors, we're told that the printing isn't done to a tty which\n> supports colors, hence we lose out on colors.\n\nWhat I don't understand is how --column is different from --no-column\nwrt colors.\n\nIn any case, this should be explained better in comments.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"271148","messageId":"CAOLa=ZSk8-6nkfEd+Kz1srOJxPLj6+zLEU9DnLgW3rW1O6kZGg@mail.gmail.com","threadId":"40474","inReplyTo":"vpqvbao86pj.fsf@grenoble-inp.fr","subject":"Re: [PATCH 8/9] branch: use ref-filter printing APIs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-10-05T17:46:42Z","receivedAt":"2015-10-05T17:46:42Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Sat, Oct 3, 2015 at 6:11 PM, Matthieu Moy\n wrote:\n> Karthik Nayak writes:\n>\n>> - if (upstream_is_gone) {\n>> - if (show_upstream_ref)\n>> - strbuf_addf(stat, _(\"[%s: gone]\"), fancy.buf);\n>\n> The old string was translated, and you're replacing it with one which\n> isn't.\n>\n> I'm not a big fan of translation, so that change doesn't disturb me, but\n> people used to having \"git branch\" talk their native language here may\n> care.\n>\n> Be careful: translation should happen only in porcelain. Typicall,\n> \"for-each-ref\" should anyway continue saying \"gone\" in english whatever\n> the configuration is.\n>\n> See e.g. 7a76c28 (status: disable translation when --porcelain is used,\n> 2014-03-20) for an example of translation only for porcelain (that was\n> for status, but also for ahead/behind/gone).\n>\n\nI'm not how translation works, thanks for the hint, I'll look around.\n\n>> +static char *get_format(struct ref_filter *filter, int maxwidth, const char *remote_prefix)\n>\n> I'd call that \"build_format\", since \"get_...\" usually implies that the\n> value exists already and you're retrieving it.\n>\n\nMakes sense.\n\n>> +{\n>> + char *remote = NULL;\n>> + char *local = NULL;\n>> + char *final = NULL;\n>> +\n>> + if (filter->verbose) {\n>> + if (filter->verbose > 1)\n>> + local = xstrfmt(\"%%(if)%%(HEAD)%%(then)* %s%%(else) %%(end)%%(align:%d,left)%%(refname:short)%%(end)%s\"\n>> + \" %%(objectname:short,7) %%(if)%%(upstream)%%(then)[%s%%(upstream:short)%s] %%(end)\"\n>> + \"%%(if)%%(upstream:track)%%(then)%%(upstream:track) %%(end)%%(contents:subject)\",\n>> + branch_get_color(BRANCH_COLOR_CURRENT), maxwidth, branch_get_color(BRANCH_COLOR_RESET),\n>> + branch_get_color(BRANCH_COLOR_UPSTREAM), branch_get_color(BRANCH_COLOR_RESET));\n>\n> OK, that is a hell of a block of code ;-).\n>\n> You should factor the common portions out of these if/else. C allows you\n> to write several string litteral next to each other, so you can eg.\n>\n> #define STAR_IF_HEAD \"%%(if)%%(HEAD)%%(then)* %s%%(else) %%(end)\"\n> #define ALIGNED_REFNAME \"%%(align:%d,left)%%(refname:short)%%(end)\"\n>\n> and then\n>\n> STAR_IF_HEAD ALIGNED_REFNAME ...\n>\n> Actually, this is not a performance-cricical piece of code at all, so I\n> think it's even better to build an strbuf little by little using\n> repeated strbuf_addf calls. This way you can do things like\n>\n> strbuf_addf(fmt, \"%%(if)%%(HEAD)%%(then)* %s%%(else) %%(end)\",\n> branch_get_color(BRANCH_COLOR_CURRENT));\n> strbuf_addf(fmt, \"%%(align:%d,left)%%(refname:short)%%(end)\", maxwidth);\n>\n> which makes it much easier to pair the %s and the corresponding\n> branch_get_color() or the %d and maxwidth.\n>\n> But fundamentally, I think this function is doing the right thing. The\n> function is a bit complex (I think it will be much nicer to read when\n> better factored), but 1) it allows getting rid of a lot of specific code\n> and 2) it's a proof that --format is expressive enough.\n>\n\nI like the idea of using \"#define\" it might make things simpler.\n\nNot sure about using a strbuf cause I don't see how that could make things\nsimpler, we'll end up with something similar to what we have currently. Just\nthat instead of using variables like \"local\", \"remote\" and \"final\" we'd just\nmake multiple calls to strbuf_addf().\n\n>> @@ -58,11 +58,11 @@ test_expect_success 'branch -v' '\n>> '\n>>\n>> cat >expect <<\\EOF\n>> -b1 [origin/master: ahead 1, behind 1] d\n>> -b2 [origin/master: ahead 1, behind 1] d\n>> -b3 [origin/master: behind 1] b\n>> -b4 [origin/master: ahead 2] f\n>> -b5 [brokenbase: gone] g\n>> +b1 [origin/master] [ahead 1, behind 1] d\n>> +b2 [origin/master] [ahead 1, behind 1] d\n>> +b3 [origin/master] [behind 1] b\n>> +b4 [origin/master] [ahead 2] f\n>> +b5 [brokenbase] [gone] g\n>\n> This corresponds to this part of the commit message:\n>\n>> Since branch.c is being ported to use ref-filter APIs to print the\n>> required data, it is constricted to the constraints of printing as per\n>> ref-filter. Which means branch.c can only print as per the atoms\n>> available in ref-filter. Hence the \"-vv\" option of 'git branch' now\n>> prints the upstream and its tracking details separately as\n>> \"[] []\" instead of \"[: > info>]\".\n>>\n>> Make changes in /t/t6040-tracking-info.sh to reflect the change.\n>\n> nit: /t/t... -> t/t... (remove leading /)\n>\n> That is a behavior change, and I actually prefered the previous one.\n>\n> I actually don't understand why you can't get the old syntax: the []\n> around the refname come from the format string it get_format(), so you\n> could decide to change \"[%s%%(upstream:short)%s]\" to\n> \"[%s%%(upstream:short)%s: \". The brackets around the status are a bit\n> more tricky, since they were here before your series. But we could have\n> a %(upstream:track,nobracket) to display just the status without the\n> brackets around.\n>\n> Then, the code must deal with up-to-date branches for which no\n> \" :ahead/behind\" must be displayed. Probably one more if/then/else\n> nested in the first one.\n>\n> So, it seems feasible to me to keep the old behavior with the new\n> system, even though it's going to be a bit tricky.\n>\n\nWasn't that hard to do, I got this working, I was just reluctant on making\na new subvalue like \"%(upstream:track,nobracket)\" (no real reason, just didn't\nwant to ).\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"271150","messageId":"vpq7fn1qhp2.fsf@grenoble-inp.fr","threadId":"40474","inReplyTo":"CAOLa=ZSk8-6nkfEd+Kz1srOJxPLj6+zLEU9DnLgW3rW1O6kZGg@mail.gmail.com","subject":"Re: [PATCH 8/9] branch: use ref-filter printing APIs","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-10-05T18:43:53Z","receivedAt":"2015-10-05T18:43:53Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Karthik Nayak writes:\n\n> On Sat, Oct 3, 2015 at 6:11 PM, Matthieu Moy\n> wrote:\n>> Actually, this is not a performance-cricical piece of code at all, so I\n>> think it's even better to build an strbuf little by little using\n>> repeated strbuf_addf calls. This way you can do things like\n>>\n>> strbuf_addf(fmt, \"%%(if)%%(HEAD)%%(then)* %s%%(else) %%(end)\",\n>> branch_get_color(BRANCH_COLOR_CURRENT));\n>> strbuf_addf(fmt, \"%%(align:%d,left)%%(refname:short)%%(end)\", maxwidth);\n>>\n>> which makes it much easier to pair the %s and the corresponding\n>> branch_get_color() or the %d and maxwidth.\n>>\n>> But fundamentally, I think this function is doing the right thing. The\n>> function is a bit complex (I think it will be much nicer to read when\n>> better factored), but 1) it allows getting rid of a lot of specific code\n>> and 2) it's a proof that --format is expressive enough.\n>>\n>\n> I like the idea of using \"#define\" it might make things simpler.\n>\n> Not sure about using a strbuf cause I don't see how that could make things\n> simpler, we'll end up with something similar to what we have\n> currently.\n\nIt allows you to really break the way you build the string into several\nsmall steps, and use if/then locally on steps which require it.\n\nFor example, you currently have\n\n+ if (filter->verbose) {\n+ if (filter->verbose > 1)\n+ local = xstrfmt(\"%%(if)%%(HEAD)%%(then)* %s%%(else) %%(end)%%(align:%d,left)%%(refname:short)%%(end)%s\"\n+ \" %%(objectname:short,7) %%(if)%%(upstream)%%(then)[%s%%(upstream:short)%s] %%(end)\"\n+ \"%%(if)%%(upstream:track)%%(then)%%(upstream:track) %%(end)%%(contents:subject)\",\n+ branch_get_color(BRANCH_COLOR_CURRENT), maxwidth, branch_get_color(BRANCH_COLOR_RESET),\n+ branch_get_color(BRANCH_COLOR_UPSTREAM), branch_get_color(BRANCH_COLOR_RESET));\n+\n+ else\n+ local = xstrfmt(\"%%(if)%%(HEAD)%%(then)* %s%%(else) %%(end)%%(align:%d,left)%\"\n+ \"%(refname:short)%%(end)%s %%(objectname:short,7) %%(if)%%(upstream:track)%\"\n+ \"%(then)%%(upstream:track) %%(end)%%(contents:subject)\",\n+ branch_get_color(BRANCH_COLOR_CURRENT), maxwidth, branch_get_color(BRANCH_COLOR_RESET));\n+\n+ remote = xstrfmt(\" %s%%(align:%d,left)%s%%(refname:short)%%(end)%s%%(if)%%(symref)%%(then) -> %%(symref:short)\"\n+ \"%%(else) %%(objectname:short,7) %%(contents:subject)%%(end)\",\n+ branch_get_color(BRANCH_COLOR_REMOTE), maxwidth,\n+ remote_prefix, branch_get_color(BRANCH_COLOR_RESET));\n+ final = xstrfmt(\"%%(if:notequals=remotes)%%(path:short)%%(then)%s%%(else)%s%%(end)\", local, remote);\n+\n+ } else {\n+ local = xstrfmt(\"%%(if)%%(HEAD)%%(then)* %s%%(else) %%(end)%%(refname:short)%s\",\n\nThe first bits of local are identical in all branches of the two-level\nif/else. You could use something like\n\n\tstrbuf_addf(format, \"%%(if)%%(HEAD)%%(then)* %s%%(else) %%(end)\",\n\t\t branch_get_color(BRANCH_COLOR_CURRENT));\n\tif (filter->verbose) {\n\t\t...\n\t}\n\nto factor it out of the if/else. Similarly, the \"if (filter->verbose >\n1)\" could be much smaller, and probably doesn't require an else branch\n(just say \"if very verbose, then add XYZ at this point in the format\nstring\").\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"271234","messageId":"CAOLa=ZS5x=ksfnBt1kLp5bJJHmqeBztR7Zn7U5VKZN-56T-_5A@mail.gmail.com","threadId":"40474","inReplyTo":"vpq7fn1qhp2.fsf@grenoble-inp.fr","subject":"Re: [PATCH 8/9] branch: use ref-filter printing APIs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-10-06T15:30:27Z","receivedAt":"2015-10-06T15:30:27Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Oct 6, 2015 at 12:13 AM, Matthieu Moy\n wrote:\n> Karthik Nayak writes:\n>\n>> On Sat, Oct 3, 2015 at 6:11 PM, Matthieu Moy\n>> wrote:\n>>> Actually, this is not a performance-cricical piece of code at all, so I\n>>> think it's even better to build an strbuf little by little using\n>>> repeated strbuf_addf calls. This way you can do things like\n>>>\n>>> strbuf_addf(fmt, \"%%(if)%%(HEAD)%%(then)* %s%%(else) %%(end)\",\n>>> branch_get_color(BRANCH_COLOR_CURRENT));\n>>> strbuf_addf(fmt, \"%%(align:%d,left)%%(refname:short)%%(end)\", maxwidth);\n>>>\n>>> which makes it much easier to pair the %s and the corresponding\n>>> branch_get_color() or the %d and maxwidth.\n>>>\n>>> But fundamentally, I think this function is doing the right thing. The\n>>> function is a bit complex (I think it will be much nicer to read when\n>>> better factored), but 1) it allows getting rid of a lot of specific code\n>>> and 2) it's a proof that --format is expressive enough.\n>>>\n>>\n>> I like the idea of using \"#define\" it might make things simpler.\n>>\n>> Not sure about using a strbuf cause I don't see how that could make things\n>> simpler, we'll end up with something similar to what we have\n>> currently.\n>\n> It allows you to really break the way you build the string into several\n> small steps, and use if/then locally on steps which require it.\n>\n> For example, you currently have\n>\n> + if (filter->verbose) {\n> + if (filter->verbose > 1)\n> + local = xstrfmt(\"%%(if)%%(HEAD)%%(then)* %s%%(else) %%(end)%%(align:%d,left)%%(refname:short)%%(end)%s\"\n> + \" %%(objectname:short,7) %%(if)%%(upstream)%%(then)[%s%%(upstream:short)%s] %%(end)\"\n> + \"%%(if)%%(upstream:track)%%(then)%%(upstream:track) %%(end)%%(contents:subject)\",\n> + branch_get_color(BRANCH_COLOR_CURRENT), maxwidth, branch_get_color(BRANCH_COLOR_RESET),\n> + branch_get_color(BRANCH_COLOR_UPSTREAM), branch_get_color(BRANCH_COLOR_RESET));\n> +\n> + else\n> + local = xstrfmt(\"%%(if)%%(HEAD)%%(then)* %s%%(else) %%(end)%%(align:%d,left)%\"\n> + \"%(refname:short)%%(end)%s %%(objectname:short,7) %%(if)%%(upstream:track)%\"\n> + \"%(then)%%(upstream:track) %%(end)%%(contents:subject)\",\n> + branch_get_color(BRANCH_COLOR_CURRENT), maxwidth, branch_get_color(BRANCH_COLOR_RESET));\n> +\n> + remote = xstrfmt(\" %s%%(align:%d,left)%s%%(refname:short)%%(end)%s%%(if)%%(symref)%%(then) -> %%(symref:short)\"\n> + \"%%(else) %%(objectname:short,7) %%(contents:subject)%%(end)\",\n> + branch_get_color(BRANCH_COLOR_REMOTE), maxwidth,\n> + remote_prefix, branch_get_color(BRANCH_COLOR_RESET));\n> + final = xstrfmt(\"%%(if:notequals=remotes)%%(path:short)%%(then)%s%%(else)%s%%(end)\", local, remote);\n> +\n> + } else {\n> + local = xstrfmt(\"%%(if)%%(HEAD)%%(then)* %s%%(else) %%(end)%%(refname:short)%s\",\n>\n> The first bits of local are identical in all branches of the two-level\n> if/else. You could use something like\n>\n> strbuf_addf(format, \"%%(if)%%(HEAD)%%(then)* %s%%(else) %%(end)\",\n> branch_get_color(BRANCH_COLOR_CURRENT));\n> if (filter->verbose) {\n> ...\n> }\n>\n> to factor it out of the if/else. Similarly, the \"if (filter->verbose >\n> 1)\" could be much smaller, and probably doesn't require an else branch\n> (just say \"if very verbose, then add XYZ at this point in the format\n> string\").\n>\n\nIf you look closely, thats only for the local branches, the remotes\nhave `align` atom when\nprinting in verbose.\n\nSo like I said, I couldn't really make it quite simpler. Maybe a line\nor two, but using `#define`\nI could cook up this:\n\nchar *remote = NULL;\nchar *final = NULL;\n\nstruct strbuf local = STRBUF_INIT;\n\nstrbuf_addf(&local, \"%%(if)%%(HEAD)%%(then)* %s%%(else) %%(end)\",\n branch_get_color(BRANCH_COLOR_CURRENT));\n\nif (filter->verbose) {\n strbuf_addf(&local, \"%%(align:%d,left)%%(refname:short)%%(end)\",\n maxwidth);\n strbuf_addf(&local, \"%s\", branch_get_color(BRANCH_COLOR_RESET));\n strbuf_addf(&local, \" %%(objectname:short=7) \");\n\n if (filter->verbose > 1)\n strbuf_addf(&local,\n\"%%(if)%%(upstream)%%(then)[%s%%(upstream:short)%s%%(if)%%(upstream:track)\"\n \"%%(then): %%(upstream:track,nobracket)%%(end)]\n%%(end)%%(contents:subject)\",\n branch_get_color(BRANCH_COLOR_UPSTREAM),\nbranch_get_color(BRANCH_COLOR_RESET));\n else\n strbuf_addf(&local,\n\"%%(if)%%(upstream:track)%%(then)%%(upstream:track)\n%%(end)%%(contents:subject)\");\n\n remote = xstrfmt(\"\n%s%%(align:%d,left)%s%%(refname:short)%%(end)%s%%(if)%%(symref)%%(then)\n-> %%(symref:short)\"\n \"%%(else) %%(objectname:short=7)\n%%(contents:subject)%%(end)\",\n branch_get_color(BRANCH_COLOR_REMOTE), maxwidth,\n remote_prefix, branch_get_color(BRANCH_COLOR_RESET));\n} else {\n strbuf_addf(&local, \"%%(refname:short)%s\",\nbranch_get_color(BRANCH_COLOR_RESET));\n remote = xstrfmt(\"\n%s%s%%(refname:short)%s%%(if)%%(symref)%%(then) ->\n%%(symref:short)%%(end)\",\n branch_get_color(BRANCH_COLOR_REMOTE),\nremote_prefix, branch_get_color(BRANCH_COLOR_RESET));\n}\n\nfinal = xstrfmt(\"%%(if:notequals=remotes)%%(refname:base)%%(then)%s%%(else)%s%%(end)\",\nlocal.buf, remote);\n\nstrbuf_release(&local);\nfree(remote);\n\nreturn final;\n\nCouldn't see anything else I could break down.\n\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"271240","messageId":"vpqmvvvhl9m.fsf@grenoble-inp.fr","threadId":"40474","inReplyTo":"CAOLa=ZS5x=ksfnBt1kLp5bJJHmqeBztR7Zn7U5VKZN-56T-_5A@mail.gmail.com","subject":"Re: [PATCH 8/9] branch: use ref-filter printing APIs","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-10-06T19:03:49Z","receivedAt":"2015-10-06T19:03:49Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Karthik Nayak writes:\n\n> If you look closely, thats only for the local branches, the remotes\n> have `align` atom when\n> printing in verbose.\n\nYes, but that's already one thing factored out of the if, even if it's\njust for local.\n\nActually, I think you can also factor some parts out of the\n%(if:notequals=remotes). In 'local', you have an %(if) to display either\n\"* \" or \" \", and in remote you always start with \" \". Why not always\napply the %(if), and let it display \" \" if not displaying the current\nbranch? Similarly, the \"verbose\" part of remote branches seems like a\nparticular case of the one for local ones (remotes don't have tracking\nbranches, so the tracking info should expand to the empty string).\n\nTo go a bit further, you can pre-build a string or strbuf aligned_short\nwith value like \"%%(align:20,left)%%(refname:short)%%(end)\" and use it\nwhere needed (it's not a constant because you have to introduce maxwidth\ninto the string, so it's not a candidate for #define).\n\n> I could cook up this:\n\nYour mailer broke the formatting, so it looks terrible, but from what I\ncould parse, it's already much better than the previous one. It's not a\nmatter of size of the function, I very much prefer reading 10 lines of\nnice code than 4 lines like\n\n> + local = xstrfmt(\"%%(if)%%(HEAD)%%(then)* %s%%(else) %%(end)%%(align:%d,left)%%(refname:short)%%(end)%s\"\n> + \" %%(objectname:short,7) %%(if)%%(upstream)%%(then)[%s%%(upstream:short)%s] %%(end)\"\n> + \"%%(if)%%(upstream:track)%%(then)%%(upstream:track) %%(end)%%(contents:subject)\",\n> + branch_get_color(BRANCH_COLOR_CURRENT), maxwidth, branch_get_color(BRANCH_COLOR_RESET),\n> + branch_get_color(BRANCH_COLOR_UPSTREAM), branch_get_color(BRANCH_COLOR_RESET));\n\n;-).\n\nOne obvious issue with the initial version was this big hard-to-parse\nblock, but another one is that the code did not make it easy to\nunderstand what was changing depending on which branch of the if, and\ndepending on local/remote. It's getting much easier already.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"271241","messageId":"CAOLa=ZSr19+0OVoyACdN_YfSFeWugtS40WNu0GB9=uZFEu8N5A@mail.gmail.com","threadId":"40474","inReplyTo":"vpq1td9zolv.fsf@grenoble-inp.fr","subject":"Re: [PATCH 6/9] ref-filter: introduce format_ref_array_item()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-10-06T19:16:09Z","receivedAt":"2015-10-06T19:16:09Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Mon, Oct 5, 2015 at 2:19 PM, Matthieu Moy\n wrote:\n> Karthik Nayak writes:\n>> which does not play well with the implementation of --column as done\n>> in tag.c. Where, If I'm not wrong the --column option captures all\n>> output, formats it and then prints it to stdout. Hence when using\n>> colors, we're told that the printing isn't done to a tty which\n>> supports colors, hence we lose out on colors.\n>\n> What I don't understand is how --column is different from --no-column\n> wrt colors.\n>\n> In any case, this should be explained better in comments.\n\nWell, If we use column the way we do in tag.c then it'll replace the tty\nand color will not print color because it will assume the output tty doesn't\nsupport colors.\n\nI hope that's what you're asking\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"271244","messageId":"vpqlhbfg52l.fsf@grenoble-inp.fr","threadId":"40474","inReplyTo":"CAOLa=ZSr19+0OVoyACdN_YfSFeWugtS40WNu0GB9=uZFEu8N5A@mail.gmail.com","subject":"Re: [PATCH 6/9] ref-filter: introduce format_ref_array_item()","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-10-06T19:38:58Z","receivedAt":"2015-10-06T19:38:58Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Karthik Nayak writes:\n\n> On Mon, Oct 5, 2015 at 2:19 PM, Matthieu Moy\n> wrote:\n>> Karthik Nayak writes:\n>>> which does not play well with the implementation of --column as done\n>>> in tag.c. Where, If I'm not wrong the --column option captures all\n>>> output, formats it and then prints it to stdout. Hence when using\n>>> colors, we're told that the printing isn't done to a tty which\n>>> supports colors, hence we lose out on colors.\n>>\n>> What I don't understand is how --column is different from --no-column\n>> wrt colors.\n>>\n>> In any case, this should be explained better in comments.\n>\n> Well, If we use column the way we do in tag.c then it'll replace the tty\n> and color will not print color because it will assume the output tty doesn't\n> support colors.\n>\n> I hope that's what you're asking\n\nLooking a bit more closely at the code, I think I understand what's\ngoing on. branch.c used print_columns which does all the clever things\nwrt columns display. tag.c used run_column_filter which pipes the output\nto \"git column\" (hence, indeed, color detection does not work since\nwe're writting to a pipe).\n\nThe branch.c way seems good to me (why fork another process when we can\nformat the list ourselves ?). Probably tag.c could (should?) be modified\nto use it too. It should just be justified, like, replacing this commit\nmessage with:\n\n-- 8< --\nTo allow column display, we will need to first render the output in a\nstring list to allow print_columns() to compute the proper size of each\ncolumn before starting the actual output. Introduce the function\nformat_ref_array_item() that does the formatting of a ref_array_item to\nan strbuf.\n\nshow_ref_array_item() is kept as a convenience wrapper around it which\nobtains the strbuf and prints it the standard output.\n-- 8< --\n\nand adding a comment next to if (column_active(colopts)) { in patch\n8/9:\n\n\t/* format to a string_list to let print_columns() do its job */\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"271245","messageId":"CAOLa=ZRxjcmKQ+Jucwk8zSoeZzJyXQQCc=R89Cg8ci5nXzQwJw@mail.gmail.com","threadId":"40474","inReplyTo":"vpqlhbfg52l.fsf@grenoble-inp.fr","subject":"Re: [PATCH 6/9] ref-filter: introduce format_ref_array_item()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-10-06T19:50:35Z","receivedAt":"2015-10-06T19:50:35Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Wed, Oct 7, 2015 at 1:08 AM, Matthieu Moy\n wrote:\n> Karthik Nayak writes:\n>\n>> On Mon, Oct 5, 2015 at 2:19 PM, Matthieu Moy\n>> wrote:\n>>> Karthik Nayak writes:\n>>>> which does not play well with the implementation of --column as done\n>>>> in tag.c. Where, If I'm not wrong the --column option captures all\n>>>> output, formats it and then prints it to stdout. Hence when using\n>>>> colors, we're told that the printing isn't done to a tty which\n>>>> supports colors, hence we lose out on colors.\n>>>\n>>> What I don't understand is how --column is different from --no-column\n>>> wrt colors.\n>>>\n>>> In any case, this should be explained better in comments.\n>>\n>> Well, If we use column the way we do in tag.c then it'll replace the tty\n>> and color will not print color because it will assume the output tty doesn't\n>> support colors.\n>>\n>> I hope that's what you're asking\n>\n> Looking a bit more closely at the code, I think I understand what's\n> going on. branch.c used print_columns which does all the clever things\n> wrt columns display. tag.c used run_column_filter which pipes the output\n> to \"git column\" (hence, indeed, color detection does not work since\n> we're writting to a pipe).\n>\n> The branch.c way seems good to me (why fork another process when we can\n> format the list ourselves ?). Probably tag.c could (should?) be modified\n> to use it too. It should just be justified, like, replacing this commit\n> message with:\n>\n\nYes! this :)\n\n\n> -- 8< --\n> To allow column display, we will need to first render the output in a\n> string list to allow print_columns() to compute the proper size of each\n> column before starting the actual output. Introduce the function\n> format_ref_array_item() that does the formatting of a ref_array_item to\n> an strbuf.\n>\n> show_ref_array_item() is kept as a convenience wrapper around it which\n> obtains the strbuf and prints it the standard output.\n> -- 8< --\n>\n\nLooks Good.\n\n> and adding a comment next to if (column_active(colopts)) { in patch\n> 8/9:\n>\n> /* format to a string_list to let print_columns() do its job */\n>\n\nWill do this, thanks a bunch\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"271263","messageId":"CAOLa=ZS4xVfsQ+F+KGD8Gz9NHzvJtuVvW16RSLV08ZiEv8DhKw@mail.gmail.com","threadId":"40474","inReplyTo":"vpqmvvvhl9m.fsf@grenoble-inp.fr","subject":"Re: [PATCH 8/9] branch: use ref-filter printing APIs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-10-07T16:47:47Z","receivedAt":"2015-10-07T16:47:47Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Wed, Oct 7, 2015 at 12:33 AM, Matthieu Moy\n wrote:\n> Karthik Nayak writes:\n>\n>> If you look closely, thats only for the local branches, the remotes\n>> have `align` atom when\n>> printing in verbose.\n>\n> Yes, but that's already one thing factored out of the if, even if it's\n> just for local.\n>\n> Actually, I think you can also factor some parts out of the\n> %(if:notequals=remotes). In 'local', you have an %(if) to display either\n> \"* \" or \" \", and in remote you always start with \" \". Why not always\n> apply the %(if), and let it display \" \" if not displaying the current\n> branch? Similarly, the \"verbose\" part of remote branches seems like a\n> particular case of the one for local ones (remotes don't have tracking\n> branches, so the tracking info should expand to the empty string).\n\nWe could factor out the \" \" and the \"* \" printing. I'll do that\n\n>\n> To go a bit further, you can pre-build a string or strbuf aligned_short\n> with value like \"%%(align:20,left)%%(refname:short)%%(end)\" and use it\n> where needed (it's not a constant because you have to introduce maxwidth\n> into the string, so it's not a candidate for #define).\n>\n\nAgain, the remote has a remote prefix here, so we can't really factor\nit out much.\n\n>> I could cook up this:\n>\n> Your mailer broke the formatting, so it looks terrible, but from what I\n> could parse, it's already much better than the previous one. It's not a\n> matter of size of the function, I very much prefer reading 10 lines of\n> nice code than 4 lines like\n>\n\nI get what you're saying I'll see if I can factor out more.\n\n>> + local = xstrfmt(\"%%(if)%%(HEAD)%%(then)* %s%%(else) %%(end)%%(align:%d,left)%%(refname:short)%%(end)%s\"\n>> + \" %%(objectname:short,7) %%(if)%%(upstream)%%(then)[%s%%(upstream:short)%s] %%(end)\"\n>> + \"%%(if)%%(upstream:track)%%(then)%%(upstream:track) %%(end)%%(contents:subject)\",\n>> + branch_get_color(BRANCH_COLOR_CURRENT), maxwidth, branch_get_color(BRANCH_COLOR_RESET),\n>> + branch_get_color(BRANCH_COLOR_UPSTREAM), branch_get_color(BRANCH_COLOR_RESET));\n>\n> ;-).\n>\n> One obvious issue with the initial version was this big hard-to-parse\n> block, but another one is that the code did not make it easy to\n> understand what was changing depending on which branch of the if, and\n> depending on local/remote. It's getting much easier already.\n>\n\nYea, we can say that :)\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"271268","messageId":"vpqmvvu4l3o.fsf@grenoble-inp.fr","threadId":"40474","inReplyTo":"CAOLa=ZS4xVfsQ+F+KGD8Gz9NHzvJtuVvW16RSLV08ZiEv8DhKw@mail.gmail.com","subject":"Re: [PATCH 8/9] branch: use ref-filter printing APIs","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-10-07T17:58:03Z","receivedAt":"2015-10-07T17:58:03Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Karthik Nayak writes:\n\n> On Wed, Oct 7, 2015 at 12:33 AM, Matthieu Moy\n> wrote:\n>\n>> To go a bit further, you can pre-build a string or strbuf aligned_short\n>> with value like \"%%(align:20,left)%%(refname:short)%%(end)\" and use it\n>> where needed (it's not a constant because you have to introduce maxwidth\n>> into the string, so it's not a candidate for #define).\n>>\n>\n> Again, the remote has a remote prefix here, so we can't really factor\n> it out much.\n\nAh, OK, there's a %s containing the remote prefix in \"remote\" that is\nnot there in \"local\", and in the last version you sent it's already\nfactored for verbose > 1 and verbose == 1. Indeed, there's nothing more\nto gain here.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"271269","messageId":"CAOLa=ZQ8QpU_FTqK7Sn0xeygtyirsLOE7bG03iwcVoPUJFpptg@mail.gmail.com","threadId":"40474","inReplyTo":"vpqmvvu4l3o.fsf@grenoble-inp.fr","subject":"Re: [PATCH 8/9] branch: use ref-filter printing APIs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-10-07T18:02:27Z","receivedAt":"2015-10-07T18:02:27Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Wed, Oct 7, 2015 at 11:28 PM, Matthieu Moy\n wrote:\n> Karthik Nayak writes:\n>\n>> On Wed, Oct 7, 2015 at 12:33 AM, Matthieu Moy\n>> wrote:\n>>\n>>> To go a bit further, you can pre-build a string or strbuf aligned_short\n>>> with value like \"%%(align:20,left)%%(refname:short)%%(end)\" and use it\n>>> where needed (it's not a constant because you have to introduce maxwidth\n>>> into the string, so it's not a candidate for #define).\n>>>\n>>\n>> Again, the remote has a remote prefix here, so we can't really factor\n>> it out much.\n>\n> Ah, OK, there's a %s containing the remote prefix in \"remote\" that is\n> not there in \"local\", and in the last version you sent it's already\n> factored for verbose > 1 and verbose == 1. Indeed, there's nothing more\n> to gain here.\n\nI'll send version2 of the patch series to the list then :)\n\n-- \nRegards,\nKarthik Nayak\n"}]}