{"thread":{"id":"35356","subject":"[PATCH v4 0/6] Replacement for rr/for-each-ref-decoration","startedAt":"2013-11-18T17:39:07Z","lastAt":"2013-11-19T17:32:58Z","messageCount":12,"participants":["Ramkumar Ramachandra","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":6},"messages":[{"id":"230763","messageId":"1384796353-18701-1-git-send-email-artagnon@gmail.com","threadId":"35356","inReplyTo":null,"subject":"[PATCH v4 0/6] Replacement for rr/for-each-ref-decoration","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-11-18T17:39:07Z","receivedAt":"2013-11-18T17:39:07Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nThe major change since v3 is in [6/6]: color no more auto-resets after\neach token, but instead only to avoid leakage (thanks to Junio). Other\nminor changes are in accordance with Eric Sunshine's review.\n\nThanks.\n\nRamkumar Ramachandra (6):\n t6300 (for-each-ref): clearly demarcate setup\n t6300 (for-each-ref): don't hardcode SHA-1 hexes\n for-each-ref: introduce %(HEAD) asterisk marker\n for-each-ref: introduce %(upstream:track[short])\n for-each-ref: introduce %(color:...) for color\n for-each-ref: avoid color leakage\n\n Documentation/git-for-each-ref.txt | 14 +++++-\n builtin/for-each-ref.c | 93 +++++++++++++++++++++++++++++++++-----\n t/t6300-for-each-ref.sh | 69 ++++++++++++++++++++++------\n 3 files changed, 151 insertions(+), 25 deletions(-)\n\n-- \n1.8.5.rc0.5.g70ebc73.dirty\n"},{"id":"230764","messageId":"1384796353-18701-2-git-send-email-artagnon@gmail.com","threadId":"35356","inReplyTo":"1384796353-18701-1-git-send-email-artagnon@gmail.com","subject":"[PATCH v4 1/6] t6300 (for-each-ref): clearly demarcate setup","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-11-18T17:39:08Z","receivedAt":"2013-11-18T17:39:08Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Condense the two-step setup into one step, and give it an appropriate\nname.\n\nSigned-off-by: Ramkumar Ramachandra \n---\n t/t6300-for-each-ref.sh | 7 ++-----\n 1 file changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 752f5cb..64301e7 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -18,16 +18,13 @@ setdate_and_increment () {\n export GIT_COMMITTER_DATE GIT_AUTHOR_DATE\n }\n \n-test_expect_success 'Create sample commit with known timestamp' '\n+test_expect_success setup '\n \tsetdate_and_increment &&\n \techo \"Using $datestamp\" > one &&\n \tgit add one &&\n \tgit commit -m \"Initial\" &&\n \tsetdate_and_increment &&\n-\tgit tag -a -m \"Tagging at $datestamp\" testtag\n-'\n-\n-test_expect_success 'Create upstream config' '\n+\tgit tag -a -m \"Tagging at $datestamp\" testtag &&\n \tgit update-ref refs/remotes/origin/master master &&\n \tgit remote add origin nowhere &&\n \tgit config branch.master.remote origin &&\n-- \n1.8.5.rc0.5.g70ebc73.dirty\n"},{"id":"230765","messageId":"1384796353-18701-3-git-send-email-artagnon@gmail.com","threadId":"35356","inReplyTo":"1384796353-18701-1-git-send-email-artagnon@gmail.com","subject":"[PATCH v4 2/6] t6300 (for-each-ref): don't hardcode SHA-1 hexes","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-11-18T17:39:09Z","receivedAt":"2013-11-18T17:39:09Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Use rev-parse in its place, making it easier for future patches to\nmodify the test script.\n\nSigned-off-by: Ramkumar Ramachandra \n---\n t/t6300-for-each-ref.sh | 16 ++++++++--------\n 1 file changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 64301e7..675c2a2 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -49,8 +49,8 @@ test_atom head refname refs/heads/master\n test_atom head upstream refs/remotes/origin/master\n test_atom head objecttype commit\n test_atom head objectsize 171\n-test_atom head objectname 67a36f10722846e891fbada1ba48ed035de75581\n-test_atom head tree 0e51c00fcb93dffc755546f27593d511e1bdb46f\n+test_atom head objectname $(git rev-parse refs/heads/master)\n+test_atom head tree $(git rev-parse refs/heads/master^{tree})\n test_atom head parent ''\n test_atom head numparent 0\n test_atom head object ''\n@@ -82,11 +82,11 @@ test_atom tag refname refs/tags/testtag\n test_atom tag upstream ''\n test_atom tag objecttype tag\n test_atom tag objectsize 154\n-test_atom tag objectname 98b46b1d36e5b07909de1b3886224e3e81e87322\n+test_atom tag objectname $(git rev-parse refs/tags/testtag)\n test_atom tag tree ''\n test_atom tag parent ''\n test_atom tag numparent ''\n-test_atom tag object '67a36f10722846e891fbada1ba48ed035de75581'\n+test_atom tag object $(git rev-parse refs/tags/testtag^0)\n test_atom tag type 'commit'\n test_atom tag author ''\n test_atom tag authorname ''\n@@ -302,7 +302,7 @@ test_expect_success 'Check short upstream format' '\n '\n \n cat >expected <expected <<\\EOF\n-408fe76d02a785a006c2e9c669b7be5589ede96d refs/tags/master\n-90b5ebede4899eda64893bc2a4c8f1d6fb6dfc40 refs/tags/bogo\n+cat >expected < refs/tags/master\n+$(git rev-parse refs/tags/bogo) refs/tags/bogo\n EOF\n \n test_expect_success 'Verify sort with multiple keys' '\n-- \n1.8.5.rc0.5.g70ebc73.dirty\n"},{"id":"230766","messageId":"1384796353-18701-4-git-send-email-artagnon@gmail.com","threadId":"35356","inReplyTo":"1384796353-18701-1-git-send-email-artagnon@gmail.com","subject":"[PATCH v4 3/6] for-each-ref: introduce %(HEAD) asterisk marker","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-11-18T17:39:10Z","receivedAt":"2013-11-18T17:39:10Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"'git branch' shows which branch you are currently on with an '*', but\n'git for-each-ref' misses this feature. So, extend its format with\n%(HEAD) for the same effect.\n\nNow you can use the following format in for-each-ref:\n\n %(HEAD) %(refname:short)\n\nto display an asterisk next to the current ref.\n\nSigned-off-by: Ramkumar Ramachandra \n---\n Documentation/git-for-each-ref.txt | 4 ++++\n builtin/for-each-ref.c | 13 +++++++++++--\n t/t6300-for-each-ref.sh | 2 ++\n 3 files changed, 17 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex f2e08d1..8f87c9a 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -93,6 +93,10 @@ upstream::\n \tfrom the displayed ref. Respects `:short` in the same way as\n \t`refname` above.\n \n+HEAD::\n+\t'*' if HEAD matches current ref (the checked out branch), ' '\n+\totherwise.\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/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex 1d4083c..5f1842f 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -75,6 +75,7 @@ static struct {\n \t{ \"upstream\" },\n \t{ \"symref\" },\n \t{ \"flag\" },\n+\t{ \"HEAD\" },\n };\n \n /*\n@@ -675,8 +676,16 @@ static void populate_value(struct refinfo *ref)\n \t\t\t\tv->s = xstrdup(buf + 1);\n \t\t\t}\n \t\t\tcontinue;\n-\t\t}\n-\t\telse\n+\t\t} else if (!strcmp(name, \"HEAD\")) {\n+\t\t\tconst char *head;\n+\t\t\tunsigned char sha1[20];\n+\t\t\thead = resolve_ref_unsafe(\"HEAD\", sha1, 1, NULL);\n+\t\t\tif (!strcmp(ref->refname, head))\n+\t\t\t\tv->s = \"*\";\n+\t\t\telse\n+\t\t\t\tv->s = \" \";\n+\t\t\tcontinue;\n+\t\t} else\n \t\t\tcontinue;\n \n \t\tformatp = strchr(name, ':');\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 675c2a2..1d998f8 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -77,6 +77,7 @@ test_atom head contents:body ''\n test_atom head contents:signature ''\n test_atom head contents 'Initial\n '\n+test_atom head HEAD '*'\n \n test_atom tag refname refs/tags/testtag\n test_atom tag upstream ''\n@@ -110,6 +111,7 @@ test_atom tag contents:body ''\n test_atom tag contents:signature ''\n test_atom tag contents 'Tagging at 1151939927\n '\n+test_atom tag HEAD ' '\n \n test_expect_success 'Check invalid atoms names are errors' '\n \ttest_must_fail git for-each-ref --format=\"%(INVALID)\" refs/heads\n-- \n1.8.5.rc0.5.g70ebc73.dirty\n"},{"id":"230767","messageId":"1384796353-18701-5-git-send-email-artagnon@gmail.com","threadId":"35356","inReplyTo":"1384796353-18701-1-git-send-email-artagnon@gmail.com","subject":"[PATCH v4 4/6] for-each-ref: introduce %(upstream:track[short])","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-11-18T17:39:11Z","receivedAt":"2013-11-18T17:39:11Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Introduce %(upstream:track) to display \"[ahead M, behind N]\" and\n%(upstream:trackshort) to display \"=\", \">\", \"<\", or \"<>\"\nappropriately (inspired by contrib/completion/git-prompt.sh).\n\nNow you can use the following format in for-each-ref:\n\n %(refname:short)%(upstream:trackshort)\n\nto display refs with terse tracking information.\n\nNote that :track and :trackshort only work with \"upstream\", and error\nout when used with anything else.\n\nSigned-off-by: Ramkumar Ramachandra \n---\n Documentation/git-for-each-ref.txt | 6 +++++-\n builtin/for-each-ref.c | 40 +++++++++++++++++++++++++++++++++++---\n t/t6300-for-each-ref.sh | 27 +++++++++++++++++++++++++\n 3 files changed, 69 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex 8f87c9a..92e82fd 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -91,7 +91,11 @@ objectname::\n upstream::\n \tThe name of a local ref which can be considered ``upstream''\n \tfrom the displayed ref. Respects `:short` in the same way as\n-\t`refname` above.\n+\t`refname` above. Additionally respects `:track` to show\n+\t\"[ahead N, behind M]\" and `:trackshort` to show the terse\n+\tversion: \">\" (ahead), \"<\" (behind), \"<>\" (ahead and behind),\n+\tor \"=\" (in sync). Has no effect if the ref does not have\n+\ttracking information associated with it.\n \n HEAD::\n \t'*' if HEAD matches current ref (the checked out branch), ' '\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex 5f1842f..ed81407 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -641,6 +641,7 @@ static void populate_value(struct refinfo *ref)\n \t\tint deref = 0;\n \t\tconst char *refname;\n \t\tconst char *formatp;\n+\t\tstruct branch *branch;\n \n \t\tif (*name == '*') {\n \t\t\tderef = 1;\n@@ -652,7 +653,6 @@ static void populate_value(struct refinfo *ref)\n \t\telse if (!prefixcmp(name, \"symref\"))\n \t\t\trefname = ref->symref ? ref->symref : \"\";\n \t\telse if (!prefixcmp(name, \"upstream\")) {\n-\t\t\tstruct branch *branch;\n \t\t\t/* only local branches may have an upstream */\n \t\t\tif (prefixcmp(ref->refname, \"refs/heads/\"))\n \t\t\t\tcontinue;\n@@ -679,6 +679,7 @@ static void populate_value(struct refinfo *ref)\n \t\t} else if (!strcmp(name, \"HEAD\")) {\n \t\t\tconst char *head;\n \t\t\tunsigned char sha1[20];\n+\n \t\t\thead = resolve_ref_unsafe(\"HEAD\", sha1, 1, NULL);\n \t\t\tif (!strcmp(ref->refname, head))\n \t\t\t\tv->s = \"*\";\n@@ -689,13 +690,46 @@ static void populate_value(struct refinfo *ref)\n \t\t\tcontinue;\n \n \t\tformatp = strchr(name, ':');\n-\t\t/* look for \"short\" refname format */\n \t\tif (formatp) {\n+\t\t\tint num_ours, num_theirs;\n+\n \t\t\tformatp++;\n \t\t\tif (!strcmp(formatp, \"short\"))\n \t\t\t\trefname = shorten_unambiguous_ref(refname,\n \t\t\t\t\t\t warn_ambiguous_refs);\n-\t\t\telse\n+\t\t\telse if (!strcmp(formatp, \"track\") &&\n+\t\t\t\t!prefixcmp(name, \"upstream\")) {\n+\t\t\t\tchar buf[40];\n+\n+\t\t\t\tstat_tracking_info(branch, &num_ours, &num_theirs);\n+\t\t\t\tif (!num_ours && !num_theirs)\n+\t\t\t\t\tv->s = \"\";\n+\t\t\t\telse if (!num_ours) {\n+\t\t\t\t\tsprintf(buf, \"[behind %d]\", num_theirs);\n+\t\t\t\t\tv->s = xstrdup(buf);\n+\t\t\t\t} else if (!num_theirs) {\n+\t\t\t\t\tsprintf(buf, \"[ahead %d]\", num_ours);\n+\t\t\t\t\tv->s = xstrdup(buf);\n+\t\t\t\t} else {\n+\t\t\t\t\tsprintf(buf, \"[ahead %d, behind %d]\",\n+\t\t\t\t\t\tnum_ours, num_theirs);\n+\t\t\t\t\tv->s = xstrdup(buf);\n+\t\t\t\t}\n+\t\t\t\tcontinue;\n+\t\t\t} else if (!strcmp(formatp, \"trackshort\") &&\n+\t\t\t\t!prefixcmp(name, \"upstream\")) {\n+\n+\t\t\t\tstat_tracking_info(branch, &num_ours, &num_theirs);\n+\t\t\t\tif (!num_ours && !num_theirs)\n+\t\t\t\t\tv->s = \"=\";\n+\t\t\t\telse if (!num_ours)\n+\t\t\t\t\tv->s = \"<\";\n+\t\t\t\telse if (!num_theirs)\n+\t\t\t\t\tv->s = \">\";\n+\t\t\t\telse\n+\t\t\t\t\tv->s = \"<>\";\n+\t\t\t\tcontinue;\n+\t\t\t} else\n \t\t\t\tdie(\"unknown %.*s format %s\",\n \t\t\t\t (int)(formatp - name), name, formatp);\n \t\t}\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 1d998f8..d88d7ac 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -303,6 +303,33 @@ test_expect_success 'Check short upstream format' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'setup for upstream:track[short]' '\n+\ttest_commit two\n+'\n+\n+cat >expected <actual &&\n+\ttest_cmp expected actual\n+'\n+\n+cat >expected <\n+EOF\n+\n+test_expect_success 'Check upstream:trackshort format' '\n+\tgit for-each-ref --format=\"%(upstream:trackshort)\" refs/heads >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'Check that :track[short] cannot be used with other atoms' '\n+\ttest_must_fail git for-each-ref --format=\"%(refname:track)\" 2>/dev/null &&\n+\ttest_must_fail git for-each-ref --format=\"%(refname:trackshort)\" 2>/dev/null\n+'\n+\n cat >expected <\n---\n Documentation/git-for-each-ref.txt | 4 ++++\n builtin/for-each-ref.c | 11 +++++++++--\n t/t6300-for-each-ref.sh | 17 +++++++++++++++++\n 3 files changed, 30 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex 92e82fd..94f5c46 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -101,6 +101,10 @@ HEAD::\n \t'*' if HEAD matches current ref (the checked out branch), ' '\n \totherwise.\n \n+color::\n+\tChange output color. Followed by `:`, where names\n+\tare described in `color.branch.*`.\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/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex ed81407..2ff4e54 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -9,6 +9,7 @@\n #include \"quote.h\"\n #include \"parse-options.h\"\n #include \"remote.h\"\n+#include \"color.h\"\n \n /* Quoting styles */\n #define QUOTE_NONE 0\n@@ -76,6 +77,7 @@ static struct {\n \t{ \"symref\" },\n \t{ \"flag\" },\n \t{ \"HEAD\" },\n+\t{ \"color\" },\n };\n \n /*\n@@ -662,8 +664,13 @@ static void populate_value(struct refinfo *ref)\n \t\t\t !branch->merge[0]->dst)\n \t\t\t\tcontinue;\n \t\t\trefname = branch->merge[0]->dst;\n-\t\t}\n-\t\telse if (!strcmp(name, \"flag\")) {\n+\t\t} else if (!prefixcmp(name, \"color:\")) {\n+\t\t\tchar color[COLOR_MAXLEN] = \"\";\n+\n+\t\t\tcolor_parse(name + 6, \"--format\", color);\n+\t\t\tv->s = xstrdup(color);\n+\t\t\tcontinue;\n+\t\t} else if (!strcmp(name, \"flag\")) {\n \t\t\tchar buf[256], *cp = buf;\n \t\t\tif (ref->flag & REF_ISSYMREF)\n \t\t\t\tcp = copy_advance(cp, \",symref\");\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex d88d7ac..69e3155 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -343,6 +343,23 @@ test_expect_success 'Check for invalid refname format' '\n \ttest_must_fail git for-each-ref --format=\"%(refname:INVALID)\"\n '\n \n+get_color ()\n+{\n+\tgit config --get-color no.such.slot \"$1\"\n+}\n+\n+cat >expected <actual &&\n+\ttest_cmp expected actual\n+'\n+\n cat >expected <<\\EOF\n heads/master\n tags/master\n-- \n1.8.5.rc0.5.g70ebc73.dirty\n"},{"id":"230769","messageId":"1384796353-18701-7-git-send-email-artagnon@gmail.com","threadId":"35356","inReplyTo":"1384796353-18701-1-git-send-email-artagnon@gmail.com","subject":"[PATCH v4 6/6] for-each-ref: avoid color leakage","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-11-18T17:39:13Z","receivedAt":"2013-11-18T17:39:13Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"To make sure that an invocation like the following doesn't leak color,\n\n $ git for-each-ref --format='%(subject)%(color:green)'\n\nauto-reset at the end of the format string when the last color token\nseen in the format string isn't a color-reset.\n\nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/for-each-ref.c | 29 +++++++++++++++++++++++++----\n t/t6300-for-each-ref.sh | 2 +-\n 2 files changed, 26 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex 2ff4e54..04e35ba 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -23,6 +23,7 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;\n struct atom_value {\n \tconst char *s;\n \tunsigned long ul; /* used for sorting when not FIELD_STR */\n+\tint color : 2; /* 1 indicates color, 2 indicates color-reset */\n };\n \n struct ref_sort {\n@@ -668,6 +669,10 @@ static void populate_value(struct refinfo *ref)\n \t\t\tchar color[COLOR_MAXLEN] = \"\";\n \n \t\t\tcolor_parse(name + 6, \"--format\", color);\n+\t\t\tif (!strcmp(name + 6, \"reset\"))\n+\t\t\t\tv->color = 2;\n+\t\t\telse\n+\t\t\t\tv->color = 1;\n \t\t\tv->s = xstrdup(color);\n \t\t\tcontinue;\n \t\t} else if (!strcmp(name, \"flag\")) {\n@@ -914,11 +919,9 @@ static void sort_refs(struct ref_sort *sort, struct refinfo **refs, int num_refs\n \tqsort(refs, num_refs, sizeof(struct refinfo *), compare_refs);\n }\n \n-static void print_value(struct refinfo *ref, int atom, int quote_style)\n+static void print_value(struct atom_value *v, int quote_style)\n {\n-\tstruct atom_value *v;\n \tstruct strbuf sb = STRBUF_INIT;\n-\tget_value(ref, atom, &v);\n \tswitch (quote_style) {\n \tcase QUOTE_NONE:\n \t\tfputs(v->s, stdout);\n@@ -983,17 +986,35 @@ static void emit(const char *cp, const char *ep)\n static void show_ref(struct refinfo *info, const char *format, int quote_style)\n {\n \tconst char *cp, *sp, *ep;\n+\tstruct atom_value *atomv, resetv;\n+\tint reset_color = 0;\n+\tchar color[COLOR_MAXLEN] = \"\";\n \n+\tcolor_parse(\"reset\", \"--format\", color);\n+\tresetv.s = color;\n \tfor (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {\n \t\tep = strchr(sp, ')');\n \t\tif (cp < sp)\n \t\t\temit(cp, sp);\n-\t\tprint_value(info, parse_atom(sp + 2, ep), quote_style);\n+\t\tget_value(info, parse_atom(sp + 2, ep), &atomv);\n+\t\tprint_value(atomv, quote_style);\n+\n+\t\t/*\n+\t\t * reset_color is used to avoid color leakage; it\n+\t\t * should be set when the last color atom is not a\n+\t\t * color-reset.\n+\t\t */\n+\t\tif (atomv->color == 1)\n+\t\t\treset_color = 1;\n+\t\telse if (atomv->color == 2)\n+\t\t\treset_color = 0;\n \t}\n \tif (*cp) {\n \t\tsp = cp + strlen(cp);\n \t\temit(cp, sp);\n \t}\n+\tif (reset_color)\n+\t\tprint_value(&resetv, quote_style);\n \tputchar('\\n');\n }\n \ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 69e3155..46866ba 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -356,7 +356,7 @@ $(git rev-parse --short refs/tags/two) $(get_color green)two$(get_color reset)\n EOF\n \n test_expect_success 'Check %(color:...) ' '\n-\tgit for-each-ref --format=\"%(objectname:short) %(color:green)%(refname:short)%(color:reset)\" >actual &&\n+\tgit for-each-ref --format=\"%(objectname:short) %(color:green)%(refname:short)\" >actual &&\n \ttest_cmp expected actual\n '\n \n-- \n1.8.5.rc0.5.g70ebc73.dirty\n"},{"id":"230784","messageId":"xmqqwqk59xyz.fsf@gitster.dls.corp.google.com","threadId":"35356","inReplyTo":"1384796353-18701-7-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH v4 6/6] for-each-ref: avoid color leakage","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-11-18T21:55:16Z","receivedAt":"2013-11-18T21:55:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> To make sure that an invocation like the following doesn't leak color,\n>\n> $ git for-each-ref --format='%(subject)%(color:green)'\n>\n> auto-reset at the end of the format string when the last color token\n> seen in the format string isn't a color-reset.\n>\n> Signed-off-by: Ramkumar Ramachandra \n> ---\n> builtin/for-each-ref.c | 29 +++++++++++++++++++++++++----\n> t/t6300-for-each-ref.sh | 2 +-\n> 2 files changed, 26 insertions(+), 5 deletions(-)\n>\n> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\n> index 2ff4e54..04e35ba 100644\n> --- a/builtin/for-each-ref.c\n> +++ b/builtin/for-each-ref.c\n> @@ -23,6 +23,7 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;\n> struct atom_value {\n> \tconst char *s;\n> \tunsigned long ul; /* used for sorting when not FIELD_STR */\n> +\tint color : 2; /* 1 indicates color, 2 indicates color-reset */\n> };\n\nHmph. It looks wasteful to have this information in atom_value.\n\nIsn't a new single bit in \"struct refinfo\" all you need to keep\ntrack of, to see the last %(color:something) you ever saw is for a\ncolor that is not reset?\n"},{"id":"230788","messageId":"CALkWK0mnYXBVW-agV_v6=mPxA=MoJAfHQaPKarwKU=x2SE+tnQ@mail.gmail.com","threadId":"35356","inReplyTo":"xmqqwqk59xyz.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4 6/6] for-each-ref: avoid color leakage","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-11-19T04:37:33Z","receivedAt":"2013-11-19T04:37:33Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n>> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\n>> index 2ff4e54..04e35ba 100644\n>> --- a/builtin/for-each-ref.c\n>> +++ b/builtin/for-each-ref.c\n>> @@ -23,6 +23,7 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;\n>> struct atom_value {\n>> const char *s;\n>> unsigned long ul; /* used for sorting when not FIELD_STR */\n>> + int color : 2; /* 1 indicates color, 2 indicates color-reset */\n>> };\n>\n> Hmph. It looks wasteful to have this information in atom_value.\n\nI wanted to avoid an ugly global. On the other end of the spectrum,\nmodifying the various functions to take an extra reset_color_leakage\nparameter seems much too intrusive. Do you have any suggestions?\n\n> Isn't a new single bit in \"struct refinfo\" all you need to keep\n> track of, to see the last %(color:something) you ever saw is for a\n> color that is not reset?\n\nNo; because I can only look at one atom and set v->color, at a time.\n"},{"id":"230792","messageId":"xmqqiovo9x2r.fsf@gitster.dls.corp.google.com","threadId":"35356","inReplyTo":"CALkWK0mnYXBVW-agV_v6=mPxA=MoJAfHQaPKarwKU=x2SE+tnQ@mail.gmail.com","subject":"Re: [PATCH v4 6/6] for-each-ref: avoid color leakage","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-11-19T16:26:52Z","receivedAt":"2013-11-19T16:26:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> Junio C Hamano wrote:\n>\n>> Isn't a new single bit in \"struct refinfo\" all you need to keep\n>> track of, to see the last %(color:something) you ever saw is for a\n>> color that is not reset?\n>\n> No; because I can only look at one atom and set v->color, at a time.\n\nThat is probably because the patch is trying to collect a wrong kind\nof information, I think. If the approach is to check if each atom is\na color atom, parse_atom() may be the right place, but that is not\nnecessary.\n\nThe only thing you need to know is if you need to emit a \"reset\"\nthat the user did not explicitly ask for at the end, and that is a\nproperty of the format string, which is constant across refs you are\niterating over. You do not even need to know which one of the atom\nis of the \"color\" kind.\n\nMy knee-jerk \"adding it to struct refinfo\" is not correct, either,\nbecause what we want to know, i.e. \"do we need an extra reset?\" is\nnot a property for each ref. It is similar to \"what is the set of\natoms the format string is using?\" and \"do we need to peel tags in\norder to show all information asked by the format string?\"\n(i.e. used_atom[] and need_tagged, respectively).\n\nUnlike need_tagged which asks \"is there any *refname that asks us to\npeel tags?\", however, \"is the _last_ color:anything in the format\nstring not 'reset'?\" depends on the order of appearance of atoms in\nthe format string, so this needs to be done in a loop that scans the\nformat string from left to right once at the very beginning, and we\nhave a perfect place to do so in verify_format().\n\nSo perhaps like this one on top?\n\n builtin/for-each-ref.c | 45 +++++++++++++++++++++------------------------\n 1 file changed, 21 insertions(+), 24 deletions(-)\n\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex 04e35ba..5ff51d1 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -23,7 +23,6 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;\n struct atom_value {\n \tconst char *s;\n \tunsigned long ul; /* used for sorting when not FIELD_STR */\n-\tint color : 2; /* 1 indicates color, 2 indicates color-reset */\n };\n \n struct ref_sort {\n@@ -94,6 +93,7 @@ static struct {\n static const char **used_atom;\n static cmp_type *used_atom_type;\n static int used_atom_cnt, sort_atom_limit, need_tagged, need_symref;\n+static int need_color_reset_at_eol;\n \n /*\n * Used to parse format string and sort specifiers\n@@ -180,13 +180,21 @@ static const char *find_next(const char *cp)\n static int verify_format(const char *format)\n {\n \tconst char *cp, *sp;\n+\tstatic const char color_reset[] = \"color:reset\";\n+\n+\tneed_color_reset_at_eol = 0;\n \tfor (cp = format; *cp && (sp = find_next(cp)); ) {\n \t\tconst char *ep = strchr(sp, ')');\n+\t\tint at;\n+\n \t\tif (!ep)\n \t\t\treturn error(\"malformed format string %s\", sp);\n \t\t/* sp points at \"%(\" and ep points at the closing \")\" */\n-\t\tparse_atom(sp + 2, ep);\n+\t\tat = parse_atom(sp + 2, ep);\n \t\tcp = ep + 1;\n+\n+\t\tif (!memcmp(used_atom[at], \"color:\", 6))\n+\t\t\tneed_color_reset_at_eol = !!strcmp(used_atom[at], color_reset);\n \t}\n \treturn 0;\n }\n@@ -644,7 +652,7 @@ static void populate_value(struct refinfo *ref)\n \t\tint deref = 0;\n \t\tconst char *refname;\n \t\tconst char *formatp;\n-\t\tstruct branch *branch;\n+\t\tstruct branch *branch = NULL;\n \n \t\tif (*name == '*') {\n \t\t\tderef = 1;\n@@ -669,10 +677,6 @@ static void populate_value(struct refinfo *ref)\n \t\t\tchar color[COLOR_MAXLEN] = \"\";\n \n \t\t\tcolor_parse(name + 6, \"--format\", color);\n-\t\t\tif (!strcmp(name + 6, \"reset\"))\n-\t\t\t\tv->color = 2;\n-\t\t\telse\n-\t\t\t\tv->color = 1;\n \t\t\tv->s = xstrdup(color);\n \t\t\tcontinue;\n \t\t} else if (!strcmp(name, \"flag\")) {\n@@ -730,7 +734,7 @@ static void populate_value(struct refinfo *ref)\n \t\t\t\tcontinue;\n \t\t\t} else if (!strcmp(formatp, \"trackshort\") &&\n \t\t\t\t!prefixcmp(name, \"upstream\")) {\n-\n+\t\t\t\tassert(branch);\n \t\t\t\tstat_tracking_info(branch, &num_ours, &num_theirs);\n \t\t\t\tif (!num_ours && !num_theirs)\n \t\t\t\t\tv->s = \"=\";\n@@ -986,35 +990,28 @@ static void emit(const char *cp, const char *ep)\n static void show_ref(struct refinfo *info, const char *format, int quote_style)\n {\n \tconst char *cp, *sp, *ep;\n-\tstruct atom_value *atomv, resetv;\n-\tint reset_color = 0;\n-\tchar color[COLOR_MAXLEN] = \"\";\n \n-\tcolor_parse(\"reset\", \"--format\", color);\n-\tresetv.s = color;\n \tfor (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) {\n+\t\tstruct atom_value *atomv;\n+\n \t\tep = strchr(sp, ')');\n \t\tif (cp < sp)\n \t\t\temit(cp, sp);\n \t\tget_value(info, parse_atom(sp + 2, ep), &atomv);\n \t\tprint_value(atomv, quote_style);\n-\n-\t\t/*\n-\t\t * reset_color is used to avoid color leakage; it\n-\t\t * should be set when the last color atom is not a\n-\t\t * color-reset.\n-\t\t */\n-\t\tif (atomv->color == 1)\n-\t\t\treset_color = 1;\n-\t\telse if (atomv->color == 2)\n-\t\t\treset_color = 0;\n \t}\n \tif (*cp) {\n \t\tsp = cp + strlen(cp);\n \t\temit(cp, sp);\n \t}\n-\tif (reset_color)\n+\tif (need_color_reset_at_eol) {\n+\t\tstruct atom_value resetv;\n+\t\tchar color[COLOR_MAXLEN] = \"\";\n+\n+\t\tcolor_parse(\"reset\", \"--format\", color);\n+\t\tresetv.s = color;\n \t\tprint_value(&resetv, quote_style);\n+\t}\n \tputchar('\\n');\n }\n \n"},{"id":"230797","messageId":"xmqqwqk48fn0.fsf@gitster.dls.corp.google.com","threadId":"35356","inReplyTo":"CALkWK0mnYXBVW-agV_v6=mPxA=MoJAfHQaPKarwKU=x2SE+tnQ@mail.gmail.com","subject":"Re: [PATCH v4 6/6] for-each-ref: avoid color leakage","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-11-19T17:28:51Z","receivedAt":"2013-11-19T17:28:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> Junio C Hamano wrote:\n>>> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\n>>> index 2ff4e54..04e35ba 100644\n>>> --- a/builtin/for-each-ref.c\n>>> +++ b/builtin/for-each-ref.c\n>>> @@ -23,6 +23,7 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;\n>>> struct atom_value {\n>>> const char *s;\n>>> unsigned long ul; /* used for sorting when not FIELD_STR */\n>>> + int color : 2; /* 1 indicates color, 2 indicates color-reset */\n>>> };\n>>\n>> Hmph. It looks wasteful to have this information in atom_value.\n>\n> I wanted to avoid an ugly global. On the other end of the spectrum,\n> modifying the various functions to take an extra reset_color_leakage\n> parameter seems much too intrusive. Do you have any suggestions?\n\nWe already represent information that is for the format string as\nexisting globals. It means that, if we ever want to make the program\naccept and use more than one format string, we can't. We need one\nset of them for each such format string before we can use more than\none.\n\nIf you want to solve that problem, complaining by using a subjective\nword \"ugly\" does not help us much. The right approach to the\nsolution would be to first think what each global really means and\ndecide which ones are per-format properties. Then turn them into a\nproper abstraction by defining a structure to hold the currently\nconsidered format string and these various \"per format string\"\nproperties.\n\nOnce you do that, you can optionally make the code pass that single\nstructure around, and that will remove the global, but I think that\nstep can wait until we actually find a need for it.\n"},{"id":"230798","messageId":"CALkWK0nKGp4XdJX1Dds-Y+4o2eRXp8XH3qpqU8MNfTG+UdwACA@mail.gmail.com","threadId":"35356","inReplyTo":"xmqqiovo9x2r.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4 6/6] for-each-ref: avoid color leakage","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-11-19T17:32:58Z","receivedAt":"2013-11-19T17:32:58Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> My knee-jerk \"adding it to struct refinfo\" is not correct, either,\n> because what we want to know, i.e. \"do we need an extra reset?\" is\n> not a property for each ref. It is similar to \"what is the set of\n> atoms the format string is using?\" and \"do we need to peel tags in\n> order to show all information asked by the format string?\"\n> (i.e. used_atom[] and need_tagged, respectively).\n\nIt's mostly cruft carried over from my reset-color-after-each-token\nimplementation. My severe distaste for global variables prevented me\nfrom looking for the simpler solution.\n\n> Unlike need_tagged which asks \"is there any *refname that asks us to\n> peel tags?\", however, \"is the _last_ color:anything in the format\n> string not 'reset'?\" depends on the order of appearance of atoms in\n> the format string, so this needs to be done in a loop that scans the\n> format string from left to right once at the very beginning, and we\n> have a perfect place to do so in verify_format().\n\nI should be shot for my laziness and lack of ingenuity. Yeah,\nverify_format() is a much better place to put the logic than\npopulate_value().\n\n> So perhaps like this one on top?\n>\n> builtin/for-each-ref.c | 45 +++++++++++++++++++++------------------------\n> 1 file changed, 21 insertions(+), 24 deletions(-)\n>\n> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\n> index 04e35ba..5ff51d1 100644\n> --- a/builtin/for-each-ref.c\n> +++ b/builtin/for-each-ref.c\n\nThanks for taking the time to do this. And apologies for the laziness.\n"}]}