{"thread":{"id":"39952","subject":"[RFC/PATCH] Port branch.c to use ref-filter APIs","startedAt":"2015-07-28T06:55:34Z","lastAt":"2015-08-19T15:52:50Z","messageCount":88,"participants":["Karthik Nayak","Jacob Keller","Christian Couder","Matthieu Moy","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"266890","messageId":"CAOLa=ZT3_DMJWFN62cbF19uxYBFsE69dGaFR=af1HPKsQ42otg@mail.gmail.com","threadId":"39952","inReplyTo":null,"subject":"[RFC/PATCH] Port branch.c to use ref-filter APIs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-28T06:55:34Z","receivedAt":"2015-07-28T06:55:34Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"This is part of my GSoC project to unify git tag -l, git branch -l,\ngit for-each-ref. This patch series is continued from: Gmane (v6)\nhttp://article.gmane.org/gmane.comp.version-control.git/274726\n\nThis is a RFC version and I'm sending to ensure that I'm on the right path.\n\nThis version also doesn't use the printing options provided by\nbranch.c. I wanted to discuss how exactly to go about that, because in\nbranch.c, we might need to change the --format based on attributes\nsuch as abbrev, verbose and so on. But ref-filter expects us to verify\nthe format before filtering.\n\nAny tips or suggestions are welcome, thanks all :)\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"266891","messageId":"1438066594-5620-1-git-send-email-Karthik.188@gmail.com","threadId":"39952","inReplyTo":"CAOLa=ZT3_DMJWFN62cbF19uxYBFsE69dGaFR=af1HPKsQ42otg@mail.gmail.com","subject":"[RFC/PATCH 01/11] ref-filter: add \"%(objectname:size=X)\" option","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-28T06:56:26Z","receivedAt":"2015-07-28T06:56:26Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"From: Karthik Nayak \n\nAdd support for %(objectname:size=X) where X is a number.\nThis will print the first X characters of an objectname.\nThe minimum value for X is 5. Hence any value lesser than\n5 will default to 5 characters.\n\nMentored-by: Christian Couder \nMentored-by: Matthieu Moy \nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 0a34924..4c5e3f8 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -196,6 +196,8 @@ 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+\tconst char *p;\n+\n \tif (!strcmp(name, \"objectname\")) {\n \t\tchar *s = xmalloc(41);\n \t\tstrcpy(s, sha1_to_hex(sha1));\n@@ -206,6 +208,13 @@ static int grab_objectname(const char *name, const unsigned char *sha1,\n \t\tv->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));\n \t\treturn 1;\n \t}\n+\tif (skip_prefix(name, \"objectname:size=\", &p)) {\n+\t\tunsigned int size = atoi(p);\n+\t\tif (size < 5)\n+\t\t\tsize = 5;\n+\t\tv->s = xstrdup(find_unique_abbrev(sha1, size));\n+\t\treturn 1;\n+\t}\n \treturn 0;\n }\n \n-- \n2.4.6\n"},{"id":"266892","messageId":"1438066594-5620-2-git-send-email-Karthik.188@gmail.com","threadId":"39952","inReplyTo":"1438066594-5620-1-git-send-email-Karthik.188@gmail.com","subject":"[RFC/PATCH 02/11] ref-filter: add 'colornext' atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-28T06:56:27Z","receivedAt":"2015-07-28T06:56:27Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The 'colornext' atom allows us to color only the succeeding atom with\na particular color. This resets any previous color preferences set. It\nis not compatible with `padright`. This is required for printing\nformats like `git branch -vv` where the upstream is colored in blue.\n\nMentored-by: Christian Couder \nMentored-by: Matthieu Moy \nSigned-off-by: Karthik Nayak \n---\n Documentation/git-for-each-ref.txt | 5 +++++\n ref-filter.c | 20 +++++++++++++++++++-\n ref-filter.h | 4 +++-\n t/t6302-for-each-ref-filter.sh | 16 ++++++++++++++++\n 4 files changed, 43 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex 2b60aee..9dc02aa 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -133,6 +133,11 @@ padright::\n \tpadding. If the `value` is greater than the atom length, then\n \tno padding is performed.\n \n+colornext::\n+\tChange output color only for the next atom. Followed by\n+\t`<:colorname>`. Not compatible with `padright` and resets any\n+\tprevious `color`, if set.\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 4c5e3f8..3ab4ab9 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -57,6 +57,7 @@ static struct {\n \t{ \"HEAD\" },\n \t{ \"color\" },\n \t{ \"padright\" },\n+\t{ \"colornext\" },\n };\n \n /*\n@@ -712,6 +713,15 @@ static void populate_value(struct ref_array_item *ref)\n \t\t\tv->modifier_atom = 1;\n \t\t\tv->pad_to_right = 1;\n \t\t\tcontinue;\n+\t\t} else if (starts_with(name, \"colornext:\")) {\n+\t\t\tchar color[COLOR_MAXLEN] = \"\";\n+\n+\t\t\tif (color_parse(name + 10, color) < 0)\n+\t\t\t\tdie(_(\"unable to parse format\"));\n+\t\t\tv->s = xstrdup(color);\n+\t\t\tv->modifier_atom = 1;\n+\t\t\tv->color_next = 1;\n+\t\t\tcontinue;\n \t\t} else\n \t\t\tcontinue;\n \n@@ -1256,7 +1266,13 @@ void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)\n static void apply_formatting_state(struct ref_formatting_state *state,\n \t\t\t\t struct atom_value *v, struct strbuf *value)\n {\n-\tif (state->pad_to_right) {\n+\tif (state->color_next && state->pad_to_right)\n+\t\tdie(_(\"cannot use `colornext` and `padright` together\"));\n+\tif (state->color_next) {\n+\t\tstrbuf_addf(value, \"%s%s%s\", state->color_next, v->s, GIT_COLOR_RESET);\n+\t\treturn;\n+\t}\n+\telse if (state->pad_to_right) {\n \t\tif (!is_utf8(v->s))\n \t\t\tstrbuf_addf(value, \"%-*s\", state->pad_to_right, v->s);\n \t\telse {\n@@ -1346,6 +1362,8 @@ static void emit(const char *cp, const char *ep)\n static void store_formatting_state(struct ref_formatting_state *state,\n \t\t\t\t struct atom_value *atomv)\n {\n+\tif (atomv->color_next)\n+\t\tstate->color_next = atomv->s;\n \tif (atomv->pad_to_right)\n \t\tstate->pad_to_right = atomv->ul;\n }\ndiff --git a/ref-filter.h b/ref-filter.h\nindex fcf469e..8c82fd1 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -21,12 +21,14 @@ struct atom_value {\n \tconst char *s;\n \tunsigned long ul; /* used for sorting when not FIELD_STR */\n \tunsigned int modifier_atom : 1, /* atoms which act as modifiers for the next atom */\n-\t\tpad_to_right : 1;\n+\t\tpad_to_right : 1,\n+\t\tcolor_next : 1;\n };\n \n struct ref_formatting_state {\n \tint quote_style;\n \tunsigned int pad_to_right;\n+\tconst char *color_next;\n };\n \n struct ref_sorting {\ndiff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\nindex 68688a9..6aad069 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -133,4 +133,20 @@ test_expect_success 'reverse version sort' '\n \ttest_cmp expect actual\n '\n \n+get_color ()\n+{\n+\tgit config --get-color no.such.slot \"$1\"\n+}\n+\n+cat >expect <actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.4.6\n"},{"id":"266893","messageId":"1438066594-5620-3-git-send-email-Karthik.188@gmail.com","threadId":"39952","inReplyTo":"1438066594-5620-1-git-send-email-Karthik.188@gmail.com","subject":"[RFC/PATCH 03/11] ref-filter: add option to filter only branches","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-28T06:56:28Z","receivedAt":"2015-07-28T06:56:28Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"From: Karthik Nayak \n\nAdd an option in 'filter_refs()' to use 'for_each_branch_ref()'\nand filter refs. This type checking is done by adding a\n'FILTER_REFS_BRANCHES' in 'ref-filter.h'.\n\nAdd an option in 'ref_filter_handler()' to filter different\ntypes of branches by calling 'filter_branch_kind()' which\nchecks for the type of branch needed.\n\nMentored-by: Christian Couder \nMentored-by: Matthieu Moy \nSigned-off-by: Karthik Nayak \n---\n ref-filter.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++\n ref-filter.h | 10 ++++++++--\n 2 files changed, 55 insertions(+), 2 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 3ab4ab9..3f40144 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1054,6 +1054,46 @@ static const unsigned char *match_points_at(struct sha1_array *points_at,\n \treturn NULL;\n }\n \n+/*\n+ * Checks if a given refname is a branch and returns the kind of\n+ * branch it is. If not a branch, 0 is returned.\n+ */\n+static int filter_branch_kind(struct ref_filter *filter, const char *refname)\n+{\n+\tint kind, i;\n+\n+\tstatic struct {\n+\t\tint kind;\n+\t\tconst char *prefix;\n+\t} ref_kind[] = {\n+\t\t{ REF_LOCAL_BRANCH, \"refs/heads/\" },\n+\t\t{ REF_REMOTE_BRANCH, \"refs/remotes/\" },\n+\t};\n+\n+\t/* If no kind is specified, no need to filter */\n+\tif (!filter->branch_kind)\n+\t\treturn REF_NO_BRANCH_FILTERING;\n+\n+\tfor (i = 0; i < ARRAY_SIZE(ref_kind); i++) {\n+\t\tif (starts_with(refname, ref_kind[i].prefix)) {\n+\t\t\tkind = ref_kind[i].kind;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\n+\tif (ARRAY_SIZE(ref_kind) <= i) {\n+\t\tif (!strcmp(refname, \"HEAD\"))\n+\t\t\tkind = REF_DETACHED_HEAD;\n+\t\telse\n+\t\t\treturn 0;\n+\t}\n+\n+\tif ((filter->branch_kind & kind) == 0)\n+\t\treturn 0;\n+\n+\treturn kind;\n+}\n+\n /* Allocate space for a new ref_array_item and copy the objectname and flag to it */\n static struct ref_array_item *new_ref_array_item(const char *refname,\n \t\t\t\t\t\t const unsigned char *objectname,\n@@ -1079,6 +1119,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n \tstruct ref_filter *filter = ref_cbdata->filter;\n \tstruct ref_array_item *ref;\n \tstruct commit *commit = NULL;\n+\tunsigned int kind;\n \n \tif (flag & REF_BAD_NAME) {\n \t\t warning(\"ignoring ref with broken name %s\", refname);\n@@ -1090,6 +1131,9 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n \t\treturn 0;\n \t}\n \n+\tif (!(kind = filter_branch_kind(filter, refname)))\n+\t\treturn 0;\n+\n \tif (!filter_pattern_match(filter, refname))\n \t\treturn 0;\n \n@@ -1118,6 +1162,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n \t */\n \tref = new_ref_array_item(refname, oid->hash, flag);\n \tref->commit = commit;\n+\tref->kind = kind;\n \n \tREALLOC_ARRAY(ref_cbdata->array->items, ref_cbdata->array->nr + 1);\n \tref_cbdata->array->items[ref_cbdata->array->nr++] = ref;\n@@ -1208,6 +1253,8 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int\n \t\tret = for_each_ref(ref_filter_handler, &ref_cbdata);\n \telse if (type & FILTER_REFS_TAGS)\n \t\tret = for_each_tag_ref_fullpath(ref_filter_handler, &ref_cbdata);\n+\telse if (type & FILTER_REFS_BRANCHES)\n+\t\tret = for_each_rawref(ref_filter_handler, &ref_cbdata);\n \telse if (type)\n \t\tdie(\"filter_refs: invalid type\");\n \ndiff --git a/ref-filter.h b/ref-filter.h\nindex 8c82fd1..a021b04 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -16,6 +16,12 @@\n #define FILTER_REFS_INCLUDE_BROKEN 0x1\n #define FILTER_REFS_ALL 0x2\n #define FILTER_REFS_TAGS 0x4\n+#define FILTER_REFS_BRANCHES 0x8\n+\n+#define REF_DETACHED_HEAD 0x01\n+#define REF_LOCAL_BRANCH 0x02\n+#define REF_REMOTE_BRANCH 0x04\n+#define REF_NO_BRANCH_FILTERING 0x08\n \n struct atom_value {\n \tconst char *s;\n@@ -40,7 +46,7 @@ struct ref_sorting {\n \n struct ref_array_item {\n \tunsigned char objectname[20];\n-\tint flag;\n+\tint flag, kind;\n \tconst char *symref;\n \tstruct commit *commit;\n \tstruct atom_value *value;\n@@ -66,7 +72,7 @@ struct ref_filter {\n \n \tunsigned int with_commit_tag_algo : 1,\n \t\tmatch_as_path : 1;\n-\tunsigned int lines;\n+\tunsigned int lines, branch_kind;\n };\n \n struct ref_filter_cbdata {\n-- \n2.4.6\n"},{"id":"266895","messageId":"1438066594-5620-4-git-send-email-Karthik.188@gmail.com","threadId":"39952","inReplyTo":"1438066594-5620-1-git-send-email-Karthik.188@gmail.com","subject":"[RFC/PATCH 04/11] ref-filter: add 'ifexists' atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-28T06:56:29Z","receivedAt":"2015-07-28T06:56:29Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The 'ifexists' atom allows us to print a required format if the\npreceeding atom has a value. If the preceeding atom has no value then\nthe format given is not printed. e.g. to print \"[]\" we can\nnow use the format \"%(ifexists:[%s])%(refname)\".\n\nAdd documentation and test for the same.\n\nMentored-by: Christian Couder \nMentored-by: Matthieu Moy \nSigned-off-by: Karthik Nayak \n---\n Documentation/git-for-each-ref.txt | 8 ++++++++\n ref-filter.c | 37 ++++++++++++++++++++++++++++++++++---\n ref-filter.h | 5 +++--\n t/t6302-for-each-ref-filter.sh | 21 +++++++++++++++++++++\n 4 files changed, 66 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex 9dc02aa..4424020 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -138,6 +138,14 @@ colornext::\n \t`<:colorname>`. Not compatible with `padright` and resets any\n \tprevious `color`, if set.\n \n+ifexists::\n+\tPrint required string only if the next atom specified in the\n+\t'--format' option exists.\n+\te.g. --format=\"%(ifexists:[%s])%(symref)\" prints the symref\n+\tlike \"[]\" only if the ref has a symref. This was\n+\tincorporated to simulate the output of 'git branch -vv', where\n+\twe need to display the upstream branch in square brackets.\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 3f40144..ff5a16b 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -58,6 +58,7 @@ static struct {\n \t{ \"color\" },\n \t{ \"padright\" },\n \t{ \"colornext\" },\n+\t{ \"ifexists\" },\n };\n \n /*\n@@ -722,6 +723,13 @@ static void populate_value(struct ref_array_item *ref)\n \t\t\tv->modifier_atom = 1;\n \t\t\tv->color_next = 1;\n \t\t\tcontinue;\n+\t\t} else if (starts_with(name, \"ifexists:\")) {\n+\t\t\tskip_prefix(name, \"ifexists:\", &v->s);\n+\t\t\tif (!*v->s)\n+\t\t\t\tdie(_(\"no string given with 'ifexists:'\"));\n+\t\t\tv->modifier_atom = 1;\n+\t\t\tv->ifexists = 1;\n+\t\t\tcontinue;\n \t\t} else\n \t\t\tcontinue;\n \n@@ -1315,11 +1323,32 @@ static void apply_formatting_state(struct ref_formatting_state *state,\n {\n \tif (state->color_next && state->pad_to_right)\n \t\tdie(_(\"cannot use `colornext` and `padright` together\"));\n-\tif (state->color_next) {\n+\tif (state->pad_to_right && state->ifexists)\n+\t\tdie(_(\"cannot use 'align' and 'ifexists' together\"));\n+\tif (state->color_next && !state->ifexists) {\n \t\tstrbuf_addf(value, \"%s%s%s\", state->color_next, v->s, GIT_COLOR_RESET);\n \t\treturn;\n-\t}\n-\telse if (state->pad_to_right) {\n+\t} else if (state->ifexists) {\n+\t\tconst char *sp = state->ifexists;\n+\n+\t\twhile (*sp) {\n+\t\t\tif (*sp != '%') {\n+\t\t\t\tstrbuf_addch(value, *sp++);\n+\t\t\t\tcontinue;\n+\t\t\t} else if (sp[1] == '%') {\n+\t\t\t\tstrbuf_addch(value, *sp++);\n+\t\t\t\tcontinue;\n+\t\t\t} else if (sp[1] == 's') {\n+\t\t\t\tif (state->color_next)\n+\t\t\t\t\tstrbuf_addf(value, \"%s%s%s\", state->color_next, v->s, GIT_COLOR_RESET);\n+\t\t\t\telse\n+\t\t\t\t\tstrbuf_addstr(value, v->s);\n+\t\t\t\tsp += 2;\n+\t\t\t}\n+\t\t}\n+\n+\t\treturn;\n+\t} else if (state->pad_to_right) {\n \t\tif (!is_utf8(v->s))\n \t\t\tstrbuf_addf(value, \"%-*s\", state->pad_to_right, v->s);\n \t\telse {\n@@ -1413,6 +1442,8 @@ static void store_formatting_state(struct ref_formatting_state *state,\n \t\tstate->color_next = atomv->s;\n \tif (atomv->pad_to_right)\n \t\tstate->pad_to_right = atomv->ul;\n+\tif (atomv->ifexists)\n+\t\tstate->ifexists = atomv->s;\n }\n \n static void reset_formatting_state(struct ref_formatting_state *state)\ndiff --git a/ref-filter.h b/ref-filter.h\nindex a021b04..7d1871d 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -28,13 +28,14 @@ struct atom_value {\n \tunsigned long ul; /* used for sorting when not FIELD_STR */\n \tunsigned int modifier_atom : 1, /* atoms which act as modifiers for the next atom */\n \t\tpad_to_right : 1,\n-\t\tcolor_next : 1;\n+\t\tcolor_next : 1,\n+\t\tifexists : 1;\n };\n \n struct ref_formatting_state {\n \tint quote_style;\n \tunsigned int pad_to_right;\n-\tconst char *color_next;\n+\tconst char *color_next, *ifexists;\n };\n \n struct ref_sorting {\ndiff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\nindex 6aad069..29ed97b 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -149,4 +149,25 @@ test_expect_success 'check `colornext` format option' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'check `ifexists` format option' '\n+\tcat >expect <<-\\EOF &&\n+\t[foo1.10]\n+\t[foo1.3]\n+\t[foo1.6]\n+\tEOF\n+\tgit for-each-ref --format=\"%(ifexists:[%s])%(refname:short)\" | grep \"foo\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+cat >expect <actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.4.6\n"},{"id":"266894","messageId":"1438066594-5620-5-git-send-email-Karthik.188@gmail.com","threadId":"39952","inReplyTo":"1438066594-5620-1-git-send-email-Karthik.188@gmail.com","subject":"[RFC/PATCH 05/11] branch: fix width computation","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-28T06:56:30Z","receivedAt":"2015-07-28T06:56:30Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"From: Karthik Nayak \n\nRemove unnecessary variables from ref_list and ref_item\nwhich were used for width computation. Make other changes\naccordingly. This patch is a precursor for porting branch.c\nto use ref-filter APIs.\n\nBased-on-patch-by: Jeff King \nMentored-by: Christian Couder \nMentored-by: Matthieu Moy \nSigned-off-by: Karthik Nayak \n---\n builtin/branch.c | 61 +++++++++++++++++++++++++++++---------------------------\n 1 file changed, 32 insertions(+), 29 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 4fc8beb..b058b74 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -282,14 +282,14 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n struct ref_item {\n \tchar *name;\n \tchar *dest;\n-\tunsigned int kind, width;\n+\tunsigned int kind;\n \tstruct commit *commit;\n \tint ignore;\n };\n \n struct ref_list {\n \tstruct rev_info revs;\n-\tint index, alloc, maxwidth, verbose, abbrev;\n+\tint index, alloc, verbose, abbrev;\n \tstruct ref_item *list;\n \tstruct commit_list *with_commit;\n \tint kinds;\n@@ -386,15 +386,8 @@ static int append_ref(const char *refname, const struct object_id *oid, int flag\n \tnewitem->name = xstrdup(refname);\n \tnewitem->kind = kind;\n \tnewitem->commit = commit;\n-\tnewitem->width = utf8_strwidth(refname);\n \tnewitem->dest = resolve_symref(orig_refname, prefix);\n \tnewitem->ignore = 0;\n-\t/* adjust for \"remotes/\" */\n-\tif (newitem->kind == REF_REMOTE_BRANCH &&\n-\t ref_list->kinds != REF_REMOTE_BRANCH)\n-\t\tnewitem->width += 8;\n-\tif (newitem->width > ref_list->maxwidth)\n-\t\tref_list->maxwidth = newitem->width;\n \n \treturn 0;\n }\n@@ -505,11 +498,12 @@ static void add_verbose_info(struct strbuf *out, struct ref_item *item,\n }\n \n static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,\n-\t\t\t int abbrev, int current, char *prefix)\n+\t\t\t int abbrev, int current, const char *remote_prefix)\n {\n \tchar c;\n \tint color;\n \tstruct strbuf out = STRBUF_INIT, name = STRBUF_INIT;\n+\tconst char *prefix = \"\";\n \n \tif (item->ignore)\n \t\treturn;\n@@ -520,6 +514,7 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,\n \t\tbreak;\n \tcase REF_REMOTE_BRANCH:\n \t\tcolor = BRANCH_COLOR_REMOTE;\n+\t\tprefix = remote_prefix;\n \t\tbreak;\n \tdefault:\n \t\tcolor = BRANCH_COLOR_PLAIN;\n@@ -557,16 +552,21 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,\n \tstrbuf_release(&out);\n }\n \n-static int calc_maxwidth(struct ref_list *refs)\n+static int calc_maxwidth(struct ref_list *refs, int remote_bonus)\n {\n-\tint i, w = 0;\n+\tint i, max = 0;\n \tfor (i = 0; i < refs->index; i++) {\n+\t\tstruct ref_item *it = &refs->list[i];\n+\t\tint w = utf8_strwidth(it->name);\n+\n \t\tif (refs->list[i].ignore)\n \t\t\tcontinue;\n-\t\tif (refs->list[i].width > w)\n-\t\t\tw = refs->list[i].width;\n+\t\tif (it->kind == REF_REMOTE_BRANCH)\n+\t\t\tw += remote_bonus;\n+\t\tif (w > max)\n+\t\t\tmax = w;\n \t}\n-\treturn w;\n+\treturn max;\n }\n \n static char *get_head_description(void)\n@@ -600,21 +600,18 @@ static char *get_head_description(void)\n \treturn strbuf_detach(&desc, NULL);\n }\n \n-static void show_detached(struct ref_list *ref_list)\n+static void show_detached(struct ref_list *ref_list, int maxwidth)\n {\n \tstruct commit *head_commit = lookup_commit_reference_gently(head_sha1, 1);\n \n \tif (head_commit && is_descendant_of(head_commit, ref_list->with_commit)) {\n \t\tstruct ref_item item;\n \t\titem.name = get_head_description();\n-\t\titem.width = utf8_strwidth(item.name);\n \t\titem.kind = REF_LOCAL_BRANCH;\n \t\titem.dest = NULL;\n \t\titem.commit = head_commit;\n \t\titem.ignore = 0;\n-\t\tif (item.width > ref_list->maxwidth)\n-\t\t\tref_list->maxwidth = item.width;\n-\t\tprint_ref_item(&item, ref_list->maxwidth, ref_list->verbose, ref_list->abbrev, 1, \"\");\n+\t\tprint_ref_item(&item, maxwidth, ref_list->verbose, ref_list->abbrev, 1, \"\");\n \t\tfree(item.name);\n \t}\n }\n@@ -624,6 +621,16 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru\n \tint i;\n \tstruct append_ref_cb cb;\n \tstruct ref_list ref_list;\n+\tint maxwidth = 0;\n+\tconst char *remote_prefix = \"\";\n+\n+\t/*\n+\t * If we are listing more than just remote branches,\n+\t * then remote branches will have a \"remotes/\" prefix.\n+\t * We need to account for this in the width.\n+\t */\n+\tif (kinds != REF_REMOTE_BRANCH)\n+\t\tremote_prefix = \"remotes/\";\n \n \tmemset(&ref_list, 0, sizeof(ref_list));\n \tref_list.kinds = kinds;\n@@ -667,26 +674,22 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru\n \t\t\tclear_commit_marks(item->commit, ALL_REV_FLAGS);\n \t\t}\n \t\tclear_commit_marks(filter, ALL_REV_FLAGS);\n-\n-\t\tif (verbose)\n-\t\t\tref_list.maxwidth = calc_maxwidth(&ref_list);\n \t}\n+\tif (verbose)\n+\t\tmaxwidth = calc_maxwidth(&ref_list, strlen(remote_prefix));\n \n \tqsort(ref_list.list, ref_list.index, sizeof(struct ref_item), ref_cmp);\n \n \tdetached = (detached && (kinds & REF_LOCAL_BRANCH));\n \tif (detached && match_patterns(pattern, \"HEAD\"))\n-\t\tshow_detached(&ref_list);\n+\t\tshow_detached(&ref_list, maxwidth);\n \n \tfor (i = 0; i < ref_list.index; i++) {\n \t\tint current = !detached &&\n \t\t\t(ref_list.list[i].kind == REF_LOCAL_BRANCH) &&\n \t\t\t!strcmp(ref_list.list[i].name, head);\n-\t\tchar *prefix = (kinds != REF_REMOTE_BRANCH &&\n-\t\t\t\tref_list.list[i].kind == REF_REMOTE_BRANCH)\n-\t\t\t\t? \"remotes/\" : \"\";\n-\t\tprint_ref_item(&ref_list.list[i], ref_list.maxwidth, verbose,\n-\t\t\t abbrev, current, prefix);\n+\t\tprint_ref_item(&ref_list.list[i], maxwidth, verbose,\n+\t\t\t abbrev, current, remote_prefix);\n \t}\n \n \tfree_ref_list(&ref_list);\n-- \n2.4.6\n"},{"id":"266897","messageId":"1438066594-5620-6-git-send-email-Karthik.188@gmail.com","threadId":"39952","inReplyTo":"1438066594-5620-1-git-send-email-Karthik.188@gmail.com","subject":"[RFC/PATCH 06/11] branch: roll show_detached HEAD into regular ref_list","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-28T06:56:31Z","receivedAt":"2015-07-28T06:56:31Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Remove show_detached() and make detached HEAD to be rolled into\nregular ref_list by adding REF_DETACHED_HEAD as a kind of branch and\nsupporting the same in append_ref().\n\nBump get_head_description() to the top.\n\nBased-on-patch-by: Jeff King \nMentored-by: Christian Couder \nMentored-by: Matthieu Moy \nSigned-off-by: Karthik Nayak \n---\n builtin/branch.c | 122 ++++++++++++++++++++++++++++---------------------------\n 1 file changed, 63 insertions(+), 59 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex b058b74..f3d83d0 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -30,6 +30,7 @@ static const char * const builtin_branch_usage[] = {\n \n #define REF_LOCAL_BRANCH 0x01\n #define REF_REMOTE_BRANCH 0x02\n+#define REF_DETACHED_HEAD 0x04\n \n static const char *head;\n static unsigned char head_sha1[20];\n@@ -352,8 +353,12 @@ static int append_ref(const char *refname, const struct object_id *oid, int flag\n \t\t\tbreak;\n \t\t}\n \t}\n-\tif (ARRAY_SIZE(ref_kind) <= i)\n-\t\treturn 0;\n+\tif (ARRAY_SIZE(ref_kind) <= i) {\n+\t\tif (!strcmp(refname, \"HEAD\"))\n+\t\t\tkind = REF_DETACHED_HEAD;\n+\t\telse\n+\t\t\treturn 0;\n+\t}\n \n \t/* Don't add types the caller doesn't want */\n \tif ((kind & ref_list->kinds) == 0)\n@@ -497,6 +502,37 @@ static void add_verbose_info(struct strbuf *out, struct ref_item *item,\n \tstrbuf_release(&subject);\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 static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,\n \t\t\t int abbrev, int current, const char *remote_prefix)\n {\n@@ -504,6 +540,7 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,\n \tint color;\n \tstruct strbuf out = STRBUF_INIT, name = STRBUF_INIT;\n \tconst char *prefix = \"\";\n+\tconst char *desc = item->name;\n \n \tif (item->ignore)\n \t\treturn;\n@@ -516,6 +553,10 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,\n \t\tcolor = BRANCH_COLOR_REMOTE;\n \t\tprefix = remote_prefix;\n \t\tbreak;\n+\tcase REF_DETACHED_HEAD:\n+\t\tcolor = BRANCH_COLOR_CURRENT;\n+\t\tdesc = get_head_description();\n+\t\tbreak;\n \tdefault:\n \t\tcolor = BRANCH_COLOR_PLAIN;\n \t\tbreak;\n@@ -527,7 +568,7 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,\n \t\tcolor = BRANCH_COLOR_CURRENT;\n \t}\n \n-\tstrbuf_addf(&name, \"%s%s\", prefix, item->name);\n+\tstrbuf_addf(&name, \"%s%s\", prefix, desc);\n \tif (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@@ -569,56 +610,9 @@ static int calc_maxwidth(struct ref_list *refs, int remote_bonus)\n \treturn max;\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-static void show_detached(struct ref_list *ref_list, int maxwidth)\n-{\n-\tstruct commit *head_commit = lookup_commit_reference_gently(head_sha1, 1);\n-\n-\tif (head_commit && is_descendant_of(head_commit, ref_list->with_commit)) {\n-\t\tstruct ref_item item;\n-\t\titem.name = get_head_description();\n-\t\titem.kind = REF_LOCAL_BRANCH;\n-\t\titem.dest = NULL;\n-\t\titem.commit = head_commit;\n-\t\titem.ignore = 0;\n-\t\tprint_ref_item(&item, maxwidth, ref_list->verbose, ref_list->abbrev, 1, \"\");\n-\t\tfree(item.name);\n-\t}\n-}\n-\n static int print_ref_list(int kinds, int detached, int verbose, int abbrev, struct commit_list *with_commit, const char **pattern)\n {\n-\tint i;\n+\tint i, index;\n \tstruct append_ref_cb cb;\n \tstruct ref_list ref_list;\n \tint maxwidth = 0;\n@@ -643,6 +637,8 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru\n \tcb.pattern = pattern;\n \tcb.ret = 0;\n \tfor_each_rawref(append_ref, &cb);\n+\tif (detached)\n+\t\thead_ref(append_ref, &cb);\n \t/*\n \t * The following implementation is currently duplicated in ref-filter. It\n \t * will eventually be removed when we port branch.c to use ref-filter APIs.\n@@ -678,15 +674,20 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru\n \tif (verbose)\n \t\tmaxwidth = calc_maxwidth(&ref_list, strlen(remote_prefix));\n \n-\tqsort(ref_list.list, ref_list.index, sizeof(struct ref_item), ref_cmp);\n+\tindex = ref_list.index;\n+\n+\t/* Print detached heads before sorting and printing the rest */\n+\tif (detached && (ref_list.list[index - 1].kind == REF_DETACHED_HEAD) &&\n+\t !strcmp(ref_list.list[index - 1].name, head)) {\n+\t\tprint_ref_item(&ref_list.list[index - 1], maxwidth, verbose, abbrev,\n+\t\t\t 1, remote_prefix);\n+\t\tindex -= 1;\n+\t}\n \n-\tdetached = (detached && (kinds & REF_LOCAL_BRANCH));\n-\tif (detached && match_patterns(pattern, \"HEAD\"))\n-\t\tshow_detached(&ref_list, maxwidth);\n+\tqsort(ref_list.list, index, sizeof(struct ref_item), ref_cmp);\n \n-\tfor (i = 0; i < ref_list.index; i++) {\n-\t\tint current = !detached &&\n-\t\t\t(ref_list.list[i].kind == REF_LOCAL_BRANCH) &&\n+\tfor (i = 0; i < index; i++) {\n+\t\tint current = !detached && (ref_list.list[i].kind == REF_LOCAL_BRANCH) &&\n \t\t\t!strcmp(ref_list.list[i].name, head);\n \t\tprint_ref_item(&ref_list.list[i], maxwidth, verbose,\n \t\t\t abbrev, current, remote_prefix);\n@@ -913,7 +914,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"branch name required\"));\n \t\treturn delete_branches(argc, argv, delete > 1, kinds, quiet);\n \t} else if (list) {\n-\t\tint ret = print_ref_list(kinds, detached, verbose, abbrev,\n+\t\tint ret;\n+\t\tif (kinds & REF_LOCAL_BRANCH)\n+\t\t\tkinds |= REF_DETACHED_HEAD;\n+\t\tret = print_ref_list(kinds, detached, verbose, abbrev,\n \t\t\t\t\t with_commit, argv);\n \t\tprint_columns(&output, colopts, NULL);\n \t\tstring_list_clear(&output, 0);\n-- \n2.4.6\n"},{"id":"266896","messageId":"1438066594-5620-7-git-send-email-Karthik.188@gmail.com","threadId":"39952","inReplyTo":"1438066594-5620-1-git-send-email-Karthik.188@gmail.com","subject":"[RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-28T06:56:32Z","receivedAt":"2015-07-28T06:56:32Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"We check if given ref is the current branch in print_ref_list(). Move\nthis check to print_ref_item() where it is checked right before\nprinting.\n\nBased-on-patch-by: Jeff King \nMentored-by: Christian Couder \nMentored-by: Matthieu Moy \nSigned-off-by: Karthik Nayak \n---\n builtin/branch.c | 29 ++++++++++++-----------------\n 1 file changed, 12 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex f3d83d0..ba9cbad 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -534,9 +534,9 @@ static char *get_head_description(void)\n }\n \n static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,\n-\t\t\t int abbrev, int current, const char *remote_prefix)\n+\t\t\t int abbrev, int detached, const char *remote_prefix)\n {\n-\tchar c;\n+\tchar c = ' ';\n \tint color;\n \tstruct strbuf out = STRBUF_INIT, name = STRBUF_INIT;\n \tconst char *prefix = \"\";\n@@ -547,7 +547,11 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,\n \n \tswitch (item->kind) {\n \tcase REF_LOCAL_BRANCH:\n-\t\tcolor = BRANCH_COLOR_LOCAL;\n+\t\tif (!detached && !strcmp(item->name, head)) {\n+\t\t\tcolor = BRANCH_COLOR_CURRENT;\n+\t\t\tc = '*';\n+\t\t} else\n+\t\t\tcolor = BRANCH_COLOR_LOCAL;\n \t\tbreak;\n \tcase REF_REMOTE_BRANCH:\n \t\tcolor = BRANCH_COLOR_REMOTE;\n@@ -556,18 +560,13 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,\n \tcase REF_DETACHED_HEAD:\n \t\tcolor = BRANCH_COLOR_CURRENT;\n \t\tdesc = get_head_description();\n+\t\tc = '*';\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 (verbose) {\n \t\tint utf8_compensation = strlen(name.buf) - utf8_strwidth(name.buf);\n@@ -677,21 +676,17 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru\n \tindex = ref_list.index;\n \n \t/* Print detached heads before sorting and printing the rest */\n-\tif (detached && (ref_list.list[index - 1].kind == REF_DETACHED_HEAD) &&\n-\t !strcmp(ref_list.list[index - 1].name, head)) {\n+\tif (detached) {\n \t\tprint_ref_item(&ref_list.list[index - 1], maxwidth, verbose, abbrev,\n-\t\t\t 1, remote_prefix);\n+\t\t\t detached, remote_prefix);\n \t\tindex -= 1;\n \t}\n \n \tqsort(ref_list.list, index, sizeof(struct ref_item), ref_cmp);\n \n-\tfor (i = 0; i < index; i++) {\n-\t\tint current = !detached && (ref_list.list[i].kind == REF_LOCAL_BRANCH) &&\n-\t\t\t!strcmp(ref_list.list[i].name, head);\n+\tfor (i = 0; i < index; i++)\n \t\tprint_ref_item(&ref_list.list[i], maxwidth, verbose,\n-\t\t\t abbrev, current, remote_prefix);\n-\t}\n+\t\t\t abbrev, detached, remote_prefix);\n \n \tfree_ref_list(&ref_list);\n \n-- \n2.4.6\n"},{"id":"266898","messageId":"1438066594-5620-8-git-send-email-Karthik.188@gmail.com","threadId":"39952","inReplyTo":"1438066594-5620-1-git-send-email-Karthik.188@gmail.com","subject":"[RFC/PATCH 08/11] branch: drop non-commit error reporting","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-28T06:56:33Z","receivedAt":"2015-07-28T06:56:33Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Based-on-patch-by: Jeff King \nMentored-by: Christian Couder \nMentored-by: Matthieu Moy \nSigned-off-by: Karthik Nayak \n---\n builtin/branch.c | 18 ++++--------------\n 1 file changed, 4 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex ba9cbad..8d0521e 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -313,7 +313,6 @@ static char *resolve_symref(const char *src, const char *prefix)\n struct append_ref_cb {\n \tstruct ref_list *ref_list;\n \tconst char **pattern;\n-\tint ret;\n };\n \n static int match_patterns(const char **pattern, const char *refname)\n@@ -370,10 +369,8 @@ static int append_ref(const char *refname, const struct object_id *oid, int flag\n \tcommit = NULL;\n \tif (ref_list->verbose || ref_list->with_commit || merge_filter != NO_FILTER) {\n \t\tcommit = lookup_commit_reference_gently(oid->hash, 1);\n-\t\tif (!commit) {\n-\t\t\tcb->ret = error(_(\"branch '%s' does not point at a commit\"), refname);\n+\t\tif (!commit)\n \t\t\treturn 0;\n-\t\t}\n \n \t\t/* Filter with with_commit if specified */\n \t\tif (!is_descendant_of(commit, ref_list->with_commit))\n@@ -609,7 +606,7 @@ static int calc_maxwidth(struct ref_list *refs, int remote_bonus)\n \treturn max;\n }\n \n-static int print_ref_list(int kinds, int detached, int verbose, int abbrev, struct commit_list *with_commit, const char **pattern)\n+static void print_ref_list(int kinds, int detached, int verbose, int abbrev, struct commit_list *with_commit, const char **pattern)\n {\n \tint i, index;\n \tstruct append_ref_cb cb;\n@@ -634,7 +631,6 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru\n \t\tinit_revisions(&ref_list.revs, NULL);\n \tcb.ref_list = &ref_list;\n \tcb.pattern = pattern;\n-\tcb.ret = 0;\n \tfor_each_rawref(append_ref, &cb);\n \tif (detached)\n \t\thead_ref(append_ref, &cb);\n@@ -689,11 +685,6 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru\n \t\t\t abbrev, detached, remote_prefix);\n \n \tfree_ref_list(&ref_list);\n-\n-\tif (cb.ret)\n-\t\terror(_(\"some refs could not be read\"));\n-\n-\treturn cb.ret;\n }\n \n static void rename_branch(const char *oldname, const char *newname, int force)\n@@ -909,14 +900,13 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"branch name required\"));\n \t\treturn delete_branches(argc, argv, delete > 1, kinds, quiet);\n \t} else if (list) {\n-\t\tint ret;\n \t\tif (kinds & REF_LOCAL_BRANCH)\n \t\t\tkinds |= REF_DETACHED_HEAD;\n-\t\tret = print_ref_list(kinds, detached, verbose, abbrev,\n+\t\tprint_ref_list(kinds, detached, verbose, abbrev,\n \t\t\t\t\t with_commit, argv);\n \t\tprint_columns(&output, colopts, NULL);\n \t\tstring_list_clear(&output, 0);\n-\t\treturn ret;\n+\t\treturn 0;\n \t}\n \telse if (edit_description) {\n \t\tconst char *branch_name;\n-- \n2.4.6\n"},{"id":"266899","messageId":"1438066594-5620-9-git-send-email-Karthik.188@gmail.com","threadId":"39952","inReplyTo":"1438066594-5620-1-git-send-email-Karthik.188@gmail.com","subject":"[RFC/PATCH 09/11] branch.c: use 'ref-filter' data structures","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-28T06:56:34Z","receivedAt":"2015-07-28T06:56:34Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Make 'branch.c' use 'ref-filter' data structures and make changes to\nsupport the new data structures. This is a part of the process of\nporting 'branch.c' to use 'ref-filter' APIs.\n\nThis is a temporary step before porting 'branch.c' to use 'ref-filter'\ncompletely. As this is a temporary step, most of the code introduced\nhere will be removed when 'branch.c' is ported over to use\n'ref-filter' APIs\n\nMentored-by: Christian Couder \nMentored-by: Matthieu Moy \nSigned-off-by: Karthik Nayak \n---\n builtin/branch.c | 289 +++++++++++++++++++++----------------------------------\n ref-filter.h | 7 +-\n 2 files changed, 116 insertions(+), 180 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 8d0521e..df74527 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -19,6 +19,7 @@\n #include \"column.h\"\n #include \"utf8.h\"\n #include \"wt-status.h\"\n+#include \"ref-filter.h\"\n \n static const char * const builtin_branch_usage[] = {\n \tN_(\"git branch [] [-r | -a] [--merged | --no-merged]\"),\n@@ -28,10 +29,6 @@ static const char * const builtin_branch_usage[] = {\n \tNULL\n };\n \n-#define REF_LOCAL_BRANCH 0x01\n-#define REF_REMOTE_BRANCH 0x02\n-#define REF_DETACHED_HEAD 0x04\n-\n static const char *head;\n static unsigned char head_sha1[20];\n \n@@ -53,13 +50,6 @@ enum color_branch {\n \tBRANCH_COLOR_UPSTREAM = 5\n };\n \n-static enum merge_filter {\n-\tNO_FILTER = 0,\n-\tSHOW_NOT_MERGED,\n-\tSHOW_MERGED\n-} merge_filter;\n-static unsigned char merge_filter_ref[20];\n-\n static struct string_list output = STRING_LIST_INIT_DUP;\n static unsigned int colopts;\n \n@@ -280,22 +270,6 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \treturn(ret);\n }\n \n-struct ref_item {\n-\tchar *name;\n-\tchar *dest;\n-\tunsigned int kind;\n-\tstruct commit *commit;\n-\tint ignore;\n-};\n-\n-struct ref_list {\n-\tstruct rev_info revs;\n-\tint index, alloc, verbose, abbrev;\n-\tstruct ref_item *list;\n-\tstruct commit_list *with_commit;\n-\tint kinds;\n-};\n-\n static char *resolve_symref(const char *src, const char *prefix)\n {\n \tunsigned char sha1[20];\n@@ -310,11 +284,6 @@ static char *resolve_symref(const char *src, const char *prefix)\n \treturn xstrdup(dst);\n }\n \n-struct append_ref_cb {\n-\tstruct ref_list *ref_list;\n-\tconst char **pattern;\n-};\n-\n static int match_patterns(const char **pattern, const char *refname)\n {\n \tif (!*pattern)\n@@ -327,11 +296,22 @@ static int match_patterns(const char **pattern, const char *refname)\n \treturn 0;\n }\n \n+static void ref_array_append(struct ref_array *array, const char *refname)\n+{\n+\tsize_t len = strlen(refname);\n+\tstruct ref_array_item *ref = xcalloc(1, sizeof(struct ref_array_item) + len + 1);\n+\tmemcpy(ref->refname, refname, len);\n+\tref->refname[len] = '\\0';\n+\tREALLOC_ARRAY(array->items, array->nr + 1);\n+\tarray->items[array->nr++] = ref;\n+}\n+\n static int append_ref(const char *refname, const struct object_id *oid, int flags, void *cb_data)\n {\n-\tstruct append_ref_cb *cb = (struct append_ref_cb *)(cb_data);\n-\tstruct ref_list *ref_list = cb->ref_list;\n-\tstruct ref_item *newitem;\n+\tstruct ref_filter_cbdata *cb = (struct ref_filter_cbdata *)(cb_data);\n+\tstruct ref_filter *filter = cb->filter;\n+\tstruct ref_array *array = cb->array;\n+\tstruct ref_array_item *item;\n \tstruct commit *commit;\n \tint kind, i;\n \tconst char *prefix, *orig_refname = refname;\n@@ -360,59 +340,47 @@ static int append_ref(const char *refname, const struct object_id *oid, int flag\n \t}\n \n \t/* Don't add types the caller doesn't want */\n-\tif ((kind & ref_list->kinds) == 0)\n+\tif ((kind & filter->branch_kind) == 0)\n \t\treturn 0;\n \n-\tif (!match_patterns(cb->pattern, refname))\n+\tif (!match_patterns(filter->name_patterns, refname))\n \t\treturn 0;\n \n \tcommit = NULL;\n-\tif (ref_list->verbose || ref_list->with_commit || merge_filter != NO_FILTER) {\n+\tif (filter->verbose || filter->with_commit || filter->merge != REF_FILTER_MERGED_NONE) {\n \t\tcommit = lookup_commit_reference_gently(oid->hash, 1);\n \t\tif (!commit)\n \t\t\treturn 0;\n \n \t\t/* Filter with with_commit if specified */\n-\t\tif (!is_descendant_of(commit, ref_list->with_commit))\n+\t\tif (!is_descendant_of(commit, filter->with_commit))\n \t\t\treturn 0;\n \n-\t\tif (merge_filter != NO_FILTER)\n-\t\t\tadd_pending_object(&ref_list->revs,\n+\t\tif (filter->merge != REF_FILTER_MERGED_NONE)\n+\t\t\tadd_pending_object(array->revs,\n \t\t\t\t\t (struct object *)commit, refname);\n \t}\n \n-\tALLOC_GROW(ref_list->list, ref_list->index + 1, ref_list->alloc);\n+\tref_array_append(array, refname);\n+\titem = array->items[array->nr - 1];\n \n \t/* Record the new item */\n-\tnewitem = &(ref_list->list[ref_list->index++]);\n-\tnewitem->name = xstrdup(refname);\n-\tnewitem->kind = kind;\n-\tnewitem->commit = commit;\n-\tnewitem->dest = resolve_symref(orig_refname, prefix);\n-\tnewitem->ignore = 0;\n+\titem->kind = kind;\n+\titem->commit = commit;\n+\titem->symref = resolve_symref(orig_refname, prefix);\n+\titem->ignore = 0;\n \n \treturn 0;\n }\n \n-static void free_ref_list(struct ref_list *ref_list)\n-{\n-\tint i;\n-\n-\tfor (i = 0; i < ref_list->index; i++) {\n-\t\tfree(ref_list->list[i].name);\n-\t\tfree(ref_list->list[i].dest);\n-\t}\n-\tfree(ref_list->list);\n-}\n-\n static int ref_cmp(const void *r1, const void *r2)\n {\n-\tstruct ref_item *c1 = (struct ref_item *)(r1);\n-\tstruct ref_item *c2 = (struct ref_item *)(r2);\n+\tstruct ref_array_item *c1 = *((struct ref_array_item **)r1);\n+\tstruct ref_array_item *c2 = *((struct ref_array_item **)r2);\n \n \tif (c1->kind != c2->kind)\n \t\treturn c1->kind - c2->kind;\n-\treturn strcmp(c1->name, c2->name);\n+\treturn strcmp(c1->refname, c2->refname);\n }\n \n static void fill_tracking_info(struct strbuf *stat, const char *branch_name,\n@@ -477,8 +445,8 @@ static void fill_tracking_info(struct strbuf *stat, const char *branch_name,\n \tfree(ref);\n }\n \n-static void add_verbose_info(struct strbuf *out, struct ref_item *item,\n-\t\t\t int verbose, int abbrev)\n+static void add_verbose_info(struct strbuf *out, struct ref_array_item *item,\n+\t\t\t struct ref_filter *filter)\n {\n \tstruct strbuf subject = STRBUF_INIT, stat = STRBUF_INIT;\n \tconst char *sub = _(\" **** invalid ref ****\");\n@@ -490,10 +458,10 @@ static void add_verbose_info(struct strbuf *out, struct ref_item *item,\n \t}\n \n \tif (item->kind == REF_LOCAL_BRANCH)\n-\t\tfill_tracking_info(&stat, item->name, verbose > 1);\n+\t\tfill_tracking_info(&stat, item->refname, filter->verbose > 1);\n \n \tstrbuf_addf(out, \" %s %s%s\",\n-\t\tfind_unique_abbrev(item->commit->object.sha1, abbrev),\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@@ -530,21 +498,21 @@ static char *get_head_description(void)\n \treturn strbuf_detach(&desc, NULL);\n }\n \n-static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,\n-\t\t\t int abbrev, int detached, const char *remote_prefix)\n+static void print_ref_item(struct ref_array_item *item, int maxwidth,\n+\t\t\t struct ref_filter *filter, const char *remote_prefix)\n {\n \tchar c = ' ';\n \tint color;\n \tstruct strbuf out = STRBUF_INIT, name = STRBUF_INIT;\n \tconst char *prefix = \"\";\n-\tconst char *desc = item->name;\n+\tconst char *desc = item->refname;\n \n \tif (item->ignore)\n \t\treturn;\n \n \tswitch (item->kind) {\n \tcase REF_LOCAL_BRANCH:\n-\t\tif (!detached && !strcmp(item->name, head)) {\n+\t\tif (!filter->detached && !strcmp(item->refname, head)) {\n \t\t\tcolor = BRANCH_COLOR_CURRENT;\n \t\t\tc = '*';\n \t\t} else\n@@ -565,7 +533,7 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,\n \t}\n \n \tstrbuf_addf(&name, \"%s%s\", prefix, desc);\n-\tif (verbose) {\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@@ -574,13 +542,13 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,\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->dest)\n-\t\tstrbuf_addf(&out, \" -> %s\", item->dest);\n-\telse if (verbose)\n+\tif (item->symref)\n+\t\tstrbuf_addf(&out, \" -> %s\", item->symref);\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, verbose, abbrev);\n+\t\tadd_verbose_info(&out, item, filter);\n \tif (column_active(colopts)) {\n-\t\tassert(!verbose && \"--column and --verbose are incompatible\");\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@@ -589,14 +557,14 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,\n \tstrbuf_release(&out);\n }\n \n-static int calc_maxwidth(struct ref_list *refs, int remote_bonus)\n+static int calc_maxwidth(struct ref_array *refs, int remote_bonus)\n {\n \tint i, max = 0;\n-\tfor (i = 0; i < refs->index; i++) {\n-\t\tstruct ref_item *it = &refs->list[i];\n-\t\tint w = utf8_strwidth(it->name);\n+\tfor (i = 0; i < refs->nr; i++) {\n+\t\tstruct ref_array_item *it = refs->items[i];\n+\t\tint w = utf8_strwidth(it->refname);\n \n-\t\tif (refs->list[i].ignore)\n+\t\tif (it->ignore)\n \t\t\tcontinue;\n \t\tif (it->kind == REF_REMOTE_BRANCH)\n \t\t\tw += remote_bonus;\n@@ -606,85 +574,76 @@ static int calc_maxwidth(struct ref_list *refs, int remote_bonus)\n \treturn max;\n }\n \n-static void print_ref_list(int kinds, int detached, int verbose, int abbrev, struct commit_list *with_commit, const char **pattern)\n+static void print_ref_list(struct ref_filter *filter)\n {\n \tint i, index;\n-\tstruct append_ref_cb cb;\n-\tstruct ref_list ref_list;\n+\tstruct ref_array array;\n+\tstruct ref_filter_cbdata data;\n \tint maxwidth = 0;\n \tconst char *remote_prefix = \"\";\n+\tstruct rev_info revs;\n \n \t/*\n \t * If we are listing more than just remote branches,\n \t * then remote branches will have a \"remotes/\" prefix.\n \t * We need to account for this in the width.\n \t */\n-\tif (kinds != REF_REMOTE_BRANCH)\n+\tif (filter->branch_kind != REF_REMOTE_BRANCH)\n \t\tremote_prefix = \"remotes/\";\n \n-\tmemset(&ref_list, 0, sizeof(ref_list));\n-\tref_list.kinds = kinds;\n-\tref_list.verbose = verbose;\n-\tref_list.abbrev = abbrev;\n-\tref_list.with_commit = with_commit;\n-\tif (merge_filter != NO_FILTER)\n-\t\tinit_revisions(&ref_list.revs, NULL);\n-\tcb.ref_list = &ref_list;\n-\tcb.pattern = pattern;\n-\tfor_each_rawref(append_ref, &cb);\n-\tif (detached)\n-\t\thead_ref(append_ref, &cb);\n+\tmemset(&array, 0, sizeof(array));\n+\tif (filter->merge != REF_FILTER_MERGED_NONE)\n+\t\tinit_revisions(&revs, NULL);\n+\n+\tdata.array = &array;\n+\tdata.filter = filter;\n+\tarray.revs = &revs;\n+\n+\tfor_each_rawref(append_ref, &data);\n+\tif (filter->detached)\n+\t\thead_ref(append_ref, &data);\n \t/*\n \t * The following implementation is currently duplicated in ref-filter. It\n \t * will eventually be removed when we port branch.c to use ref-filter APIs.\n \t */\n-\tif (merge_filter != NO_FILTER) {\n-\t\tstruct commit *filter;\n-\t\tfilter = lookup_commit_reference_gently(merge_filter_ref, 0);\n-\t\tif (!filter)\n-\t\t\tdie(_(\"object '%s' does not point to a commit\"),\n-\t\t\t sha1_to_hex(merge_filter_ref));\n-\n-\t\tfilter->object.flags |= UNINTERESTING;\n-\t\tadd_pending_object(&ref_list.revs,\n-\t\t\t\t (struct object *) filter, \"\");\n-\t\tref_list.revs.limited = 1;\n-\n-\t\tif (prepare_revision_walk(&ref_list.revs))\n+\tif (filter->merge != REF_FILTER_MERGED_NONE) {\n+\t\tfilter->merge_commit->object.flags |= UNINTERESTING;\n+\t\tadd_pending_object(&revs, &filter->merge_commit->object, \"\");\n+\t\trevs.limited = 1;\n+\n+\t\tif (prepare_revision_walk(&revs))\n \t\t\tdie(_(\"revision walk setup failed\"));\n \n-\t\tfor (i = 0; i < ref_list.index; i++) {\n-\t\t\tstruct ref_item *item = &ref_list.list[i];\n+\t\tfor (i = 0; i < array.nr; i++) {\n+\t\t\tstruct ref_array_item *item = array.items[i];\n \t\t\tstruct commit *commit = item->commit;\n \t\t\tint is_merged = !!(commit->object.flags & UNINTERESTING);\n-\t\t\titem->ignore = is_merged != (merge_filter == SHOW_MERGED);\n+\t\t\titem->ignore = is_merged != (filter->merge == REF_FILTER_MERGED_INCLUDE);\n \t\t}\n \n-\t\tfor (i = 0; i < ref_list.index; i++) {\n-\t\t\tstruct ref_item *item = &ref_list.list[i];\n+\t\tfor (i = 0; i < array.nr; i++) {\n+\t\t\tstruct ref_array_item *item = array.items[i];\n \t\t\tclear_commit_marks(item->commit, ALL_REV_FLAGS);\n \t\t}\n-\t\tclear_commit_marks(filter, ALL_REV_FLAGS);\n+\t\tclear_commit_marks(filter->merge_commit, ALL_REV_FLAGS);\n \t}\n-\tif (verbose)\n-\t\tmaxwidth = calc_maxwidth(&ref_list, strlen(remote_prefix));\n+\tif (filter->verbose)\n+\t\tmaxwidth = calc_maxwidth(&array, strlen(remote_prefix));\n \n-\tindex = ref_list.index;\n+\tindex = array.nr;\n \n \t/* Print detached heads before sorting and printing the rest */\n-\tif (detached) {\n-\t\tprint_ref_item(&ref_list.list[index - 1], maxwidth, verbose, abbrev,\n-\t\t\t detached, remote_prefix);\n+\tif (filter->detached) {\n+\t\tprint_ref_item(array.items[index - 1], maxwidth, filter, remote_prefix);\n \t\tindex -= 1;\n \t}\n \n-\tqsort(ref_list.list, index, sizeof(struct ref_item), ref_cmp);\n+\tqsort(array.items, index, sizeof(struct ref_array_item *), ref_cmp);\n \n \tfor (i = 0; i < index; i++)\n-\t\tprint_ref_item(&ref_list.list[i], maxwidth, verbose,\n-\t\t\t abbrev, detached, remote_prefix);\n+\t\tprint_ref_item(array.items[i], maxwidth, filter, remote_prefix);\n \n-\tfree_ref_list(&ref_list);\n+\tref_array_clear(&array);\n }\n \n static void rename_branch(const char *oldname, const char *newname, int force)\n@@ -740,24 +699,6 @@ static void rename_branch(const char *oldname, const char *newname, int force)\n \tstrbuf_release(&newsection);\n }\n \n-/*\n- * This function is duplicated in ref-filter. It will eventually be removed\n- * when we port branch.c to use ref-filter APIs.\n- */\n-static int opt_parse_merge_filter(const struct option *opt, const char *arg, int unset)\n-{\n-\tmerge_filter = ((opt->long_name[0] == 'n')\n-\t\t\t? SHOW_NOT_MERGED\n-\t\t\t: SHOW_MERGED);\n-\tif (unset)\n-\t\tmerge_filter = SHOW_NOT_MERGED; /* b/c for --no-merged */\n-\tif (!arg)\n-\t\targ = \"HEAD\";\n-\tif (get_sha1(arg, merge_filter_ref))\n-\t\tdie(_(\"malformed object name %s\"), arg);\n-\treturn 0;\n-}\n-\n static const char edit_description[] = \"BRANCH_DESCRIPTION\";\n \n static int edit_branch_description(const char *branch_name)\n@@ -797,17 +738,15 @@ static int edit_branch_description(const char *branch_name)\n int cmd_branch(int argc, const char **argv, const char *prefix)\n {\n \tint delete = 0, rename = 0, force = 0, list = 0;\n-\tint verbose = 0, abbrev = -1, detached = 0;\n \tint reflog = 0, edit_description = 0;\n \tint quiet = 0, unset_upstream = 0;\n \tconst char *new_upstream = NULL;\n \tenum branch_track track;\n-\tint kinds = REF_LOCAL_BRANCH;\n-\tstruct commit_list *with_commit = NULL;\n+\tstruct ref_filter filter;\n \n \tstruct option options[] = {\n \t\tOPT_GROUP(N_(\"Generic options\")),\n-\t\tOPT__VERBOSE(&verbose,\n+\t\tOPT__VERBOSE(&filter.verbose,\n \t\t\tN_(\"show hash and subject, give twice for upstream branch\")),\n \t\tOPT__QUIET(&quiet, N_(\"suppress informational messages\")),\n \t\tOPT_SET_INT('t', \"track\", &track, N_(\"set up tracking mode (see git-pull(1))\"),\n@@ -817,14 +756,14 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tOPT_STRING('u', \"set-upstream-to\", &new_upstream, \"upstream\", \"change the upstream info\"),\n \t\tOPT_BOOL(0, \"unset-upstream\", &unset_upstream, \"Unset the upstream info\"),\n \t\tOPT__COLOR(&branch_use_color, N_(\"use colored output\")),\n-\t\tOPT_SET_INT('r', \"remotes\", &kinds, N_(\"act on remote-tracking branches\"),\n+\t\tOPT_SET_INT('r', \"remotes\", &filter.branch_kind, N_(\"act on remote-tracking branches\"),\n \t\t\tREF_REMOTE_BRANCH),\n-\t\tOPT_CONTAINS(&with_commit, N_(\"print only branches that contain the commit\")),\n-\t\tOPT_WITH(&with_commit, N_(\"print only branches that contain the commit\")),\n-\t\tOPT__ABBREV(&abbrev),\n+\t\tOPT_CONTAINS(&filter.with_commit, N_(\"print only branches that contain the commit\")),\n+\t\tOPT_WITH(&filter.with_commit, N_(\"print only branches that contain the commit\")),\n+\t\tOPT__ABBREV(&filter.abbrev),\n \n \t\tOPT_GROUP(N_(\"Specific git-branch actions:\")),\n-\t\tOPT_SET_INT('a', \"all\", &kinds, N_(\"list both remote-tracking and local branches\"),\n+\t\tOPT_SET_INT('a', \"all\", &filter.branch_kind, N_(\"list both remote-tracking and local branches\"),\n \t\t\tREF_REMOTE_BRANCH | REF_LOCAL_BRANCH),\n \t\tOPT_BIT('d', \"delete\", &delete, N_(\"delete fully merged branch\"), 1),\n \t\tOPT_BIT('D', NULL, &delete, N_(\"delete branch (even if not merged)\"), 2),\n@@ -835,22 +774,16 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"edit-description\", &edit_description,\n \t\t\t N_(\"edit the description for the branch\")),\n \t\tOPT__FORCE(&force, N_(\"force creation, move/rename, deletion\")),\n-\t\t{\n-\t\t\tOPTION_CALLBACK, 0, \"no-merged\", &merge_filter_ref,\n-\t\t\tN_(\"commit\"), N_(\"print only not merged branches\"),\n-\t\t\tPARSE_OPT_LASTARG_DEFAULT | PARSE_OPT_NONEG,\n-\t\t\topt_parse_merge_filter, (intptr_t) \"HEAD\",\n-\t\t},\n-\t\t{\n-\t\t\tOPTION_CALLBACK, 0, \"merged\", &merge_filter_ref,\n-\t\t\tN_(\"commit\"), N_(\"print only merged branches\"),\n-\t\t\tPARSE_OPT_LASTARG_DEFAULT | PARSE_OPT_NONEG,\n-\t\t\topt_parse_merge_filter, (intptr_t) \"HEAD\",\n-\t\t},\n+\t\tOPT_MERGED(&filter, N_(\"print only branches that are merged\")),\n+\t\tOPT_NO_MERGED(&filter, N_(\"print only branches that are not merged\")),\n \t\tOPT_COLUMN(0, \"column\", &colopts, N_(\"list branches in columns\")),\n \t\tOPT_END(),\n \t};\n \n+\tmemset(&filter, 0, sizeof(filter));\n+\tfilter.branch_kind = REF_LOCAL_BRANCH;\n+\tfilter.abbrev = -1;\n+\n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n \t\tusage_with_options(builtin_branch_usage, options);\n \n@@ -862,11 +795,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \tif (!head)\n \t\tdie(_(\"Failed to resolve HEAD as a valid ref.\"));\n \tif (!strcmp(head, \"HEAD\"))\n-\t\tdetached = 1;\n+\t\tfilter.detached = 1;\n \telse if (!skip_prefix(head, \"refs/heads/\", &head))\n \t\tdie(_(\"HEAD not found below refs/heads!\"));\n-\thashcpy(merge_filter_ref, head_sha1);\n-\n \n \targc = parse_options(argc, argv, prefix, options, builtin_branch_usage,\n \t\t\t 0);\n@@ -874,17 +805,17 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \tif (!delete && !rename && !edit_description && !new_upstream && !unset_upstream && argc == 0)\n \t\tlist = 1;\n \n-\tif (with_commit || merge_filter != NO_FILTER)\n+\tif (filter.with_commit || filter.merge != REF_FILTER_MERGED_NONE)\n \t\tlist = 1;\n \n \tif (!!delete + !!rename + !!new_upstream +\n \t list + unset_upstream > 1)\n \t\tusage_with_options(builtin_branch_usage, options);\n \n-\tif (abbrev == -1)\n-\t\tabbrev = DEFAULT_ABBREV;\n+\tif (filter.abbrev == -1)\n+\t\tfilter.abbrev = DEFAULT_ABBREV;\n \tfinalize_colopts(&colopts, -1);\n-\tif (verbose) {\n+\tif (filter.verbose) {\n \t\tif (explicitly_enable_column(colopts))\n \t\t\tdie(_(\"--column and --verbose are incompatible\"));\n \t\tcolopts = 0;\n@@ -898,12 +829,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \tif (delete) {\n \t\tif (!argc)\n \t\t\tdie(_(\"branch name required\"));\n-\t\treturn delete_branches(argc, argv, delete > 1, kinds, quiet);\n+\t\treturn delete_branches(argc, argv, delete > 1, filter.branch_kind, quiet);\n \t} else if (list) {\n-\t\tif (kinds & REF_LOCAL_BRANCH)\n-\t\t\tkinds |= REF_DETACHED_HEAD;\n-\t\tprint_ref_list(kinds, detached, verbose, abbrev,\n-\t\t\t\t\t with_commit, argv);\n+\t\tif (filter.branch_kind & REF_LOCAL_BRANCH)\n+\t\t\tfilter.branch_kind |= REF_DETACHED_HEAD;\n+\t\tfilter.name_patterns = argv;\n+\t\tprint_ref_list(&filter);\n \t\tprint_columns(&output, colopts, NULL);\n \t\tstring_list_clear(&output, 0);\n \t\treturn 0;\n@@ -913,7 +844,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tstruct strbuf branch_ref = STRBUF_INIT;\n \n \t\tif (!argc) {\n-\t\t\tif (detached)\n+\t\t\tif (filter.detached)\n \t\t\t\tdie(_(\"Cannot give description to detached HEAD\"));\n \t\t\tbranch_name = head;\n \t\t} else if (argc == 1)\n@@ -1001,7 +932,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tif (!branch)\n \t\t\tdie(_(\"no such branch '%s'\"), argv[0]);\n \n-\t\tif (kinds != REF_LOCAL_BRANCH)\n+\t\tif (filter.branch_kind != REF_LOCAL_BRANCH)\n \t\t\tdie(_(\"-a and -r options to 'git branch' do not make sense with a branch name\"));\n \n \t\tif (track == BRANCH_TRACK_OVERRIDE)\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 7d1871d..3458595 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -5,6 +5,7 @@\n #include \"refs.h\"\n #include \"commit.h\"\n #include \"parse-options.h\"\n+#include \"revision.h\"\n \n /* Quoting styles */\n #define QUOTE_NONE 0\n@@ -48,6 +49,7 @@ struct ref_sorting {\n struct ref_array_item {\n \tunsigned char objectname[20];\n \tint flag, kind;\n+\tint ignore : 1;\n \tconst char *symref;\n \tstruct commit *commit;\n \tstruct atom_value *value;\n@@ -57,6 +59,7 @@ struct ref_array_item {\n struct ref_array {\n \tint nr, alloc;\n \tstruct ref_array_item **items;\n+\tstruct rev_info *revs;\n };\n \n struct ref_filter {\n@@ -72,8 +75,10 @@ struct ref_filter {\n \tstruct commit *merge_commit;\n \n \tunsigned int with_commit_tag_algo : 1,\n-\t\tmatch_as_path : 1;\n+\t\tmatch_as_path : 1,\n+\t\tdetached : 1;\n \tunsigned int lines, branch_kind;\n+\tint abbrev, verbose;\n };\n \n struct ref_filter_cbdata {\n-- \n2.4.6\n"},{"id":"266900","messageId":"1438067468-6835-1-git-send-email-Karthik.188@gmail.com","threadId":"39952","inReplyTo":"CAOLa=ZT3_DMJWFN62cbF19uxYBFsE69dGaFR=af1HPKsQ42otg@mail.gmail.com","subject":"[RFC/PATCH 10/11] branch.c: use 'ref-filter' APIs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-28T07:11:07Z","receivedAt":"2015-07-28T07:11:07Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Make 'branch.c' use 'ref-filter' APIs for iterating through refs\nsorting. This removes most of the code used in 'branch.c' replacing it\nwith calls to the 'ref-filter' library.\n\nMake 'tag.c' use the 'filter_refs()' function provided by 'ref-filter'\nto filter out tags based on the options set.\n\nWe provide a sorting option provided for 'branch.c' by using the sorting\noptions provided by 'ref-filter'.\n\nModify documentation and add tests for the same.\n\nMentored-by: Christian Couder \nMentored-by: Matthieu Moy \nSigned-off-by: Karthik Nayak \n---\n Documentation/git-branch.txt | 9 +-\n builtin/branch.c | 200 ++++++++-----------------------------------\n ref-filter.c | 8 +-\n ref-filter.h | 1 -\n t/t1430-bad-ref-name.sh | 2 +-\n t/t3203-branch-output.sh | 11 +++\n 6 files changed, 60 insertions(+), 171 deletions(-)\n\ndiff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt\nindex a67138a..897cd81 100644\n--- a/Documentation/git-branch.txt\n+++ b/Documentation/git-branch.txt\n@@ -11,7 +11,7 @@ SYNOPSIS\n 'git branch' [--color[=] | --no-color] [-r | -a]\n \t[--list] [-v [--abbrev= | --no-abbrev]]\n \t[--column[=] | --no-column]\n-\t[(--merged | --no-merged | --contains) []] [...]\n+\t[(--merged | --no-merged | --contains) []] [--sort=] [...]\n 'git branch' [--set-upstream | --track | --no-track] [-l] [-f] []\n 'git branch' (--set-upstream-to= | -u ) []\n 'git branch' --unset-upstream []\n@@ -229,6 +229,13 @@ start-point is either a local or remote-tracking branch.\n \tThe new name for an existing branch. The same restrictions as for\n \t apply.\n \n+--sort=::\n+\tSort based on the key given. Prefix `-` to sort in descending\n+\torder of the value. You may use the --sort= option\n+\tmultiple times, in which case the last key becomes the primary\n+\tkey. The keys supported are the same as those in `git\n+\tfor-each-ref`. Sort order defaults to sorting based on branch\n+\ttype.\n \n Examples\n --------\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex df74527..75d8bfd 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -270,119 +270,6 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,\n \treturn(ret);\n }\n \n-static char *resolve_symref(const char *src, const char *prefix)\n-{\n-\tunsigned char sha1[20];\n-\tint flag;\n-\tconst char *dst;\n-\n-\tdst = resolve_ref_unsafe(src, 0, sha1, &flag);\n-\tif (!(dst && (flag & REF_ISSYMREF)))\n-\t\treturn NULL;\n-\tif (prefix)\n-\t\tskip_prefix(dst, prefix, &dst);\n-\treturn xstrdup(dst);\n-}\n-\n-static int match_patterns(const char **pattern, const char *refname)\n-{\n-\tif (!*pattern)\n-\t\treturn 1; /* no pattern always matches */\n-\twhile (*pattern) {\n-\t\tif (!wildmatch(*pattern, refname, 0, NULL))\n-\t\t\treturn 1;\n-\t\tpattern++;\n-\t}\n-\treturn 0;\n-}\n-\n-static void ref_array_append(struct ref_array *array, const char *refname)\n-{\n-\tsize_t len = strlen(refname);\n-\tstruct ref_array_item *ref = xcalloc(1, sizeof(struct ref_array_item) + len + 1);\n-\tmemcpy(ref->refname, refname, len);\n-\tref->refname[len] = '\\0';\n-\tREALLOC_ARRAY(array->items, array->nr + 1);\n-\tarray->items[array->nr++] = ref;\n-}\n-\n-static int append_ref(const char *refname, const struct object_id *oid, int flags, void *cb_data)\n-{\n-\tstruct ref_filter_cbdata *cb = (struct ref_filter_cbdata *)(cb_data);\n-\tstruct ref_filter *filter = cb->filter;\n-\tstruct ref_array *array = cb->array;\n-\tstruct ref_array_item *item;\n-\tstruct commit *commit;\n-\tint kind, i;\n-\tconst char *prefix, *orig_refname = refname;\n-\n-\tstatic struct {\n-\t\tint kind;\n-\t\tconst char *prefix;\n-\t} ref_kind[] = {\n-\t\t{ REF_LOCAL_BRANCH, \"refs/heads/\" },\n-\t\t{ REF_REMOTE_BRANCH, \"refs/remotes/\" },\n-\t};\n-\n-\t/* Detect kind */\n-\tfor (i = 0; i < ARRAY_SIZE(ref_kind); i++) {\n-\t\tprefix = ref_kind[i].prefix;\n-\t\tif (skip_prefix(refname, prefix, &refname)) {\n-\t\t\tkind = ref_kind[i].kind;\n-\t\t\tbreak;\n-\t\t}\n-\t}\n-\tif (ARRAY_SIZE(ref_kind) <= i) {\n-\t\tif (!strcmp(refname, \"HEAD\"))\n-\t\t\tkind = REF_DETACHED_HEAD;\n-\t\telse\n-\t\t\treturn 0;\n-\t}\n-\n-\t/* Don't add types the caller doesn't want */\n-\tif ((kind & filter->branch_kind) == 0)\n-\t\treturn 0;\n-\n-\tif (!match_patterns(filter->name_patterns, refname))\n-\t\treturn 0;\n-\n-\tcommit = NULL;\n-\tif (filter->verbose || filter->with_commit || filter->merge != REF_FILTER_MERGED_NONE) {\n-\t\tcommit = lookup_commit_reference_gently(oid->hash, 1);\n-\t\tif (!commit)\n-\t\t\treturn 0;\n-\n-\t\t/* Filter with with_commit if specified */\n-\t\tif (!is_descendant_of(commit, filter->with_commit))\n-\t\t\treturn 0;\n-\n-\t\tif (filter->merge != REF_FILTER_MERGED_NONE)\n-\t\t\tadd_pending_object(array->revs,\n-\t\t\t\t\t (struct object *)commit, refname);\n-\t}\n-\n-\tref_array_append(array, refname);\n-\titem = array->items[array->nr - 1];\n-\n-\t/* Record the new item */\n-\titem->kind = kind;\n-\titem->commit = commit;\n-\titem->symref = resolve_symref(orig_refname, prefix);\n-\titem->ignore = 0;\n-\n-\treturn 0;\n-}\n-\n-static int ref_cmp(const void *r1, const void *r2)\n-{\n-\tstruct ref_array_item *c1 = *((struct ref_array_item **)r1);\n-\tstruct ref_array_item *c2 = *((struct ref_array_item **)r2);\n-\n-\tif (c1->kind != c2->kind)\n-\t\treturn c1->kind - c2->kind;\n-\treturn strcmp(c1->refname, c2->refname);\n-}\n-\n static void fill_tracking_info(struct strbuf *stat, const char *branch_name,\n \t\tint show_upstream_ref)\n {\n@@ -446,7 +333,7 @@ static void fill_tracking_info(struct strbuf *stat, const char *branch_name,\n }\n \n static void add_verbose_info(struct strbuf *out, struct ref_array_item *item,\n-\t\t\t struct ref_filter *filter)\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@@ -458,7 +345,7 @@ static void add_verbose_info(struct strbuf *out, struct ref_array_item *item,\n \t}\n \n \tif (item->kind == REF_LOCAL_BRANCH)\n-\t\tfill_tracking_info(&stat, item->refname, filter->verbose > 1);\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@@ -507,18 +394,17 @@ static void print_ref_item(struct ref_array_item *item, int maxwidth,\n \tconst char *prefix = \"\";\n \tconst char *desc = item->refname;\n \n-\tif (item->ignore)\n-\t\treturn;\n-\n \tswitch (item->kind) {\n \tcase REF_LOCAL_BRANCH:\n-\t\tif (!filter->detached && !strcmp(item->refname, head)) {\n+\t\tskip_prefix(desc, \"refs/heads/\", &desc);\n+\t\tif (!filter->detached && !strcmp(desc, head)) {\n \t\t\tcolor = BRANCH_COLOR_CURRENT;\n \t\t\tc = '*';\n \t\t} else\n \t\t\tcolor = BRANCH_COLOR_LOCAL;\n \t\tbreak;\n \tcase REF_REMOTE_BRANCH:\n+\t\tskip_prefix(desc, \"refs/remotes/\", &desc);\n \t\tcolor = BRANCH_COLOR_REMOTE;\n \t\tprefix = remote_prefix;\n \t\tbreak;\n@@ -542,11 +428,13 @@ static void print_ref_item(struct ref_array_item *item, int maxwidth,\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\tstrbuf_addf(&out, \" -> %s\", item->symref);\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);\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@@ -562,10 +450,13 @@ static int calc_maxwidth(struct ref_array *refs, int remote_bonus)\n \tint i, max = 0;\n \tfor (i = 0; i < refs->nr; i++) {\n \t\tstruct ref_array_item *it = refs->items[i];\n-\t\tint w = utf8_strwidth(it->refname);\n+\t\tconst char *desc = it->refname;\n+\t\tint w;\n+\n+\t\tskip_prefix(it->refname, \"refs/heads/\", &desc);\n+\t\tskip_prefix(it->refname, \"refs/remotes/\", &desc);\n+\t\tw = utf8_strwidth(desc);\n \n-\t\tif (it->ignore)\n-\t\t\tcontinue;\n \t\tif (it->kind == REF_REMOTE_BRANCH)\n \t\t\tw += remote_bonus;\n \t\tif (w > max)\n@@ -574,14 +465,14 @@ static int calc_maxwidth(struct ref_array *refs, int remote_bonus)\n \treturn max;\n }\n \n-static void print_ref_list(struct ref_filter *filter)\n+static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sorting)\n {\n \tint i, index;\n \tstruct ref_array array;\n-\tstruct ref_filter_cbdata data;\n \tint maxwidth = 0;\n \tconst char *remote_prefix = \"\";\n-\tstruct rev_info revs;\n+\tstruct ref_sorting def_sorting;\n+\tconst char *sort_type = \"type\";\n \n \t/*\n \t * If we are listing more than just remote branches,\n@@ -592,41 +483,10 @@ static void print_ref_list(struct ref_filter *filter)\n \t\tremote_prefix = \"remotes/\";\n \n \tmemset(&array, 0, sizeof(array));\n-\tif (filter->merge != REF_FILTER_MERGED_NONE)\n-\t\tinit_revisions(&revs, NULL);\n \n-\tdata.array = &array;\n-\tdata.filter = filter;\n-\tarray.revs = &revs;\n+\tverify_ref_format(\"%(refname)%(symref)\");\n+\tfilter_refs(&array, filter, FILTER_REFS_BRANCHES);\n \n-\tfor_each_rawref(append_ref, &data);\n-\tif (filter->detached)\n-\t\thead_ref(append_ref, &data);\n-\t/*\n-\t * The following implementation is currently duplicated in ref-filter. It\n-\t * will eventually be removed when we port branch.c to use ref-filter APIs.\n-\t */\n-\tif (filter->merge != REF_FILTER_MERGED_NONE) {\n-\t\tfilter->merge_commit->object.flags |= UNINTERESTING;\n-\t\tadd_pending_object(&revs, &filter->merge_commit->object, \"\");\n-\t\trevs.limited = 1;\n-\n-\t\tif (prepare_revision_walk(&revs))\n-\t\t\tdie(_(\"revision walk setup failed\"));\n-\n-\t\tfor (i = 0; i < array.nr; i++) {\n-\t\t\tstruct ref_array_item *item = array.items[i];\n-\t\t\tstruct commit *commit = item->commit;\n-\t\t\tint is_merged = !!(commit->object.flags & UNINTERESTING);\n-\t\t\titem->ignore = is_merged != (filter->merge == REF_FILTER_MERGED_INCLUDE);\n-\t\t}\n-\n-\t\tfor (i = 0; i < array.nr; i++) {\n-\t\t\tstruct ref_array_item *item = array.items[i];\n-\t\t\tclear_commit_marks(item->commit, ALL_REV_FLAGS);\n-\t\t}\n-\t\tclear_commit_marks(filter->merge_commit, ALL_REV_FLAGS);\n-\t}\n \tif (filter->verbose)\n \t\tmaxwidth = calc_maxwidth(&array, strlen(remote_prefix));\n \n@@ -635,14 +495,21 @@ static void print_ref_list(struct ref_filter *filter)\n \t/* Print detached heads before sorting and printing the rest */\n \tif (filter->detached) {\n \t\tprint_ref_item(array.items[index - 1], maxwidth, filter, remote_prefix);\n-\t\tindex -= 1;\n+\t\tarray.nr--;\n \t}\n \n-\tqsort(array.items, index, sizeof(struct ref_array_item *), ref_cmp);\n+\tif (!sorting) {\n+\t\tdef_sorting.next = NULL;\n+\t\tdef_sorting.atom = parse_ref_filter_atom(sort_type,\n+\t\t\t\t\t\t\t sort_type + strlen(sort_type));\n+\t\tsorting = &def_sorting;\n+\t}\n+\tref_array_sort(sorting, &array);\n \n-\tfor (i = 0; i < index; i++)\n+\tfor (i = 0; i < array.nr; i++)\n \t\tprint_ref_item(array.items[i], maxwidth, filter, remote_prefix);\n \n+\tarray.nr = index;\n \tref_array_clear(&array);\n }\n \n@@ -743,6 +610,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \tconst char *new_upstream = NULL;\n \tenum branch_track track;\n \tstruct ref_filter filter;\n+\tstatic struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;\n \n \tstruct option options[] = {\n \t\tOPT_GROUP(N_(\"Generic options\")),\n@@ -777,6 +645,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tOPT_MERGED(&filter, N_(\"print only branches that are merged\")),\n \t\tOPT_NO_MERGED(&filter, N_(\"print only branches that are not merged\")),\n \t\tOPT_COLUMN(0, \"column\", &colopts, N_(\"list branches in columns\")),\n+\t\tOPT_CALLBACK(0 , \"sort\", sorting_tail, N_(\"key\"),\n+\t\t\t N_(\"field name to sort on\"), &parse_opt_ref_sorting),\n \t\tOPT_END(),\n \t};\n \n@@ -834,7 +704,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tif (filter.branch_kind & REF_LOCAL_BRANCH)\n \t\t\tfilter.branch_kind |= REF_DETACHED_HEAD;\n \t\tfilter.name_patterns = argv;\n-\t\tprint_ref_list(&filter);\n+\t\tprint_ref_list(&filter, sorting);\n \t\tprint_columns(&output, colopts, NULL);\n \t\tstring_list_clear(&output, 0);\n \t\treturn 0;\ndiff --git a/ref-filter.c b/ref-filter.c\nindex ff5a16b..52224f0 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1153,7 +1153,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n \t * obtain the commit using the 'oid' available and discard all\n \t * non-commits early. The actual filtering is done later.\n \t */\n-\tif (filter->merge_commit || filter->with_commit) {\n+\tif (filter->merge_commit || filter->with_commit || filter->verbose) {\n \t\tcommit = lookup_commit_reference_gently(oid->hash, 1);\n \t\tif (!commit)\n \t\t\treturn 0;\n@@ -1261,9 +1261,11 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int\n \t\tret = for_each_ref(ref_filter_handler, &ref_cbdata);\n \telse if (type & FILTER_REFS_TAGS)\n \t\tret = for_each_tag_ref_fullpath(ref_filter_handler, &ref_cbdata);\n-\telse if (type & FILTER_REFS_BRANCHES)\n+\telse if (type & FILTER_REFS_BRANCHES) {\n \t\tret = for_each_rawref(ref_filter_handler, &ref_cbdata);\n-\telse if (type)\n+\t\tif (filter->detached)\n+\t\t\thead_ref(ref_filter_handler, &ref_cbdata);\n+\t} else if (type)\n \t\tdie(\"filter_refs: invalid type\");\n \n \t/* Filters that need revision walking */\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 3458595..311543a 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -49,7 +49,6 @@ struct ref_sorting {\n struct ref_array_item {\n \tunsigned char objectname[20];\n \tint flag, kind;\n-\tint ignore : 1;\n \tconst char *symref;\n \tstruct commit *commit;\n \tstruct atom_value *value;\ndiff --git a/t/t1430-bad-ref-name.sh b/t/t1430-bad-ref-name.sh\nindex 16d0b8b..b45f5e2 100755\n--- a/t/t1430-bad-ref-name.sh\n+++ b/t/t1430-bad-ref-name.sh\n@@ -38,7 +38,7 @@ test_expect_success 'fast-import: fail on invalid branch name \"bad[branch]name\"'\n \ttest_must_fail git fast-import output &&\ndiff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh\nindex f51d0f3..38c68bd 100755\n--- a/t/t3203-branch-output.sh\n+++ b/t/t3203-branch-output.sh\n@@ -143,4 +143,15 @@ EOF\n \ttest_i18ncmp expect actual\n '\n \n+test_expect_success 'git branch `--sort` option' '\n+\tcat >expect <actual &&\n+\ttest_i18ncmp expect actual\n+'\n+\n test_done\n-- \n2.4.6\n"},{"id":"266901","messageId":"1438067468-6835-2-git-send-email-Karthik.188@gmail.com","threadId":"39952","inReplyTo":"CAOLa=ZT3_DMJWFN62cbF19uxYBFsE69dGaFR=af1HPKsQ42otg@mail.gmail.com","subject":"[RFC/PATCH 11/11] branch: add '--points-at' option","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-28T07:11:08Z","receivedAt":"2015-07-28T07:11:08Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Add the '--points-at' option provided by 'ref-filter'. The option lets\nthe user to list only branches which points at the given object.\n\nAdd documentation and tests for the same.\n\nMentored-by: Christian Couder \nMentored-by: Matthieu Moy \nSigned-off-by: Karthik Nayak \n---\n Documentation/git-branch.txt | 6 +++++-\n builtin/branch.c | 7 ++++++-\n t/t3203-branch-output.sh | 9 +++++++++\n 3 files changed, 20 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt\nindex 897cd81..efa23a5 100644\n--- a/Documentation/git-branch.txt\n+++ b/Documentation/git-branch.txt\n@@ -11,7 +11,8 @@ SYNOPSIS\n 'git branch' [--color[=] | --no-color] [-r | -a]\n \t[--list] [-v [--abbrev= | --no-abbrev]]\n \t[--column[=] | --no-column]\n-\t[(--merged | --no-merged | --contains) []] [--sort=] [...]\n+\t[(--merged | --no-merged | --contains) []] [--sort=]\n+\t[--points-at ] [...]\n 'git branch' [--set-upstream | --track | --no-track] [-l] [-f] []\n 'git branch' (--set-upstream-to= | -u ) []\n 'git branch' --unset-upstream []\n@@ -237,6 +238,9 @@ start-point is either a local or remote-tracking branch.\n \tfor-each-ref`. Sort order defaults to sorting based on branch\n \ttype.\n \n+--points-at ::\n+\tOnly list tags of the given object.\n+\n Examples\n --------\n \ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 75d8bfd..d25f43b 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -26,6 +26,7 @@ static const char * const builtin_branch_usage[] = {\n \tN_(\"git branch [] [-l] [-f] []\"),\n \tN_(\"git branch [] [-r] (-d | -D) ...\"),\n \tN_(\"git branch [] (-m | -M) [] \"),\n+\tN_(\"git branch [] [-r | -a] [--points-at]\"),\n \tNULL\n };\n \n@@ -647,6 +648,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tOPT_COLUMN(0, \"column\", &colopts, N_(\"list branches in columns\")),\n \t\tOPT_CALLBACK(0 , \"sort\", sorting_tail, N_(\"key\"),\n \t\t\t N_(\"field name to sort on\"), &parse_opt_ref_sorting),\n+\t\t{\n+\t\t\tOPTION_CALLBACK, 0, \"points-at\", &filter.points_at, N_(\"object\"),\n+\t\t\tN_(\"print only tags of the object\"), 0, parse_opt_object_name\n+\t\t},\n \t\tOPT_END(),\n \t};\n \n@@ -675,7 +680,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \tif (!delete && !rename && !edit_description && !new_upstream && !unset_upstream && argc == 0)\n \t\tlist = 1;\n \n-\tif (filter.with_commit || filter.merge != REF_FILTER_MERGED_NONE)\n+\tif (filter.with_commit || filter.merge != REF_FILTER_MERGED_NONE || filter.points_at.nr)\n \t\tlist = 1;\n \n \tif (!!delete + !!rename + !!new_upstream +\ndiff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh\nindex 38c68bd..1deb7cb 100755\n--- a/t/t3203-branch-output.sh\n+++ b/t/t3203-branch-output.sh\n@@ -154,4 +154,13 @@ EOF\n \ttest_i18ncmp expect actual\n '\n \n+test_expect_success 'git branch --points-at option' '\n+\tcat >expect <actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.4.6\n"},{"id":"266905","messageId":"CA+P7+xp6FKDw26Qudi+kHi+CQ-rB1BFwOQdk65VyyAwTRW=vGg@mail.gmail.com","threadId":"39952","inReplyTo":"1438067468-6835-2-git-send-email-Karthik.188@gmail.com","subject":"Re: [RFC/PATCH 11/11] branch: add '--points-at' option","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-07-28T07:46:26Z","receivedAt":"2015-07-28T07:46:26Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, Jul 28, 2015 at 12:11 AM, Karthik Nayak wrote:\n> Add the '--points-at' option provided by 'ref-filter'. The option lets\n> the user to list only branches which points at the given object.\n>\n> Add documentation and tests for the same.\n>\n> Mentored-by: Christian Couder \n> Mentored-by: Matthieu Moy \n> Signed-off-by: Karthik Nayak \n> ---\n> Documentation/git-branch.txt | 6 +++++-\n> builtin/branch.c | 7 ++++++-\n> t/t3203-branch-output.sh | 9 +++++++++\n> 3 files changed, 20 insertions(+), 2 deletions(-)\n>\n> diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt\n> index 897cd81..efa23a5 100644\n> --- a/Documentation/git-branch.txt\n> +++ b/Documentation/git-branch.txt\n> @@ -11,7 +11,8 @@ SYNOPSIS\n> 'git branch' [--color[=] | --no-color] [-r | -a]\n> [--list] [-v [--abbrev= | --no-abbrev]]\n> [--column[=] | --no-column]\n> - [(--merged | --no-merged | --contains) []] [--sort=] [...]\n> + [(--merged | --no-merged | --contains) []] [--sort=]\n> + [--points-at ] [...]\n> 'git branch' [--set-upstream | --track | --no-track] [-l] [-f] []\n> 'git branch' (--set-upstream-to= | -u ) []\n> 'git branch' --unset-upstream []\n> @@ -237,6 +238,9 @@ start-point is either a local or remote-tracking branch.\n> for-each-ref`. Sort order defaults to sorting based on branch\n> type.\n>\n> +--points-at ::\n> + Only list tags of the given object.\n> +\n\ns/tags/branches/ ?? Since this is for the branch version, I think this\nis just a copy-paste oversight.\n\n> Examples\n> --------\n>\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> index 75d8bfd..d25f43b 100644\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -26,6 +26,7 @@ static const char * const builtin_branch_usage[] = {\n> N_(\"git branch [] [-l] [-f] []\"),\n> N_(\"git branch [] [-r] (-d | -D) ...\"),\n> N_(\"git branch [] (-m | -M) [] \"),\n> + N_(\"git branch [] [-r | -a] [--points-at]\"),\n> NULL\n> };\n>\n> @@ -647,6 +648,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n> OPT_COLUMN(0, \"column\", &colopts, N_(\"list branches in columns\")),\n> OPT_CALLBACK(0 , \"sort\", sorting_tail, N_(\"key\"),\n> N_(\"field name to sort on\"), &parse_opt_ref_sorting),\n> + {\n> + OPTION_CALLBACK, 0, \"points-at\", &filter.points_at, N_(\"object\"),\n> + N_(\"print only tags of the object\"), 0, parse_opt_object_name\n> + },\n\nSame as above. s/tags/branches/\n\n> OPT_END(),\n> };\n>\n> @@ -675,7 +680,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n> if (!delete && !rename && !edit_description && !new_upstream && !unset_upstream && argc == 0)\n> list = 1;\n>\n> - if (filter.with_commit || filter.merge != REF_FILTER_MERGED_NONE)\n> + if (filter.with_commit || filter.merge != REF_FILTER_MERGED_NONE || filter.points_at.nr)\n> list = 1;\n>\n> if (!!delete + !!rename + !!new_upstream +\n> diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh\n> index 38c68bd..1deb7cb 100755\n> --- a/t/t3203-branch-output.sh\n> +++ b/t/t3203-branch-output.sh\n> @@ -154,4 +154,13 @@ EOF\n> test_i18ncmp expect actual\n> '\n>\n> +test_expect_success 'git branch --points-at option' '\n> + cat >expect < + master\n> + branch-one\n> +EOF\n> + git branch --points-at=branch-one >actual &&\n> + test_cmp expect actual\n> +'\n> +\n> test_done\n> --\n> 2.4.6\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"266906","messageId":"CA+P7+xq_jC=gE3J=PTkVx8BpTRzCJCfU0V-ydBve2FYbg1gmBg@mail.gmail.com","threadId":"39952","inReplyTo":"1438066594-5620-4-git-send-email-Karthik.188@gmail.com","subject":"Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-07-28T07:54:52Z","receivedAt":"2015-07-28T07:54:52Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Jul 27, 2015 at 11:56 PM, Karthik Nayak wrote:\n> The 'ifexists' atom allows us to print a required format if the\n> preceeding atom has a value. If the preceeding atom has no value then\n\nDon't you mean \"following atom\" here? since you do document it as \"the\nnext atom\" below you should fix the commit message as well to match.\nIn any respect, this is a useful formatting atom :)\n\n%(ifexists:[%s])%(atom) where the contents of atom will be placed into %s?\n\n> the format given is not printed. e.g. to print \"[]\" we can\n> now use the format \"%(ifexists:[%s])%(refname)\".\n>\n> Add documentation and test for the same.\n>\n> Mentored-by: Christian Couder \n> Mentored-by: Matthieu Moy \n> Signed-off-by: Karthik Nayak \n> ---\n> Documentation/git-for-each-ref.txt | 8 ++++++++\n> ref-filter.c | 37 ++++++++++++++++++++++++++++++++++---\n> ref-filter.h | 5 +++--\n> t/t6302-for-each-ref-filter.sh | 21 +++++++++++++++++++++\n> 4 files changed, 66 insertions(+), 5 deletions(-)\n>\n> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\n> index 9dc02aa..4424020 100644\n> --- a/Documentation/git-for-each-ref.txt\n> +++ b/Documentation/git-for-each-ref.txt\n> @@ -138,6 +138,14 @@ colornext::\n> `<:colorname>`. Not compatible with `padright` and resets any\n> previous `color`, if set.\n>\n> +ifexists::\n> + Print required string only if the next atom specified in the\n> + '--format' option exists.\n> + e.g. --format=\"%(ifexists:[%s])%(symref)\" prints the symref\n> + like \"[]\" only if the ref has a symref. This was\n> + incorporated to simulate the output of 'git branch -vv', where\n> + we need to display the upstream branch in square brackets.\n> +\n\nI suggest documenting that the atom will be placed into the contents\nof ifexists via the %s indicator, as you do show an example but don't\nexplicitely say %s is the formatting character.\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.\n> diff --git a/ref-filter.c b/ref-filter.c\n> index 3f40144..ff5a16b 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -58,6 +58,7 @@ static struct {\n> { \"color\" },\n> { \"padright\" },\n> { \"colornext\" },\n> + { \"ifexists\" },\n> };\n>\n> /*\n> @@ -722,6 +723,13 @@ static void populate_value(struct ref_array_item *ref)\n> v->modifier_atom = 1;\n> v->color_next = 1;\n> continue;\n> + } else if (starts_with(name, \"ifexists:\")) {\n> + skip_prefix(name, \"ifexists:\", &v->s);\n> + if (!*v->s)\n> + die(_(\"no string given with 'ifexists:'\"));\n> + v->modifier_atom = 1;\n> + v->ifexists = 1;\n> + continue;\n> } else\n> continue;\n>\n> @@ -1315,11 +1323,32 @@ static void apply_formatting_state(struct ref_formatting_state *state,\n> {\n> if (state->color_next && state->pad_to_right)\n> die(_(\"cannot use `colornext` and `padright` together\"));\n> - if (state->color_next) {\n> + if (state->pad_to_right && state->ifexists)\n> + die(_(\"cannot use 'align' and 'ifexists' together\"));\n> + if (state->color_next && !state->ifexists) {\n> strbuf_addf(value, \"%s%s%s\", state->color_next, v->s, GIT_COLOR_RESET);\n> return;\n> - }\n> - else if (state->pad_to_right) {\n> + } else if (state->ifexists) {\n> + const char *sp = state->ifexists;\n> +\n> + while (*sp) {\n> + if (*sp != '%') {\n> + strbuf_addch(value, *sp++);\n> + continue;\n> + } else if (sp[1] == '%') {\n> + strbuf_addch(value, *sp++);\n> + continue;\n> + } else if (sp[1] == 's') {\n> + if (state->color_next)\n> + strbuf_addf(value, \"%s%s%s\", state->color_next, v->s, GIT_COLOR_RESET);\n> + else\n> + strbuf_addstr(value, v->s);\n> + sp += 2;\n> + }\n> + }\n> +\n> + return;\n> + } else if (state->pad_to_right) {\n> if (!is_utf8(v->s))\n> strbuf_addf(value, \"%-*s\", state->pad_to_right, v->s);\n> else {\n> @@ -1413,6 +1442,8 @@ static void store_formatting_state(struct ref_formatting_state *state,\n> state->color_next = atomv->s;\n> if (atomv->pad_to_right)\n> state->pad_to_right = atomv->ul;\n> + if (atomv->ifexists)\n> + state->ifexists = atomv->s;\n> }\n>\n> static void reset_formatting_state(struct ref_formatting_state *state)\n> diff --git a/ref-filter.h b/ref-filter.h\n> index a021b04..7d1871d 100644\n> --- a/ref-filter.h\n> +++ b/ref-filter.h\n> @@ -28,13 +28,14 @@ struct atom_value {\n> unsigned long ul; /* used for sorting when not FIELD_STR */\n> unsigned int modifier_atom : 1, /* atoms which act as modifiers for the next atom */\n> pad_to_right : 1,\n> - color_next : 1;\n> + color_next : 1,\n> + ifexists : 1;\n> };\n>\n> struct ref_formatting_state {\n> int quote_style;\n> unsigned int pad_to_right;\n> - const char *color_next;\n> + const char *color_next, *ifexists;\n> };\n>\n> struct ref_sorting {\n> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\n> index 6aad069..29ed97b 100755\n> --- a/t/t6302-for-each-ref-filter.sh\n> +++ b/t/t6302-for-each-ref-filter.sh\n> @@ -149,4 +149,25 @@ test_expect_success 'check `colornext` format option' '\n> test_cmp expect actual\n> '\n>\n> +test_expect_success 'check `ifexists` format option' '\n> + cat >expect <<-\\EOF &&\n> + [foo1.10]\n> + [foo1.3]\n> + [foo1.6]\n> + EOF\n> + git for-each-ref --format=\"%(ifexists:[%s])%(refname:short)\" | grep \"foo\" >actual &&\n> + test_cmp expect actual\n> +'\n> +\n> +cat >expect < +[$(get_color green)foo1.10$(get_color reset)]||foo1.10\n> +[$(get_color green)foo1.3$(get_color reset)]||foo1.3\n> +[$(get_color green)foo1.6$(get_color reset)]||foo1.6\n> +EOF\n> +\n> +test_expect_success 'check `ifexists` with `colornext` format option' '\n> + git for-each-ref --format=\"%(ifexists:[%s])%(colornext:green)%(refname:short)||%(refname:short)\" | grep \"foo\" >actual &&\n> + test_cmp expect actual\n> +'\n> +\n> test_done\n> --\n> 2.4.6\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"266907","messageId":"CA+P7+xpU4KdmutkGDdiAaXFpzs4zq=f2rXhqxqogAPJqopjdRQ@mail.gmail.com","threadId":"39952","inReplyTo":"1438067468-6835-1-git-send-email-Karthik.188@gmail.com","subject":"Re: [RFC/PATCH 10/11] branch.c: use 'ref-filter' APIs","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-07-28T07:57:33Z","receivedAt":"2015-07-28T07:57:33Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, Jul 28, 2015 at 12:11 AM, Karthik Nayak wrote:\n> Make 'branch.c' use 'ref-filter' APIs for iterating through refs\n> sorting. This removes most of the code used in 'branch.c' replacing it\n> with calls to the 'ref-filter' library.\n>\n> Make 'tag.c' use the 'filter_refs()' function provided by 'ref-filter'\n> to filter out tags based on the options set.\n>\n\nThis patch doesn't do anything to tag.c, so you should update the\ncommit message to remove this or replace it with s/tag/branch if it\nwas intended to be about branch.c?\n\nRegards,\nJake\n"},{"id":"266908","messageId":"CAP8UFD1q5oxchNUhWc8331nAq-eVkzVOe9=56v68HcY2CDXW4A@mail.gmail.com","threadId":"39952","inReplyTo":"1438067468-6835-1-git-send-email-Karthik.188@gmail.com","subject":"Re: [RFC/PATCH 10/11] branch.c: use 'ref-filter' APIs","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-07-28T08:09:32Z","receivedAt":"2015-07-28T08:09:32Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Jul 28, 2015 at 9:11 AM, Karthik Nayak wrote:\n\n> diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt\n> index a67138a..897cd81 100644\n> --- a/Documentation/git-branch.txt\n> +++ b/Documentation/git-branch.txt\n> @@ -11,7 +11,7 @@ SYNOPSIS\n> 'git branch' [--color[=] | --no-color] [-r | -a]\n> [--list] [-v [--abbrev= | --no-abbrev]]\n> [--column[=] | --no-column]\n> - [(--merged | --no-merged | --contains) []] [...]\n> + [(--merged | --no-merged | --contains) []] [--sort=] [...]\n\nMaybe: [(--[no-]merged | --contains) []]\n"},{"id":"266909","messageId":"CAP8UFD3L4QEYQMCda7AbByXGZcvgy5r9y0LMBqEysb6AZ+ydOQ@mail.gmail.com","threadId":"39952","inReplyTo":"1438066594-5620-9-git-send-email-Karthik.188@gmail.com","subject":"Re: [RFC/PATCH 09/11] branch.c: use 'ref-filter' data structures","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-07-28T08:17:09Z","receivedAt":"2015-07-28T08:17:09Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Jul 28, 2015 at 8:56 AM, Karthik Nayak wrote:\n\n> +static void ref_array_append(struct ref_array *array, const char *refname)\n> +{\n> + size_t len = strlen(refname);\n> + struct ref_array_item *ref = xcalloc(1, sizeof(struct ref_array_item) + len + 1);\n> + memcpy(ref->refname, refname, len);\n> + ref->refname[len] = '\\0';\n> + REALLOC_ARRAY(array->items, array->nr + 1);\n> + array->items[array->nr++] = ref;\n> +}\n\nThis function belongs more to ref-filter.{c,h}...\n\n[...]\n\n> - ALLOC_GROW(ref_list->list, ref_list->index + 1, ref_list->alloc);\n> + ref_array_append(array, refname);\n> + item = array->items[array->nr - 1];\n\n...and the above is a bit ugly.\n"},{"id":"266910","messageId":"CAP8UFD1Bx_o2gWP0R4EbsHO0d3LRRJ2rpHDKQhRVQpTrD6pjBw@mail.gmail.com","threadId":"39952","inReplyTo":"1438066594-5620-9-git-send-email-Karthik.188@gmail.com","subject":"Re: [RFC/PATCH 09/11] branch.c: use 'ref-filter' data structures","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-07-28T08:22:59Z","receivedAt":"2015-07-28T08:22:59Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Jul 28, 2015 at 8:56 AM, Karthik Nayak wrote:\n> diff --git a/ref-filter.h b/ref-filter.h\n> index 7d1871d..3458595 100644\n> --- a/ref-filter.h\n> +++ b/ref-filter.h\n> @@ -5,6 +5,7 @@\n> #include \"refs.h\"\n> #include \"commit.h\"\n> #include \"parse-options.h\"\n> +#include \"revision.h\"\n>\n> /* Quoting styles */\n> #define QUOTE_NONE 0\n> @@ -48,6 +49,7 @@ struct ref_sorting {\n> struct ref_array_item {\n> unsigned char objectname[20];\n> int flag, kind;\n> + int ignore : 1;\n\nMaybe you could add a comment to say that this will be removed in the\nnext patch.\n\n> const char *symref;\n> struct commit *commit;\n> struct atom_value *value;\n"},{"id":"266911","messageId":"vpqio941xn5.fsf@anie.imag.fr","threadId":"39952","inReplyTo":"1438066594-5620-1-git-send-email-Karthik.188@gmail.com","subject":"Re: [RFC/PATCH 01/11] ref-filter: add \"%(objectname:size=X)\" option","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-07-28T08:42:22Z","receivedAt":"2015-07-28T08:42:22Z","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 (skip_prefix(name, \"objectname:size=\", &p)) {\n> +\t\tunsigned int size = atoi(p);\n\nYou have the same problem as for tag.c: don't use atoi, and make\naccurate error checking (absence of value, negative value, non-integer\nvalue).\n\nIf you have other higher-priorities task, leave a temporary comment \n/* FIXME: ... */ or /* TODO: ... */ and make sure you have no such\ncomment when you drop the \"RFC\" in the subject of your emails.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"266912","messageId":"vpqd1zc1xik.fsf@anie.imag.fr","threadId":"39952","inReplyTo":"1438066594-5620-2-git-send-email-Karthik.188@gmail.com","subject":"Re: [RFC/PATCH 02/11] ref-filter: add 'colornext' atom","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-07-28T08:45:07Z","receivedAt":"2015-07-28T08:45:07Z","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> --- a/t/t6302-for-each-ref-filter.sh\n> +++ b/t/t6302-for-each-ref-filter.sh\n> @@ -133,4 +133,20 @@ test_expect_success 'reverse version sort' '\n> \ttest_cmp expect actual\n> '\n> \n> +get_color ()\n> +{\n> +\tgit config --get-color no.such.slot \"$1\"\n> +}\n> +\n> +cat >expect < +$(get_color green)foo1.10$(get_color reset)||\n> +$(get_color green)foo1.3$(get_color reset)||\n> +$(get_color green)foo1.6$(get_color reset)||\n> +EOF\n> +\n> +test_expect_success 'check `colornext` format option' '\n> +\tgit for-each-ref --format=\"%(colornext:green)%(refname:short)||\" | grep \"foo\" >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nThis is not a very good test: you're not checking that colornext applies\nto the next and only this one. Similarly to what I suggested for\npadright, I'd suggest\n\n --format=\"%(refname:short)%(colornext:green)|%(refname:short)|%(refname:short)|\"\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"266913","messageId":"vpq7fpk1x9a.fsf@anie.imag.fr","threadId":"39952","inReplyTo":"1438066594-5620-4-git-send-email-Karthik.188@gmail.com","subject":"Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-07-28T08:50:41Z","receivedAt":"2015-07-28T08:50:41Z","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> --- a/t/t6302-for-each-ref-filter.sh\n> +++ b/t/t6302-for-each-ref-filter.sh\n> @@ -149,4 +149,25 @@ test_expect_success 'check `colornext` format option' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'check `ifexists` format option' '\n> +\tcat >expect <<-\\EOF &&\n> +\t[foo1.10]\n> +\t[foo1.3]\n> +\t[foo1.6]\n> +\tEOF\n> +\tgit for-each-ref --format=\"%(ifexists:[%s])%(refname:short)\" | grep \"foo\" >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nYou're testing only the positive case of ifexists, right? You need a\ntest for the negative case (i.e. the attribute does not exist) too.\nIdeally, check that the ifexist actually applies to the right attribute,\nlike\n\n--format '%(ifexist:<%s>)%(attribute1) %(ifexist:[%s])%(attribute2)'\n\nand manage to get an output like\n\n\n [bar]\n [barfoo]\n\n> +cat >expect < +[$(get_color green)foo1.10$(get_color reset)]||foo1.10\n> +[$(get_color green)foo1.3$(get_color reset)]||foo1.3\n> +[$(get_color green)foo1.6$(get_color reset)]||foo1.6\n> +EOF\n> +\n> +test_expect_success 'check `ifexists` with `colornext` format option' '\n> +\tgit for-each-ref --format=\"%(ifexists:[%s])%(colornext:green)%(refname:short)||%(refname:short)\" | grep \"foo\" >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nYou have a double || that is not useful. Not really harmful either, but\nit may distract the reader. You may want to s/||/|/.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"266914","messageId":"CAP8UFD2x5rz18hUXmJVWw81EMEVZc5Hm=_GUwfuc6Fo--8Ukfw@mail.gmail.com","threadId":"39952","inReplyTo":"1438066594-5620-2-git-send-email-Karthik.188@gmail.com","subject":"Re: [RFC/PATCH 02/11] ref-filter: add 'colornext' atom","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-07-28T09:13:50Z","receivedAt":"2015-07-28T09:13:50Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Jul 28, 2015 at 8:56 AM, Karthik Nayak wrote:\n> @@ -712,6 +713,15 @@ static void populate_value(struct ref_array_item *ref)\n> v->modifier_atom = 1;\n> v->pad_to_right = 1;\n> continue;\n> + } else if (starts_with(name, \"colornext:\")) {\n> + char color[COLOR_MAXLEN] = \"\";\n> +\n> + if (color_parse(name + 10, color) < 0)\n> + die(_(\"unable to parse format\"));\n\nMaybe use skip_prefix(), and die() with a more helpful message.\n\n> + v->s = xstrdup(color);\n> + v->modifier_atom = 1;\n> + v->color_next = 1;\n> + continue;\n> } else\n> continue;\n"},{"id":"266915","messageId":"vpq6154zk9o.fsf@anie.imag.fr","threadId":"39952","inReplyTo":"1438066594-5620-5-git-send-email-Karthik.188@gmail.com","subject":"Re: [RFC/PATCH 05/11] branch: fix width computation","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-07-28T09:47:15Z","receivedAt":"2015-07-28T09:47:15Z","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> From: Karthik Nayak \n\nWhy did send-email add this From: header? Strange, it has the same\ncontent as your actual From: field.\n\n> Remove unnecessary variables from ref_list and ref_item\n> which were used for width computation. Make other changes\n> accordingly. This patch is a precursor for porting branch.c\n> to use ref-filter APIs.\n\nYou can explain better why this is needed. I guess something like \"we're\nmaking struct ref_item similar to ref-filter's ref_array_item\", but the\nreader shouldn't have to guess.\n\nYou should adujst the subject like BTW, I don't think you are \"fixing\"\nanything.\n\nOn a side note: on the \"tag\" series, see how explaining better and\nsplitting patches led not only to a better history, but also to better\nfinal code, and to finding a actual bugs (the %(color) thing and the\nabsence of reset on the state variable) even after sereval rounds of\nreview? I'm being picky and demanding, but don't see that as a complain\nfrom me, just hints on getting even better ;-).\n\n> @@ -386,15 +386,8 @@ static int append_ref(const char *refname, const struct object_id *oid, int flag\n> \tnewitem->name = xstrdup(refname);\n> \tnewitem->kind = kind;\n> \tnewitem->commit = commit;\n> -\tnewitem->width = utf8_strwidth(refname);\n> \tnewitem->dest = resolve_symref(orig_refname, prefix);\n> \tnewitem->ignore = 0;\n> -\t/* adjust for \"remotes/\" */\n> -\tif (newitem->kind == REF_REMOTE_BRANCH &&\n> -\t ref_list->kinds != REF_REMOTE_BRANCH)\n> -\t\tnewitem->width += 8;\n> -\tif (newitem->width > ref_list->maxwidth)\n> -\t\tref_list->maxwidth = newitem->width;\n> \n> \treturn 0;\n> }\n\nOK, in the old code, the width computation is done when inserting the\nref into the array. In the new code, you build the array and then do the\nwidth computation. You can explain this better in the commit message I\nthink (instead of \"Make other changes accordingly\" which doesn't bring\nmuch).\n\n> @@ -505,11 +498,12 @@ static void add_verbose_info(struct strbuf *out, struct ref_item *item,\n> }\n> \n> static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,\n> -\t\t\t int abbrev, int current, char *prefix)\n> +\t\t\t int abbrev, int current, const char *remote_prefix)\n> {\n> \tchar c;\n> \tint color;\n> \tstruct strbuf out = STRBUF_INIT, name = STRBUF_INIT;\n> +\tconst char *prefix = \"\";\n> \n> \tif (item->ignore)\n> \t\treturn;\n> @@ -520,6 +514,7 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,\n> \t\tbreak;\n> \tcase REF_REMOTE_BRANCH:\n> \t\tcolor = BRANCH_COLOR_REMOTE;\n> +\t\tprefix = remote_prefix;\n> \t\tbreak;\n\nWhy do you need these two hunks? I did not investigate in details, but\nit seems you're calling print_ref_item either with prefix=\"\" or with\nprefix=remote_prefix so it would seem that keeping \"prefix\" as argument\nwould work. I guess I missed something.\n\n> -static int calc_maxwidth(struct ref_list *refs)\n> +static int calc_maxwidth(struct ref_list *refs, int remote_bonus)\n> {\n> -\tint i, w = 0;\n> +\tint i, max = 0;\n> \tfor (i = 0; i < refs->index; i++) {\n> +\t\tstruct ref_item *it = &refs->list[i];\n> +\t\tint w = utf8_strwidth(it->name);\n> +\n> \t\tif (refs->list[i].ignore)\n> \t\t\tcontinue;\n> -\t\tif (refs->list[i].width > w)\n> -\t\t\tw = refs->list[i].width;\n> +\t\tif (it->kind == REF_REMOTE_BRANCH)\n> +\t\t\tw += remote_bonus;\n> +\t\tif (w > max)\n> +\t\t\tmax = w;\n> \t}\n> -\treturn w;\n> +\treturn max;\n> }\n\nThe old code was using 'w' for the max and no variable for the value at\nthe current iteration. You're using 'max' for the max and 'w' at the\ncurrent iteration. Using the same name 'w' for different things in the\npre- and post-image of the patch distracts the reader.\n\nIt may make sense to s/w/max/ in a separate preparatory patch. Or maybe\nit's overkill.\n\n> @@ -600,21 +600,18 @@ static char *get_head_description(void)\n> \treturn strbuf_detach(&desc, NULL);\n> }\n> \n> -static void show_detached(struct ref_list *ref_list)\n> +static void show_detached(struct ref_list *ref_list, int maxwidth)\n> {\n> \tstruct commit *head_commit = lookup_commit_reference_gently(head_sha1, 1);\n> \n> \tif (head_commit && is_descendant_of(head_commit, ref_list->with_commit)) {\n> \t\tstruct ref_item item;\n> \t\titem.name = get_head_description();\n> -\t\titem.width = utf8_strwidth(item.name);\n> \t\titem.kind = REF_LOCAL_BRANCH;\n> \t\titem.dest = NULL;\n> \t\titem.commit = head_commit;\n> \t\titem.ignore = 0;\n> -\t\tif (item.width > ref_list->maxwidth)\n> -\t\t\tref_list->maxwidth = item.width;\n> -\t\tprint_ref_item(&item, ref_list->maxwidth, ref_list->verbose, ref_list->abbrev, 1, \"\");\n> +\t\tprint_ref_item(&item, maxwidth, ref_list->verbose, ref_list->abbrev, 1, \"\");\n> \t\tfree(item.name);\n> \t}\n> }\n...\n> +\tint maxwidth = 0;\n...\n> +\tif (verbose)\n> +\t\tmaxwidth = calc_maxwidth(&ref_list, strlen(remote_prefix));\n> \n> \tqsort(ref_list.list, ref_list.index, sizeof(struct ref_item), ref_cmp);\n> \n> \tdetached = (detached && (kinds & REF_LOCAL_BRANCH));\n> \tif (detached && match_patterns(pattern, \"HEAD\"))\n> -\t\tshow_detached(&ref_list);\n> +\t\tshow_detached(&ref_list, maxwidth);\n\nThis hunks could ideally go in a preparatory patch that would just move\nthe place where maxwidth is computed. This preparatory patch would just\nsay\n\n\tmaxwidth = ref_list->maxwidth;\n\nand then you would do the actual change to\n\n\tif (verbose)\n\t\tmaxwidth = calc_maxwidth(...)\n\nwithout the distracting changes in the function's argument list.\n\nI won't insist on that, again it may be overkill. But reading the patch,\nI found it both rather trivial and hard to read, so there's room for\nimprovement.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"266920","messageId":"vpqlhe0xwpv.fsf@anie.imag.fr","threadId":"39952","inReplyTo":"1438066594-5620-6-git-send-email-Karthik.188@gmail.com","subject":"Re: [RFC/PATCH 06/11] branch: roll show_detached HEAD into regular ref_list","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-07-28T13:01:16Z","receivedAt":"2015-07-28T13:01:16Z","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> Remove show_detached() and make detached HEAD to be rolled into\n> regular ref_list by adding REF_DETACHED_HEAD as a kind of branch and\n> supporting the same in append_ref().\n\nAgain, this lacks the \"why?\" explanation.\n\n> Bump get_head_description() to the top.\n\nHere also: why? And this could easily go to a separate patch to let the\nreviewer focus on actual changes.\n\n> @@ -504,6 +540,7 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,\n> \tint color;\n> \tstruct strbuf out = STRBUF_INIT, name = STRBUF_INIT;\n> \tconst char *prefix = \"\";\n> +\tconst char *desc = item->name;\n> \n> \tif (item->ignore)\n> \t\treturn;\n> @@ -516,6 +553,10 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,\n> \t\tcolor = BRANCH_COLOR_REMOTE;\n> \t\tprefix = remote_prefix;\n> \t\tbreak;\n> +\tcase REF_DETACHED_HEAD:\n> +\t\tcolor = BRANCH_COLOR_CURRENT;\n> +\t\tdesc = get_head_description();\n\nI think you're leaking a string here: get_head_description() builds an\nstrbuf and returns the dynamically allocated string, which you never\nfree.\n\n> -static void show_detached(struct ref_list *ref_list, int maxwidth)\n> -{\n> -\tstruct commit *head_commit = lookup_commit_reference_gently(head_sha1, 1);\n> -\n> -\tif (head_commit && is_descendant_of(head_commit, ref_list->with_commit)) {\n\nI'm not sure what this if was doing, and why you can get rid of it. My\nunderstanding is that with_commit comes from --contains, and in the\nprevious code the filtering was done at display time (detached HEAD was\nnot shown if it was not contained in commits specified with --contains).\n\nEventually, you'll use ref-filter to do this filtering so you won't need\nthis check at display time.\n\nBut am I correct that for a few commits, you ignore --contains on\ndetached HEAD?\n\n> @@ -678,15 +674,20 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru\n> \tif (verbose)\n> \t\tmaxwidth = calc_maxwidth(&ref_list, strlen(remote_prefix));\n> \n> -\tqsort(ref_list.list, ref_list.index, sizeof(struct ref_item), ref_cmp);\n> +\tindex = ref_list.index;\n> +\n> +\t/* Print detached heads before sorting and printing the rest */\n\nI think you mean head (no s) or HEAD. It's not Mercurial ;-).\n\n> +\tif (detached && (ref_list.list[index - 1].kind == REF_DETACHED_HEAD) &&\n> +\t !strcmp(ref_list.list[index - 1].name, head)) {\n> +\t\tprint_ref_item(&ref_list.list[index - 1], maxwidth, verbose, abbrev,\n> +\t\t\t 1, remote_prefix);\n> +\t\tindex -= 1;\n> +\t}\n\nThis relies on the assumption that HEAD has to be the last in the array.\nThe assumption is correct since you call head_ref(append_ref, &cb) after\nfor_each_rawref(append_ref, &cb). I think this deserves a comment to\nremind the reader that HEAD is always the last (here or at the call to\nhead_ref to make sure nobody try to change the order between head_ref\nand for_each_rawref).\n\nIt may be more natural to do it the other way around: call head_ref\nfirst and get HEAD first, as you are going to display it first (but in\nany case, you'll have to call qsort on a subset of the array so it\ndoesn't change much).\n\n> @@ -913,7 +914,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n> \t\t\tdie(_(\"branch name required\"));\n> \t\treturn delete_branches(argc, argv, delete > 1, kinds, quiet);\n> \t} else if (list) {\n> -\t\tint ret = print_ref_list(kinds, detached, verbose, abbrev,\n> +\t\tint ret;\n> +\t\tif (kinds & REF_LOCAL_BRANCH)\n> +\t\t\tkinds |= REF_DETACHED_HEAD;\n\nPerhaps add\n\n\t/* git branch --local also shows HEAD when it is detached */\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"266921","messageId":"vpqbnewxwcx.fsf@anie.imag.fr","threadId":"39952","inReplyTo":"1438066594-5620-7-git-send-email-Karthik.188@gmail.com","subject":"Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-07-28T13:09:02Z","receivedAt":"2015-07-28T13:09:02Z","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> We check if given ref is the current branch in print_ref_list(). Move\n> this check to print_ref_item() where it is checked right before\n> printing.\n\nThis means that the '*' and the different color are coded in C, hence\nit's not possible to mimick this using \"git for-each-ref --format ...\".\n\nI do not consider this as blocking, but I think the ultimate goal should\nbe to allow this, so that all the goodies of \"git branch\" can be made\navailable to other ref-listing commands.\n\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -534,9 +534,9 @@ static char *get_head_description(void)\n> }\n> \n> static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,\n> -\t\t\t int abbrev, int current, const char *remote_prefix)\n> +\t\t\t int abbrev, int detached, const char *remote_prefix)\n> {\n> -\tchar c;\n> +\tchar c = ' ';\n> \tint color;\n> \tstruct strbuf out = STRBUF_INIT, name = STRBUF_INIT;\n> \tconst char *prefix = \"\";\n> @@ -547,7 +547,11 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,\n> \n> \tswitch (item->kind) {\n> \tcase REF_LOCAL_BRANCH:\n> -\t\tcolor = BRANCH_COLOR_LOCAL;\n> +\t\tif (!detached && !strcmp(item->name, head)) {\n> +\t\t\tcolor = BRANCH_COLOR_CURRENT;\n> +\t\t\tc = '*';\n> +\t\t} else\n> +\t\t\tcolor = BRANCH_COLOR_LOCAL;\n> \t\tbreak;\n> \tcase REF_REMOTE_BRANCH:\n> \t\tcolor = BRANCH_COLOR_REMOTE;\n> @@ -556,18 +560,13 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,\n> \tcase REF_DETACHED_HEAD:\n> \t\tcolor = BRANCH_COLOR_CURRENT;\n> \t\tdesc = get_head_description();\n> +\t\tc = '*';\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\nI actually prefered the old way: current is a Boolean that says\nsemantically \"this is the current branch\", and the Boolean is turned\ninto presentation directives in a separate piece of code.\n\nI'd write\n\nchar c;\nint current = 0;\n\n...\n\nif (...)\n\tcurrent = 1;\n...\ncase REF_DETACHED_HEAD:\n\tcurrent = 1;\n...\n\nand keep the last hunk.\n\n(IOW, turn current from a parameter into a local variable)\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"266922","messageId":"vpqoaiwwgl2.fsf@anie.imag.fr","threadId":"39952","inReplyTo":"CAOLa=ZT3_DMJWFN62cbF19uxYBFsE69dGaFR=af1HPKsQ42otg@mail.gmail.com","subject":"Re: [RFC/PATCH] Port branch.c to use ref-filter APIs","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-07-28T13:35:05Z","receivedAt":"2015-07-28T13:35:05Z","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 version also doesn't use the printing options provided by\n> branch.c.\n\nDo you mean \"provided by ref-filter.{c,h}\"?\n\n> I wanted to discuss how exactly to go about that, because in branch.c,\n> we might need to change the --format based on attributes such as\n> abbrev, verbose and so on. But ref-filter expects us to verify the\n> format before filtering.\n\nI took time to understand the problem, but here's my understanding:\n\nref-filter expects the code to look like\n\n\tformat = ...;\n\tverify_ref_format(format);\n\tfilter_refs(...);\n\tfor (...)\n\t\tshow_ref_array_item(..., format, ...);\n\nand in the case of \"git branch -v\", you need to align the sha1s based on\nthe longest branch name, i.e. use %(padright:N+1) where N is the longest\nbranch name. And you can get N only after calling filter_refs, while you\nneed it for verify_ref_format().\n\nIs my understanding correct?\n\nIf so, what prevents swapping the order of verify_ref_format and\nfilter_refs? I understand that verify_ref_format() builds used_atom and\nother data-structures, hence it has to be called before\nshow_ref_array_item() and before sorting, but I don't think you need it\nbefore filter_refs (I may have missed something though).\n\nSo, the code could look like:\n\nfilter_refs(...)\nif (--format is given)\n\tformat = the argument of --format\nelse\n\tformat = STRBUF_INIT;\n\tstrbuf_add(&format, \"%(some_directive_to_display '*' if needed)\");\n\tif (verbose)\n\t\tstrbuf_addf(&format, \"%(padright:%d)\", max_width);\n\t...\nverify_ref_format(format);\nref_array_sort(...);\nfor (...)\n\tshow_ref_array_item(...);\n\n(BTW, a trivial helper function to display the whole ref_array could\nhelp. It would avoid having each caller write the 'for' loop)\n\nIdeally, you could also have a modifier atom %(alignleft) that would\nnot need an argument and that would go through the ref_array to find the\nmaxwidth, and do the alignment. That would give even more flexibility to\nthe end users of \"for-each-ref --format\".\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"266923","messageId":"vpqio94wgf9.fsf@anie.imag.fr","threadId":"39952","inReplyTo":"1438066594-5620-3-git-send-email-Karthik.188@gmail.com","subject":"Re: [RFC/PATCH 03/11] ref-filter: add option to filter only branches","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-07-28T13:38:34Z","receivedAt":"2015-07-28T13:38:34Z","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> +static int filter_branch_kind(struct ref_filter *filter, const char *refname)\n> +{\n> +\tint kind, i;\n> +\n> +\tstatic struct {\n> +\t\tint kind;\n> +\t\tconst char *prefix;\n> +\t} ref_kind[] = {\n> +\t\t{ REF_LOCAL_BRANCH, \"refs/heads/\" },\n> +\t\t{ REF_REMOTE_BRANCH, \"refs/remotes/\" },\n> +\t};\n\nNit: I would swap the order of fields, to make it a bit clearer that\nthis is a kind of dictionary key -> value (I think it's more common to\nwrite it in this order than value <- key).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"266924","messageId":"vpqa8ugwfyf.fsf@anie.imag.fr","threadId":"39952","inReplyTo":"CAP8UFD3L4QEYQMCda7AbByXGZcvgy5r9y0LMBqEysb6AZ+ydOQ@mail.gmail.com","subject":"Re: [RFC/PATCH 09/11] branch.c: use 'ref-filter' data structures","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-07-28T13:48:40Z","receivedAt":"2015-07-28T13:48:40Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Christian Couder writes:\n\n> On Tue, Jul 28, 2015 at 8:56 AM, Karthik Nayak wrote:\n>\n>> +static void ref_array_append(struct ref_array *array, const char *refname)\n>> +{\n>> + size_t len = strlen(refname);\n>> + struct ref_array_item *ref = xcalloc(1, sizeof(struct ref_array_item) + len + 1);\n>> + memcpy(ref->refname, refname, len);\n>> + ref->refname[len] = '\\0';\n\nThis looks very much like new_ref_array_item, except that the later also\ntakes an objectname parameter. I find it suspicious that you leave the\nobjectname field uninitialized.\n\nWhy is this code not calling new_ref_array_item?\n\nA detail: you could return a pointer to the newly allocated object to\nwrite\n\n\titem = ref_array_append(array, refname);\n\ninstead of\n\n\tref_array_append(array, refname);\n\titem = array->items[array->nr - 1];\n\n>> + REALLOC_ARRAY(array->items, array->nr + 1);\n>> + array->items[array->nr++] = ref;\n>> +}\n>\n> This function belongs more to ref-filter.{c,h}...\n\nThe function disapears in the next commit, but I also think that this\nfunction deserves to exist in ref-filter.{c,h} and remain after the end\nof the series.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"266925","messageId":"vpqh9oov017.fsf@anie.imag.fr","threadId":"39952","inReplyTo":"1438067468-6835-1-git-send-email-Karthik.188@gmail.com","subject":"Re: [RFC/PATCH 10/11] branch.c: use 'ref-filter' APIs","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-07-28T14:17:56Z","receivedAt":"2015-07-28T14:17:56Z","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> @@ -458,7 +345,7 @@ static void add_verbose_info(struct strbuf *out, struct ref_array_item *item,\n> \t}\n> \n> \tif (item->kind == REF_LOCAL_BRANCH)\n> -\t\tfill_tracking_info(&stat, item->refname, filter->verbose > 1);\n> +\t\tfill_tracking_info(&stat, refname, filter->verbose > 1);\n\nWhy can't you continue using item->refname?\n\n(It's a real question)\n\n> @@ -635,14 +495,21 @@ static void print_ref_list(struct ref_filter *filter)\n> \t/* Print detached heads before sorting and printing the rest */\n> \tif (filter->detached) {\n> \t\tprint_ref_item(array.items[index - 1], maxwidth, filter, remote_prefix);\n> -\t\tindex -= 1;\n> +\t\tarray.nr--;\n> \t}\n> \n> -\tqsort(array.items, index, sizeof(struct ref_array_item *), ref_cmp);\n> +\tif (!sorting) {\n> +\t\tdef_sorting.next = NULL;\n> +\t\tdef_sorting.atom = parse_ref_filter_atom(sort_type,\n> +\t\t\t\t\t\t\t sort_type + strlen(sort_type));\n> +\t\tsorting = &def_sorting;\n> +\t}\n> +\tref_array_sort(sorting, &array);\n\nDoes this belong to print_ref_list()? Is it not possible to extract it\nto get a code closer to the simple:\n\n\tfilter_refs(...);\n\tref_array_sort(...);\n\tprint_ref_list(...);\n\n?\n\n> -\tfor (i = 0; i < index; i++)\n> +\tfor (i = 0; i < array.nr; i++)\n> \t\tprint_ref_item(array.items[i], maxwidth, filter, remote_prefix);\n\nNow that we have show_ref_array_item, it may make sense to rename\nprint_ref_item to something that make the difference between these\nfunctions more explicit. Well, ideally, you'd get rid of it an actually\nuse show_ref_array_item, but if you are to keep it, maybe\nprint_ref_item_default_branch_format (or something shorter)?\n\n> --- a/ref-filter.h\n> +++ b/ref-filter.h\n> @@ -49,7 +49,6 @@ struct ref_sorting {\n> struct ref_array_item {\n> \tunsigned char objectname[20];\n> \tint flag, kind;\n> -\tint ignore : 1;\n\nYou should explain why you needed it and why you don't need it anymore\n(I guess, because it was used to implement --merge and you now get it\nfrom ref-filter).\n\n> --- a/t/t1430-bad-ref-name.sh\n> +++ b/t/t1430-bad-ref-name.sh\n> @@ -38,7 +38,7 @@ test_expect_success 'fast-import: fail on invalid branch name \"bad[branch]name\"'\n> \ttest_must_fail git fast-import '\n> \n> -test_expect_success 'git branch shows badly named ref' '\n> +test_expect_failure 'git branch does not show badly named ref' '\n\nI'm not sure what's the convention, but I think the test description\nshould give the expected behavior even with test_expect_failure.\n\nAnd please help the reviewers by saying what's the status wrt this test\n(any plan on how to fix it?).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"266929","messageId":"xmqqzj2gffth.fsf@gitster.dls.corp.google.com","threadId":"39952","inReplyTo":"1438066594-5620-1-git-send-email-Karthik.188@gmail.com","subject":"Re: [RFC/PATCH 01/11] ref-filter: add \"%(objectname:size=X)\" option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-28T15:43:38Z","receivedAt":"2015-07-28T15:43:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> From: Karthik Nayak \n>\n> Add support for %(objectname:size=X) where X is a number.\n> This will print the first X characters of an objectname.\n> The minimum value for X is 5. Hence any value lesser than\n> 5 will default to 5 characters.\n\nIs the reason why you do not call this \"abbrev\" because you are\nallowing it to truncate to a non-unique prefix?\n\nIf so, wouldn't it make more sense to treat this kind of string\nfunction in a way very similar to your earlier \"padright\"? I.e.\n\"%(maxwidth:5)%(objectname)\" or something?\n\nIf not, and if this is really an abbrev, then it makes sense to make\nit specific to the objectname, e.g. \"%(objectname:abbrev=7)\".\n"},{"id":"266930","messageId":"CAOLa=ZTDsYDbyvCfC0Ad1uZ+5wPeFusCoa0JuzOcYS2qB8Wbjg@mail.gmail.com","threadId":"39952","inReplyTo":"vpqoaiwwgl2.fsf@anie.imag.fr","subject":"Re: [RFC/PATCH] Port branch.c to use ref-filter APIs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-28T15:48:06Z","receivedAt":"2015-07-28T15:48:06Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Jul 28, 2015 at 7:05 PM, Matthieu Moy\n wrote:\n> Karthik Nayak writes:\n>\n>> This version also doesn't use the printing options provided by\n>> branch.c.\n>\n> Do you mean \"provided by ref-filter.{c,h}\"?\n>\n\nYes! my bad.\n\n>> I wanted to discuss how exactly to go about that, because in branch.c,\n>> we might need to change the --format based on attributes such as\n>> abbrev, verbose and so on. But ref-filter expects us to verify the\n>> format before filtering.\n>\n> I took time to understand the problem, but here's my understanding:\n>\n> ref-filter expects the code to look like\n>\n> format = ...;\n> verify_ref_format(format);\n> filter_refs(...);\n> for (...)\n> show_ref_array_item(..., format, ...);\n>\n> and in the case of \"git branch -v\", you need to align the sha1s based on\n> the longest branch name, i.e. use %(padright:N+1) where N is the longest\n> branch name. And you can get N only after calling filter_refs, while you\n> need it for verify_ref_format().\n>\n> Is my understanding correct?\n\nAbsolutely correct :)\n\n>\n> If so, what prevents swapping the order of verify_ref_format and\n> filter_refs? I understand that verify_ref_format() builds used_atom and\n> other data-structures, hence it has to be called before\n> show_ref_array_item() and before sorting, but I don't think you need it\n> before filter_refs (I may have missed something though).\n>\n\nNope! This is exactly what I'm trying :D\n\n> So, the code could look like:\n>\n> filter_refs(...)\n> if (--format is given)\n> format = the argument of --format\n> else\n> format = STRBUF_INIT;\n> strbuf_add(&format, \"%(some_directive_to_display '*' if needed)\");\n> if (verbose)\n> strbuf_addf(&format, \"%(padright:%d)\", max_width);\n> ...\n> verify_ref_format(format);\n> ref_array_sort(...);\n> for (...)\n> show_ref_array_item(...);\n>\n> (BTW, a trivial helper function to display the whole ref_array could\n> help. It would avoid having each caller write the 'for' loop)\n>\n\nThis I gotta try! Have been just keeping the flow the same and trying to mess\naround with how formatting works instead.\n\n> Ideally, you could also have a modifier atom %(alignleft) that would\n> not need an argument and that would go through the ref_array to find the\n> maxwidth, and do the alignment. That would give even more flexibility to\n> the end users of \"for-each-ref --format\".\n\nThis could work for refname, as each ref_array_item holds the refname,\nBut other atoms are only filled in after a call to verify_ref_format().\nWhat we could do would be after filling in all atom values, have a loop\nthrough all items with their atom values and maybe implement this.\nBut I don't see this as priority for now, so will mark it off for later.\nThanks\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"266931","messageId":"CAOLa=ZSWb6CKp2M6QRkSmdyf6XNJ_9kYTOuPHiw2_cU6JU3MHQ@mail.gmail.com","threadId":"39952","inReplyTo":"vpqio941xn5.fsf@anie.imag.fr","subject":"Re: [RFC/PATCH 01/11] ref-filter: add \"%(objectname:size=X)\" option","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-28T15:54:09Z","receivedAt":"2015-07-28T15:54:09Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Jul 28, 2015 at 2:12 PM, Matthieu Moy\n wrote:\n> Karthik Nayak writes:\n>\n>> + if (skip_prefix(name, \"objectname:size=\", &p)) {\n>> + unsigned int size = atoi(p);\n>\n> You have the same problem as for tag.c: don't use atoi, and make\n> accurate error checking (absence of value, negative value, non-integer\n> value).\n>\n> If you have other higher-priorities task, leave a temporary comment\n> /* FIXME: ... */ or /* TODO: ... */ and make sure you have no such\n> comment when you drop the \"RFC\" in the subject of your emails.\n>\n\nIt's a small change, will fix it with the drop in RFC :)\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"266932","messageId":"CAOLa=ZS4jeDXy35B=cLin-3e-owfV1g04Z6=kE1Ovjuh-H1qhA@mail.gmail.com","threadId":"39952","inReplyTo":"xmqqzj2gffth.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC/PATCH 01/11] ref-filter: add \"%(objectname:size=X)\" option","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-28T15:55:14Z","receivedAt":"2015-07-28T15:55:14Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Jul 28, 2015 at 9:13 PM, Junio C Hamano wrote:\n> Karthik Nayak writes:\n>\n>> From: Karthik Nayak \n>>\n>> Add support for %(objectname:size=X) where X is a number.\n>> This will print the first X characters of an objectname.\n>> The minimum value for X is 5. Hence any value lesser than\n>> 5 will default to 5 characters.\n>\n> Is the reason why you do not call this \"abbrev\" because you are\n> allowing it to truncate to a non-unique prefix?\n>\n> If so, wouldn't it make more sense to treat this kind of string\n> function in a way very similar to your earlier \"padright\"? I.e.\n> \"%(maxwidth:5)%(objectname)\" or something?\n>\n> If not, and if this is really an abbrev, then it makes sense to make\n> it specific to the objectname, e.g. \"%(objectname:abbrev=7)\".\n\nIt is finding unique abbrev, I just named size as it was smaller. But\nabbrev seems better, will rename, thanks :)\n\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"266933","messageId":"CAOLa=ZSDQLWF=ZsSeJ0rCtqTxh23bKSSSRfj9mGYaVJiMYbj8w@mail.gmail.com","threadId":"39952","inReplyTo":"vpqd1zc1xik.fsf@anie.imag.fr","subject":"Re: [RFC/PATCH 02/11] ref-filter: add 'colornext' atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-28T16:03:11Z","receivedAt":"2015-07-28T16:03:11Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Jul 28, 2015 at 2:15 PM, Matthieu Moy\n wrote:\n> Karthik Nayak writes:\n>\n>> --- a/t/t6302-for-each-ref-filter.sh\n>> +++ b/t/t6302-for-each-ref-filter.sh\n>> @@ -133,4 +133,20 @@ test_expect_success 'reverse version sort' '\n>> test_cmp expect actual\n>> '\n>>\n>> +get_color ()\n>> +{\n>> + git config --get-color no.such.slot \"$1\"\n>> +}\n>> +\n>> +cat >expect <> +$(get_color green)foo1.10$(get_color reset)||\n>> +$(get_color green)foo1.3$(get_color reset)||\n>> +$(get_color green)foo1.6$(get_color reset)||\n>> +EOF\n>> +\n>> +test_expect_success 'check `colornext` format option' '\n>> + git for-each-ref --format=\"%(colornext:green)%(refname:short)||\" | grep \"foo\" >actual &&\n>> + test_cmp expect actual\n>> +'\n>\n> This is not a very good test: you're not checking that colornext applies\n> to the next and only this one. Similarly to what I suggested for\n> padright, I'd suggest\n>\n> --format=\"%(refname:short)%(colornext:green)|%(refname:short)|%(refname:short)|\"\n>\n\nThat was the purpose of the \"||\" but that doesn't check the color of next atom,\nThanks for the example will use that :)\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"266934","messageId":"CAOLa=ZSz8xpDD5LJ3AW6xszTJxCU8EK71V2LJcymB14-ebhnZQ@mail.gmail.com","threadId":"39952","inReplyTo":"CAP8UFD2x5rz18hUXmJVWw81EMEVZc5Hm=_GUwfuc6Fo--8Ukfw@mail.gmail.com","subject":"Re: [RFC/PATCH 02/11] ref-filter: add 'colornext' atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-28T16:04:06Z","receivedAt":"2015-07-28T16:04:06Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Jul 28, 2015 at 2:43 PM, Christian Couder\n wrote:\n> On Tue, Jul 28, 2015 at 8:56 AM, Karthik Nayak wrote:\n>> @@ -712,6 +713,15 @@ static void populate_value(struct ref_array_item *ref)\n>> v->modifier_atom = 1;\n>> v->pad_to_right = 1;\n>> continue;\n>> + } else if (starts_with(name, \"colornext:\")) {\n>> + char color[COLOR_MAXLEN] = \"\";\n>> +\n>> + if (color_parse(name + 10, color) < 0)\n>> + die(_(\"unable to parse format\"));\n>\n> Maybe use skip_prefix(), and die() with a more helpful message.\n>\n\nOkay will do. Thanks!\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"266935","messageId":"xmqqvbd4fe5w.fsf@gitster.dls.corp.google.com","threadId":"39952","inReplyTo":"1438066594-5620-1-git-send-email-Karthik.188@gmail.com","subject":"Re: [RFC/PATCH 01/11] ref-filter: add \"%(objectname:size=X)\" option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-28T16:19:23Z","receivedAt":"2015-07-28T16:19:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> From: Karthik Nayak \n>\n> Add support for %(objectname:size=X) where X is a number.\n> This will print the first X characters of an objectname.\n> The minimum value for X is 5. Hence any value lesser than\n> 5 will default to 5 characters.\n\nWhere does this hardcoded 5 come from?\n\nI'd agree that we would want some minimum for sanity (not safety),\nbut I do not think we want random callers of find-unique-abbrev\narbitrarily imposing their own minimum, making different codepaths\nbehave inconsistently without a good reason.\n\nIt seems that the minimum we use for sanity at the core level is\nMINIMUM_ABBREV. Is there a reason why that value is inappropriate\nfor this codepath?\n\n>\n> Mentored-by: Christian Couder \n> Mentored-by: Matthieu Moy \n> Signed-off-by: Karthik Nayak \n> ---\n> ref-filter.c | 9 +++++++++\n> 1 file changed, 9 insertions(+)\n>\n> diff --git a/ref-filter.c b/ref-filter.c\n> index 0a34924..4c5e3f8 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -196,6 +196,8 @@ 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> +\tconst char *p;\n> +\n> \tif (!strcmp(name, \"objectname\")) {\n> \t\tchar *s = xmalloc(41);\n> \t\tstrcpy(s, sha1_to_hex(sha1));\n> @@ -206,6 +208,13 @@ static int grab_objectname(const char *name, const unsigned char *sha1,\n> \t\tv->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));\n> \t\treturn 1;\n> \t}\n> +\tif (skip_prefix(name, \"objectname:size=\", &p)) {\n> +\t\tunsigned int size = atoi(p);\n> +\t\tif (size < 5)\n> +\t\t\tsize = 5;\n> +\t\tv->s = xstrdup(find_unique_abbrev(sha1, size));\n> +\t\treturn 1;\n> +\t}\n> \treturn 0;\n> }\n"},{"id":"266940","messageId":"CAOLa=ZTkSDqgTYH1fmcGssSPbkbr8RN3zGMrbZPx9pWQCfs+9A@mail.gmail.com","threadId":"39952","inReplyTo":"vpqio94wgf9.fsf@anie.imag.fr","subject":"Re: [RFC/PATCH 03/11] ref-filter: add option to filter only branches","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-28T16:42:45Z","receivedAt":"2015-07-28T16:42:45Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Jul 28, 2015 at 7:08 PM, Matthieu Moy\n wrote:\n> Karthik Nayak writes:\n>\n>> +static int filter_branch_kind(struct ref_filter *filter, const char *refname)\n>> +{\n>> + int kind, i;\n>> +\n>> + static struct {\n>> + int kind;\n>> + const char *prefix;\n>> + } ref_kind[] = {\n>> + { REF_LOCAL_BRANCH, \"refs/heads/\" },\n>> + { REF_REMOTE_BRANCH, \"refs/remotes/\" },\n>> + };\n>\n> Nit: I would swap the order of fields, to make it a bit clearer that\n> this is a kind of dictionary key -> value (I think it's more common to\n> write it in this order than value <- key).\n>\n\nThis was borrowed from branch.c, but ok will change it!\nThanks\n\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"266943","messageId":"CAOLa=ZQhSEX47LGKAX4AuHqwtE3RKCzpLPJ3xxkiYt3aam0E3Q@mail.gmail.com","threadId":"39952","inReplyTo":"CA+P7+xq_jC=gE3J=PTkVx8BpTRzCJCfU0V-ydBve2FYbg1gmBg@mail.gmail.com","subject":"Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-28T16:47:40Z","receivedAt":"2015-07-28T16:47:40Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Jul 28, 2015 at 1:24 PM, Jacob Keller wrote:\n> On Mon, Jul 27, 2015 at 11:56 PM, Karthik Nayak wrote:\n>> The 'ifexists' atom allows us to print a required format if the\n>> preceeding atom has a value. If the preceeding atom has no value then\n>\n> Don't you mean \"following atom\" here? since you do document it as \"the\n> next atom\" below you should fix the commit message as well to match.\n> In any respect, this is a useful formatting atom :)\n>\n\nThat should have been `succeeding` atom. My bad! thanks :)\n\n> %(ifexists:[%s])%(atom) where the contents of atom will be placed into %s?\n>\n> I suggest documenting that the atom will be placed into the contents\n> of ifexists via the %s indicator, as you do show an example but don't\n> explicitely say %s is the formatting character.\n>\n\nYeah! I should have explicitly mentioned that, thanks!\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"266949","messageId":"CAOLa=ZQ7Tc=73BFS_p5Dqnjn-SWhfJ6fqKyCOf71Mrj=puTjOg@mail.gmail.com","threadId":"39952","inReplyTo":"vpq7fpk1x9a.fsf@anie.imag.fr","subject":"Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-28T17:39:10Z","receivedAt":"2015-07-28T17:39:10Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Jul 28, 2015 at 2:20 PM, Matthieu Moy\n wrote:\n> Karthik Nayak writes:\n>\n>> --- a/t/t6302-for-each-ref-filter.sh\n>> +++ b/t/t6302-for-each-ref-filter.sh\n>> @@ -149,4 +149,25 @@ test_expect_success 'check `colornext` format option' '\n>> test_cmp expect actual\n>> '\n>>\n>> +test_expect_success 'check `ifexists` format option' '\n>> + cat >expect <<-\\EOF &&\n>> + [foo1.10]\n>> + [foo1.3]\n>> + [foo1.6]\n>> + EOF\n>> + git for-each-ref --format=\"%(ifexists:[%s])%(refname:short)\" | grep \"foo\" >actual &&\n>> + test_cmp expect actual\n>> +'\n>\n> You're testing only the positive case of ifexists, right? You need a\n> test for the negative case (i.e. the attribute does not exist) too.\n> Ideally, check that the ifexist actually applies to the right attribute,\n> like\n>\n> --format '%(ifexist:<%s>)%(attribute1) %(ifexist:[%s])%(attribute2)'\n>\n> and manage to get an output like\n>\n> \n> [bar]\n> [barfoo]\n>\n\nYes! this seems like an important test, thanks :)\n\n>\n> You have a double || that is not useful. Not really harmful either, but\n> it may distract the reader. You may want to s/||/|/.\n>\n\nWill change!\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"266952","messageId":"vpqr3nsnp7o.fsf@anie.imag.fr","threadId":"39952","inReplyTo":"CAOLa=ZTDsYDbyvCfC0Ad1uZ+5wPeFusCoa0JuzOcYS2qB8Wbjg@mail.gmail.com","subject":"Re: [RFC/PATCH] Port branch.c to use ref-filter APIs","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-07-28T17:53:31Z","receivedAt":"2015-07-28T17:53:31Z","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 Tue, Jul 28, 2015 at 7:05 PM, Matthieu Moy\n> wrote:\n>\n>> Ideally, you could also have a modifier atom %(alignleft) [...]\n>\n> This could work for refname, as each ref_array_item holds the refname,\n> But other atoms are only filled in after a call to\n> verify_ref_format().\n\nIf you swap the order and call verify_ref_format() after filter_refs(),\nthen you could detect %(alignleft) and iterate over the list as needed\nin verify_ref_format().\n\n(You would actually force swapping the order and you would need to adapt\nother callers in for-each-ref and tag)\n\n> But I don't see this as priority for now, so will mark it off for later.\n\nAbsolutely, that's what I meant by \"Ideally\". I'm just thinking aloud.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"266953","messageId":"xmqq8ua0f9lk.fsf@gitster.dls.corp.google.com","threadId":"39952","inReplyTo":"1438066594-5620-4-git-send-email-Karthik.188@gmail.com","subject":"Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-28T17:57:59Z","receivedAt":"2015-07-28T17:57:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> The 'ifexists' atom allows us to print a required format if the\n> preceeding atom has a value. If the preceeding atom has no value then\n> the format given is not printed. e.g. to print \"[]\" we can\n> now use the format \"%(ifexists:[%s])%(refname)\".\n\nA handful of \"huh?\" on the design.\n\n - The atom says \"if *exists*\" and explanation says \"has a value\".\n How are they related? Does an atom whose value is an empty\n string has a value? Or is \"ifexists\" meant to be used only to\n ignore meaningless atom, e.g. %(*objectname) applied to a ref that\n refers to an object that is not an annotated tag?\n\n - That %s looks ugly. Are there cases where a user may want to say\n %(ifexists:[%i]) or something other than 's' after that per-cent?\n\n . Is it allowed to have more than one %s there?\n . Is it allowed to have no %s there?\n\n - The syntax makes the reader wonder if [] is part of the\n construct, or just an example of any arbitrary string, i.e. is\n \"%(ifexists:the %s can be part of arbitrary string)\" valid?\n\n - If an arbitrary string is allowed, is there any quoting mechanism\n to allow \")\" to be part of that arbitrary string?\n \n - What, if anything, is allowed to come between %(ifexists...) and\n the next atom like %(refname)? For example, are these valid\n constructs?\n\n . %(ifexists...)%(padright:20)%(refname)\n . %(ifexists...) %(refname) [%(subject)]\n\n - This syntax does not seem to allow switching on an attribute to\n show or not to show another, e.g. \"if %(*objectname) makes sense,\n then show '%(padright:20)%(refname:short) %(*subject)' for it\".\n"},{"id":"266961","messageId":"CAOLa=ZTsr3rFanRFwDW7YUE=VmcTgpaOHEKX2cJpfKOhW1FZGA@mail.gmail.com","threadId":"39952","inReplyTo":"vpq6154zk9o.fsf@anie.imag.fr","subject":"Re: [RFC/PATCH 05/11] branch: fix width computation","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-28T18:16:59Z","receivedAt":"2015-07-28T18:16:59Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Jul 28, 2015 at 3:17 PM, Matthieu Moy\n wrote:\n> Karthik Nayak writes:\n>\n>> From: Karthik Nayak \n>\n> Why did send-email add this From: header? Strange, it has the same\n> content as your actual From: field.\n>\n\nNot sure why, everything else came out fine. Idk what happened here.\n\n>> Remove unnecessary variables from ref_list and ref_item\n>> which were used for width computation. Make other changes\n>> accordingly. This patch is a precursor for porting branch.c\n>> to use ref-filter APIs.\n>\n> You can explain better why this is needed. I guess something like \"we're\n> making struct ref_item similar to ref-filter's ref_array_item\", but the\n> reader shouldn't have to guess.\n>\n\nWill explain like you suggested.\n\n> You should adujst the subject like BTW, I don't think you are \"fixing\"\n> anything.\n>\n\nI guess refactor would be a better word here.\n\n> On a side note: on the \"tag\" series, see how explaining better and\n> splitting patches led not only to a better history, but also to better\n> final code, and to finding a actual bugs (the %(color) thing and the\n> absence of reset on the state variable) even after sereval rounds of\n> review? I'm being picky and demanding, but don't see that as a complain\n> from me, just hints on getting even better ;-).\n>\n\nHaha, I look forward to reviews, they show things I usually miss out on,\nand help me get better. so keep them coming, I'll keep improving :)\nThanks\n\n>> @@ -386,15 +386,8 @@ static int append_ref(const char *refname, const struct object_id *oid, int flag\n>> newitem->name = xstrdup(refname);\n>> newitem->kind = kind;\n>> newitem->commit = commit;\n>> - newitem->width = utf8_strwidth(refname);\n>> newitem->dest = resolve_symref(orig_refname, prefix);\n>> newitem->ignore = 0;\n>> - /* adjust for \"remotes/\" */\n>> - if (newitem->kind == REF_REMOTE_BRANCH &&\n>> - ref_list->kinds != REF_REMOTE_BRANCH)\n>> - newitem->width += 8;\n>> - if (newitem->width > ref_list->maxwidth)\n>> - ref_list->maxwidth = newitem->width;\n>>\n>> return 0;\n>> }\n>\n> OK, in the old code, the width computation is done when inserting the\n> ref into the array. In the new code, you build the array and then do the\n> width computation. You can explain this better in the commit message I\n> think (instead of \"Make other changes accordingly\" which doesn't bring\n> much).\n\nOkay I guess will do, just didn't want to explain the whole thing in the commit\nmessage.\n\n>\n>> @@ -505,11 +498,12 @@ static void add_verbose_info(struct strbuf *out, struct ref_item *item,\n>> }\n>>\n>> static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,\n>> - int abbrev, int current, char *prefix)\n>> + int abbrev, int current, const char *remote_prefix)\n>> {\n>> char c;\n>> int color;\n>> struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;\n>> + const char *prefix = \"\";\n>>\n>> if (item->ignore)\n>> return;\n>> @@ -520,6 +514,7 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,\n>> break;\n>> case REF_REMOTE_BRANCH:\n>> color = BRANCH_COLOR_REMOTE;\n>> + prefix = remote_prefix;\n>> break;\n>\n> Why do you need these two hunks? I did not investigate in details, but\n> it seems you're calling print_ref_item either with prefix=\"\" or with\n> prefix=remote_prefix so it would seem that keeping \"prefix\" as argument\n> would work. I guess I missed something.\n\nThis is needed as whenever we do \"git branch\", show_detached() calls\nprint_ref_item()\nwith remote_prefix=\"\". But print_ref_list() calls print_ref_item()\nwith remote_prefix=\"remotes\"\n(only when `git branch -a` is called remote_prefix=\"\"). But only\nremote branches should be\ngiven the remotes prefix.\n\n>\n>> -static int calc_maxwidth(struct ref_list *refs)\n>> +static int calc_maxwidth(struct ref_list *refs, int remote_bonus)\n>> {\n>> - int i, w = 0;\n>> + int i, max = 0;\n>> for (i = 0; i < refs->index; i++) {\n>> + struct ref_item *it = &refs->list[i];\n>> + int w = utf8_strwidth(it->name);\n>> +\n>> if (refs->list[i].ignore)\n>> continue;\n>> - if (refs->list[i].width > w)\n>> - w = refs->list[i].width;\n>> + if (it->kind == REF_REMOTE_BRANCH)\n>> + w += remote_bonus;\n>> + if (w > max)\n>> + max = w;\n>> }\n>> - return w;\n>> + return max;\n>> }\n>\n> The old code was using 'w' for the max and no variable for the value at\n> the current iteration. You're using 'max' for the max and 'w' at the\n> current iteration. Using the same name 'w' for different things in the\n> pre- and post-image of the patch distracts the reader.\n>\n> It may make sense to s/w/max/ in a separate preparatory patch. Or maybe\n> it's overkill.\n>\n\nSince the change was minimal and easily understandable I think it's ok.\nBut if you still feel otherwise, I'll be happy to introduce a preparatory patch.\n\n>> @@ -600,21 +600,18 @@ static char *get_head_description(void)\n>> return strbuf_detach(&desc, NULL);\n>> }\n>>\n>> -static void show_detached(struct ref_list *ref_list)\n>> +static void show_detached(struct ref_list *ref_list, int maxwidth)\n>> {\n>> struct commit *head_commit = lookup_commit_reference_gently(head_sha1, 1);\n>>\n>> if (head_commit && is_descendant_of(head_commit, ref_list->with_commit)) {\n>> struct ref_item item;\n>> item.name = get_head_description();\n>> - item.width = utf8_strwidth(item.name);\n>> item.kind = REF_LOCAL_BRANCH;\n>> item.dest = NULL;\n>> item.commit = head_commit;\n>> item.ignore = 0;\n>> - if (item.width > ref_list->maxwidth)\n>> - ref_list->maxwidth = item.width;\n>> - print_ref_item(&item, ref_list->maxwidth, ref_list->verbose, ref_list->abbrev, 1, \"\");\n>> + print_ref_item(&item, maxwidth, ref_list->verbose, ref_list->abbrev, 1, \"\");\n>> free(item.name);\n>> }\n>> }\n> ...\n>> + int maxwidth = 0;\n> ...\n>> + if (verbose)\n>> + maxwidth = calc_maxwidth(&ref_list, strlen(remote_prefix));\n>>\n>> qsort(ref_list.list, ref_list.index, sizeof(struct ref_item), ref_cmp);\n>>\n>> detached = (detached && (kinds & REF_LOCAL_BRANCH));\n>> if (detached && match_patterns(pattern, \"HEAD\"))\n>> - show_detached(&ref_list);\n>> + show_detached(&ref_list, maxwidth);\n>\n> This hunks could ideally go in a preparatory patch that would just move\n> the place where maxwidth is computed. This preparatory patch would just\n> say\n>\n> maxwidth = ref_list->maxwidth;\n>\n> and then you would do the actual change to\n>\n> if (verbose)\n> maxwidth = calc_maxwidth(...)\n>\n> without the distracting changes in the function's argument list.\n>\n> I won't insist on that, again it may be overkill. But reading the patch,\n> I found it both rather trivial and hard to read, so there's room for\n> improvement.\n>\n\nI find it too small to make a preparatory patch again, but if you really feel\nso, like I said, I'll change :)\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"266969","messageId":"CAOLa=ZR4DSzdnR8+3eiVTCX92aeFu2o6=iDm7QpdOewizSuxng@mail.gmail.com","threadId":"39952","inReplyTo":"vpqlhe0xwpv.fsf@anie.imag.fr","subject":"Re: [RFC/PATCH 06/11] branch: roll show_detached HEAD into regular ref_list","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-28T19:19:51Z","receivedAt":"2015-07-28T19:19:51Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Jul 28, 2015 at 6:31 PM, Matthieu Moy\n wrote:\n> Karthik Nayak writes:\n>\n>> Remove show_detached() and make detached HEAD to be rolled into\n>> regular ref_list by adding REF_DETACHED_HEAD as a kind of branch and\n>> supporting the same in append_ref().\n>\n> Again, this lacks the \"why?\" explanation.\n\nWill include a small explanation.\n\n>\n>> Bump get_head_description() to the top.\n>\n> Here also: why? And this could easily go to a separate patch to let the\n> reviewer focus on actual changes.\n\nI'll move this to a preparatory patch.\n\n>\n> I think you're leaking a string here: get_head_description() builds an\n> strbuf and returns the dynamically allocated string, which you never\n> free.\n>\n\nAh! good catch, will fix that.\n\n>> -static void show_detached(struct ref_list *ref_list, int maxwidth)\n>> -{\n>> - struct commit *head_commit = lookup_commit_reference_gently(head_sha1, 1);\n>> -\n>> - if (head_commit && is_descendant_of(head_commit, ref_list->with_commit)) {\n>\n> I'm not sure what this if was doing, and why you can get rid of it. My\n> understanding is that with_commit comes from --contains, and in the\n> previous code the filtering was done at display time (detached HEAD was\n> not shown if it was not contained in commits specified with --contains).\n>\n> Eventually, you'll use ref-filter to do this filtering so you won't need\n> this check at display time.\n>\n> But am I correct that for a few commits, you ignore --contains on\n> detached HEAD?\n>\n\nNo we don't ignore --contains on detached HEAD.\n\nSince detached HEAD now gets its data from append_ref(). The function\nalso checks for the --contains option.\n\n>> @@ -678,15 +674,20 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru\n>> if (verbose)\n>> maxwidth = calc_maxwidth(&ref_list, strlen(remote_prefix));\n>>\n>> - qsort(ref_list.list, ref_list.index, sizeof(struct ref_item), ref_cmp);\n>> + index = ref_list.index;\n>> +\n>> + /* Print detached heads before sorting and printing the rest */\n>\n> I think you mean head (no s) or HEAD. It's not Mercurial ;-).\n>\n\nI meant HEAD, lol.\n\n>> + if (detached && (ref_list.list[index - 1].kind == REF_DETACHED_HEAD) &&\n>> + !strcmp(ref_list.list[index - 1].name, head)) {\n>> + print_ref_item(&ref_list.list[index - 1], maxwidth, verbose, abbrev,\n>> + 1, remote_prefix);\n>> + index -= 1;\n>> + }\n>\n> This relies on the assumption that HEAD has to be the last in the array.\n> The assumption is correct since you call head_ref(append_ref, &cb) after\n> for_each_rawref(append_ref, &cb). I think this deserves a comment to\n> remind the reader that HEAD is always the last (here or at the call to\n> head_ref to make sure nobody try to change the order between head_ref\n> and for_each_rawref).\n>\n\nYeah! makes sense, will add at the comment at the call to head_ref().\n\n> It may be more natural to do it the other way around: call head_ref\n> first and get HEAD first, as you are going to display it first (but in\n> any case, you'll have to call qsort on a subset of the array so it\n> doesn't change much).\n\nThat sorta messes things up with qsort, this keeps it clean I feel.\n\n>\n>> @@ -913,7 +914,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n>> die(_(\"branch name required\"));\n>> return delete_branches(argc, argv, delete > 1, kinds, quiet);\n>> } else if (list) {\n>> - int ret = print_ref_list(kinds, detached, verbose, abbrev,\n>> + int ret;\n>> + if (kinds & REF_LOCAL_BRANCH)\n>> + kinds |= REF_DETACHED_HEAD;\n>\n> Perhaps add\n>\n> /* git branch --local also shows HEAD when it is detached */\n>\n\nYeah definitely!\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"266987","messageId":"CAOLa=ZTy+KX4K_inpDoMy+thzFdF2RF93yMEHJUExzT4OvjPww@mail.gmail.com","threadId":"39952","inReplyTo":"vpqbnewxwcx.fsf@anie.imag.fr","subject":"Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-28T20:12:10Z","receivedAt":"2015-07-28T20:12:10Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Jul 28, 2015 at 6:39 PM, Matthieu Moy\n wrote:\n> Karthik Nayak writes:\n>\n>> We check if given ref is the current branch in print_ref_list(). Move\n>> this check to print_ref_item() where it is checked right before\n>> printing.\n>\n> This means that the '*' and the different color are coded in C, hence\n> it's not possible to mimick this using \"git for-each-ref --format ...\".\n>\n> I do not consider this as blocking, but I think the ultimate goal should\n> be to allow this, so that all the goodies of \"git branch\" can be made\n> available to other ref-listing commands.\n>\n\nNot sure what you mean here.\n\n>> --- a/builtin/branch.c\n>> +++ b/builtin/branch.c\n>> @@ -534,9 +534,9 @@ static char *get_head_description(void)\n>> }\n>>\n>> static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,\n>> - int abbrev, int current, const char *remote_prefix)\n>> + int abbrev, int detached, const char *remote_prefix)\n>> {\n>> - char c;\n>> + char c = ' ';\n>> int color;\n>> struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;\n>> const char *prefix = \"\";\n>> @@ -547,7 +547,11 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,\n>>\n>> switch (item->kind) {\n>> case REF_LOCAL_BRANCH:\n>> - color = BRANCH_COLOR_LOCAL;\n>> + if (!detached && !strcmp(item->name, head)) {\n>> + color = BRANCH_COLOR_CURRENT;\n>> + c = '*';\n>> + } else\n>> + color = BRANCH_COLOR_LOCAL;\n>> break;\n>> case REF_REMOTE_BRANCH:\n>> color = BRANCH_COLOR_REMOTE;\n>> @@ -556,18 +560,13 @@ static void print_ref_item(struct ref_item *item, int maxwidth, int verbose,\n>> case REF_DETACHED_HEAD:\n>> color = BRANCH_COLOR_CURRENT;\n>> desc = get_head_description();\n>> + c = '*';\n>> break;\n>> default:\n>> color = BRANCH_COLOR_PLAIN;\n>> break;\n>> }\n>>\n>> - c = ' ';\n>> - if (current) {\n>> - c = '*';\n>> - color = BRANCH_COLOR_CURRENT;\n>> - }\n>\n> I actually prefered the old way: current is a Boolean that says\n> semantically \"this is the current branch\", and the Boolean is turned\n> into presentation directives in a separate piece of code.\n>\n> I'd write\n>\n> char c;\n> int current = 0;\n>\n> ...\n>\n> if (...)\n> current = 1;\n> ...\n> case REF_DETACHED_HEAD:\n> current = 1;\n> ...\n>\n> and keep the last hunk.\n>\n> (IOW, turn current from a parameter into a local variable)\n>\n\nThanks will do this.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"266991","messageId":"CAOLa=ZTcMJ00WGoKwzrexmZGyaVG==vRHEZ7di+y0E2YKCO1ag@mail.gmail.com","threadId":"39952","inReplyTo":"CAP8UFD1Bx_o2gWP0R4EbsHO0d3LRRJ2rpHDKQhRVQpTrD6pjBw@mail.gmail.com","subject":"Re: [RFC/PATCH 09/11] branch.c: use 'ref-filter' data structures","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-28T20:31:05Z","receivedAt":"2015-07-28T20:31:05Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Jul 28, 2015 at 1:52 PM, Christian Couder\n wrote:\n> On Tue, Jul 28, 2015 at 8:56 AM, Karthik Nayak wrote:\n>> diff --git a/ref-filter.h b/ref-filter.h\n>> index 7d1871d..3458595 100644\n>> --- a/ref-filter.h\n>> +++ b/ref-filter.h\n>> @@ -5,6 +5,7 @@\n>> #include \"refs.h\"\n>> #include \"commit.h\"\n>> #include \"parse-options.h\"\n>> +#include \"revision.h\"\n>>\n>> /* Quoting styles */\n>> #define QUOTE_NONE 0\n>> @@ -48,6 +49,7 @@ struct ref_sorting {\n>> struct ref_array_item {\n>> unsigned char objectname[20];\n>> int flag, kind;\n>> + int ignore : 1;\n>\n> Maybe you could add a comment to say that this will be removed in the\n> next patch.\n>\n\nYes, Will do. Thanks :)\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"266993","messageId":"CAOLa=ZSD-sz5Ed9xGqORo5teBuqzSKSeZS-2n06RsuGSZKUa4A@mail.gmail.com","threadId":"39952","inReplyTo":"CAP8UFD3L4QEYQMCda7AbByXGZcvgy5r9y0LMBqEysb6AZ+ydOQ@mail.gmail.com","subject":"Re: [RFC/PATCH 09/11] branch.c: use 'ref-filter' data structures","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-28T20:38:03Z","receivedAt":"2015-07-28T20:38:03Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Jul 28, 2015 at 1:47 PM, Christian Couder\n wrote:\n> On Tue, Jul 28, 2015 at 8:56 AM, Karthik Nayak wrote:\n>\n>> +static void ref_array_append(struct ref_array *array, const char *refname)\n>> +{\n>> + size_t len = strlen(refname);\n>> + struct ref_array_item *ref = xcalloc(1, sizeof(struct ref_array_item) + len + 1);\n>> + memcpy(ref->refname, refname, len);\n>> + ref->refname[len] = '\\0';\n>> + REALLOC_ARRAY(array->items, array->nr + 1);\n>> + array->items[array->nr++] = ref;\n>> +}\n>\n> This function belongs more to ref-filter.{c,h}...\n>\n\nIts a temporary function which is removed in the next patch.\n\n>\n>> - ALLOC_GROW(ref_list->list, ref_list->index + 1, ref_list->alloc);\n>> + ref_array_append(array, refname);\n>> + item = array->items[array->nr - 1];\n>\n> ...and the above is a bit ugly.\n\nWill fix that :)\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"266994","messageId":"CAOLa=ZTXSutXd4HCrXoVoLT0kQzppiC7gUJpnk8DYYCo7M5XNw@mail.gmail.com","threadId":"39952","inReplyTo":"vpqa8ugwfyf.fsf@anie.imag.fr","subject":"Re: [RFC/PATCH 09/11] branch.c: use 'ref-filter' data structures","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-28T20:41:11Z","receivedAt":"2015-07-28T20:41:11Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Jul 28, 2015 at 7:18 PM, Matthieu Moy\n wrote:\n> Christian Couder writes:\n>\n>> On Tue, Jul 28, 2015 at 8:56 AM, Karthik Nayak wrote:\n>>\n>>> +static void ref_array_append(struct ref_array *array, const char *refname)\n>>> +{\n>>> + size_t len = strlen(refname);\n>>> + struct ref_array_item *ref = xcalloc(1, sizeof(struct ref_array_item) + len + 1);\n>>> + memcpy(ref->refname, refname, len);\n>>> + ref->refname[len] = '\\0';\n>\n> This looks very much like new_ref_array_item, except that the later also\n> takes an objectname parameter. I find it suspicious that you leave the\n> objectname field uninitialized.\n>\n\nWell the objectname is not required here, the idea is to retain the way branch.c\nworks.\n\n> Why is this code not calling new_ref_array_item?\n>\n\nBecause no objectname is there.\n\n> A detail: you could return a pointer to the newly allocated object to\n> write\n>\n> item = ref_array_append(array, refname);\n>\n> instead of\n>\n> ref_array_append(array, refname);\n> item = array->items[array->nr - 1];\n>\n\nYeah doing that :)\n\n>>> + REALLOC_ARRAY(array->items, array->nr + 1);\n>>> + array->items[array->nr++] = ref;\n>>> +}\n>>\n>> This function belongs more to ref-filter.{c,h}...\n>\n> The function disapears in the next commit, but I also think that this\n> function deserves to exist in ref-filter.{c,h} and remain after the end\n> of the series.\n>\n\nWhy though? I don't see the need when new_ref_array_item() is present.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"267020","messageId":"CA+P7+xrGjeXPg3Fbp3A3gJbFeK2E-CP0uXQHWBb3NbNVUHUe2g@mail.gmail.com","threadId":"39952","inReplyTo":"CAOLa=ZTy+KX4K_inpDoMy+thzFdF2RF93yMEHJUExzT4OvjPww@mail.gmail.com","subject":"Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-07-29T00:46:51Z","receivedAt":"2015-07-29T00:46:51Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, Jul 28, 2015 at 1:12 PM, Karthik Nayak wrote:\n> On Tue, Jul 28, 2015 at 6:39 PM, Matthieu Moy\n> wrote:\n>> Karthik Nayak writes:\n>>\n>>> We check if given ref is the current branch in print_ref_list(). Move\n>>> this check to print_ref_item() where it is checked right before\n>>> printing.\n>>\n>> This means that the '*' and the different color are coded in C, hence\n>> it's not possible to mimick this using \"git for-each-ref --format ...\".\n>>\n>> I do not consider this as blocking, but I think the ultimate goal should\n>> be to allow this, so that all the goodies of \"git branch\" can be made\n>> available to other ref-listing commands.\n>>\n>\n> Not sure what you mean here.\n>\n\nHe means to make sure that any feature that was in tag, branch,\nfor-each-ref, etc should be available as part of ref-filter and in all\nof them\n\nMaybe he misunderstood code or maybe you misunderstood the comment here?\n\nRegards,\nJake\n"},{"id":"267030","messageId":"CAOLa=ZS+UoPG_50-QBNUCfW03tXZwVE3Tabk15O=Q=2+SV_GzA@mail.gmail.com","threadId":"39952","inReplyTo":"CA+P7+xpU4KdmutkGDdiAaXFpzs4zq=f2rXhqxqogAPJqopjdRQ@mail.gmail.com","subject":"Re: [RFC/PATCH 10/11] branch.c: use 'ref-filter' APIs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-29T03:46:40Z","receivedAt":"2015-07-29T03:46:40Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Jul 28, 2015 at 1:27 PM, Jacob Keller wrote:\n> On Tue, Jul 28, 2015 at 12:11 AM, Karthik Nayak wrote:\n>> Make 'branch.c' use 'ref-filter' APIs for iterating through refs\n>> sorting. This removes most of the code used in 'branch.c' replacing it\n>> with calls to the 'ref-filter' library.\n>>\n>> Make 'tag.c' use the 'filter_refs()' function provided by 'ref-filter'\n>> to filter out tags based on the options set.\n>>\n>\n> This patch doesn't do anything to tag.c, so you should update the\n> commit message to remove this or replace it with s/tag/branch if it\n> was intended to be about branch.c?\n>\n> Regards,\n\nCopy-paste error, thanks for pointing out.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"267031","messageId":"CAOLa=ZQee46Z+hh3E13QngGv-BJePHkUQ1bHf7JChqOTGDEVDA@mail.gmail.com","threadId":"39952","inReplyTo":"CAP8UFD1q5oxchNUhWc8331nAq-eVkzVOe9=56v68HcY2CDXW4A@mail.gmail.com","subject":"Re: [RFC/PATCH 10/11] branch.c: use 'ref-filter' APIs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-29T03:48:02Z","receivedAt":"2015-07-29T03:48:02Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Jul 28, 2015 at 1:39 PM, Christian Couder\n wrote:\n> On Tue, Jul 28, 2015 at 9:11 AM, Karthik Nayak wrote:\n>\n>> diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt\n>> index a67138a..897cd81 100644\n>> --- a/Documentation/git-branch.txt\n>> +++ b/Documentation/git-branch.txt\n>> @@ -11,7 +11,7 @@ SYNOPSIS\n>> 'git branch' [--color[=] | --no-color] [-r | -a]\n>> [--list] [-v [--abbrev= | --no-abbrev]]\n>> [--column[=] | --no-column]\n>> - [(--merged | --no-merged | --contains) []] [...]\n>> + [(--merged | --no-merged | --contains) []] [--sort=] [...]\n>\n> Maybe: [(--[no-]merged | --contains) []]\n\nThats an existing string. So not sure if this patch is the right place to\nmake this change.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"267036","messageId":"vpqwpxjmgnc.fsf@anie.imag.fr","threadId":"39952","inReplyTo":"CAOLa=ZR4DSzdnR8+3eiVTCX92aeFu2o6=iDm7QpdOewizSuxng@mail.gmail.com","subject":"Re: [RFC/PATCH 06/11] branch: roll show_detached HEAD into regular ref_list","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-07-29T09:56:07Z","receivedAt":"2015-07-29T09:56:07Z","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 Tue, Jul 28, 2015 at 6:31 PM, Matthieu Moy\n> wrote:\n>>\n>>> -static void show_detached(struct ref_list *ref_list, int maxwidth)\n>>> -{\n>>> - struct commit *head_commit = lookup_commit_reference_gently(head_sha1, 1);\n>>> -\n>>> - if (head_commit && is_descendant_of(head_commit, ref_list->with_commit)) {\n>>\n>> I'm not sure what this if was doing, and why you can get rid of it. My\n>> understanding is that with_commit comes from --contains, and in the\n>> previous code the filtering was done at display time (detached HEAD was\n>> not shown if it was not contained in commits specified with --contains).\n>>\n>> Eventually, you'll use ref-filter to do this filtering so you won't need\n>> this check at display time.\n>>\n>> But am I correct that for a few commits, you ignore --contains on\n>> detached HEAD?\n>>\n>\n> No we don't ignore --contains on detached HEAD.\n>\n> Since detached HEAD now gets its data from append_ref(). The function\n> also checks for the --contains option.\n\nAh, OK. Previously, detached HEAD and branches were completely\ndifferent, each having its own if (is_descendant_of(...)), and you're\nnow using only one in append_ref() before removing it completely in\nfavor of ref-filter.\n\nThat would deserve an explanation for other reviewers I think.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"267037","messageId":"vpqk2tjmgeg.fsf@anie.imag.fr","threadId":"39952","inReplyTo":"CAOLa=ZTy+KX4K_inpDoMy+thzFdF2RF93yMEHJUExzT4OvjPww@mail.gmail.com","subject":"Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-07-29T10:01:27Z","receivedAt":"2015-07-29T10:01:27Z","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 Tue, Jul 28, 2015 at 6:39 PM, Matthieu Moy\n> wrote:\n>> Karthik Nayak writes:\n>>\n>>> We check if given ref is the current branch in print_ref_list(). Move\n>>> this check to print_ref_item() where it is checked right before\n>>> printing.\n>>\n>> This means that the '*' and the different color are coded in C, hence\n>> it's not possible to mimick this using \"git for-each-ref --format ...\".\n>>\n>> I do not consider this as blocking, but I think the ultimate goal should\n>> be to allow this, so that all the goodies of \"git branch\" can be made\n>> available to other ref-listing commands.\n>>\n>\n> Not sure what you mean here.\n\nWhat you already know, but probably badly explained ;-).\n\nEventually, the output of \"git branch\" should correspond to a format\nstring (so git branch would be almost an alias for \"git for-each-ref\nrefs/heads/ --format '...'\"). Internally, this would mean using\nshow_ref_array_item instead of print_ref_item. This is what you managed\nto do for \"git tag\".\n\nYou already identified one difficulty with sha1 alignment in \"git branch\n-v\". I'm pointing out another which is that displaying the \"*\" in front\nof the current branch is currently not possible with a format string.\nYou would need an atom like %(displayAStarIfTheBranchIsTheCurrentOne)\n(for which you'd need to find a short-and-sweet name ;-) ).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"267038","messageId":"vpq7fpjmg35.fsf@anie.imag.fr","threadId":"39952","inReplyTo":"CAOLa=ZTXSutXd4HCrXoVoLT0kQzppiC7gUJpnk8DYYCo7M5XNw@mail.gmail.com","subject":"Re: [RFC/PATCH 09/11] branch.c: use 'ref-filter' data structures","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-07-29T10:08:14Z","receivedAt":"2015-07-29T10:08:14Z","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 Tue, Jul 28, 2015 at 7:18 PM, Matthieu Moy\n> wrote:\n>> Christian Couder writes:\n>>\n>>> On Tue, Jul 28, 2015 at 8:56 AM, Karthik Nayak wrote:\n>>>\n>>>> +static void ref_array_append(struct ref_array *array, const char *refname)\n>>>> +{\n>>>> + size_t len = strlen(refname);\n>>>> + struct ref_array_item *ref = xcalloc(1, sizeof(struct ref_array_item) + len + 1);\n>>>> + memcpy(ref->refname, refname, len);\n>>>> + ref->refname[len] = '\\0';\n>>\n>> This looks very much like new_ref_array_item, except that the later also\n>> takes an objectname parameter. I find it suspicious that you leave the\n>> objectname field uninitialized.\n>>\n>\n> Well the objectname is not required here, the idea is to retain the way branch.c\n> works.\n>\n>> Why is this code not calling new_ref_array_item?\n>>\n>\n> Because no objectname is there.\n\nYou do have the object_id in the scope of the call-site, so why not use\nit?\n\n(Well, in any case, do as you think is best, it's temporary throw-away\ncode, we shouldn't loose too much time polishing it)\n\n>> The function disapears in the next commit, but I also think that this\n>> function deserves to exist in ref-filter.{c,h} and remain after the end\n>> of the series.\n>>\n>\n> Why though? I don't see the need when new_ref_array_item() is present.\n\nOK, if the function is specificly for \"append an item but leave the\nobjectname uninitialized\", it's too specific to be useful somewhere\nelse. But then, make it more explicit in the name of the function and/or\nin a comment.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"267047","messageId":"CAOLa=ZT28aFqi3WzTvemA4Yc0i28bavV7+SF2qH5S43Bp5LghQ@mail.gmail.com","threadId":"39952","inReplyTo":"vpqh9oov017.fsf@anie.imag.fr","subject":"Re: [RFC/PATCH 10/11] branch.c: use 'ref-filter' APIs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-29T15:32:45Z","receivedAt":"2015-07-29T15:32:45Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Re-sending this as it wasn't sent as plain text and failed.\n\nOn Tue, Jul 28, 2015 at 7:47 PM, Matthieu Moy\n wrote:\n> Karthik Nayak writes:\n>\n>> @@ -458,7 +345,7 @@ static void add_verbose_info(struct strbuf *out, struct ref_array_item *item,\n>> }\n>>\n>> if (item->kind == REF_LOCAL_BRANCH)\n>> - fill_tracking_info(&stat, item->refname, filter->verbose > 1);\n>> + fill_tracking_info(&stat, refname, filter->verbose > 1);\n>\n> Why can't you continue using item->refname?\n>\n> (It's a real question)\n\nBecause we call add_verbose_info() is called in print_ref_item() which calls\nadd_verbose_info() with refname=desc, where desc is a shortened refname.\nAnd fill_tracking_info expects a shortened refname. hence we give it refname\nwhich is nothing but desc.\n\n>\n>> @@ -635,14 +495,21 @@ static void print_ref_list(struct ref_filter *filter)\n>> /* Print detached heads before sorting and printing the rest */\n>> if (filter->detached) {\n>> print_ref_item(array.items[index - 1], maxwidth, filter, remote_prefix);\n>> - index -= 1;\n>> + array.nr--;\n>> }\n>>\n>> - qsort(array.items, index, sizeof(struct ref_array_item *), ref_cmp);\n>> + if (!sorting) {\n>> + def_sorting.next = NULL;\n>> + def_sorting.atom = parse_ref_filter_atom(sort_type,\n>> + sort_type + strlen(sort_type));\n>> + sorting = &def_sorting;\n>> + }\n>> + ref_array_sort(sorting, &array);\n>\n> Does this belong to print_ref_list()? Is it not possible to extract it\n> to get a code closer to the simple:\n>\n> filter_refs(...);\n> ref_array_sort(...);\n> print_ref_list(...);\n>\n> ?\n>\n\nWe have a ref_defaulting_sorting but that defaults to sorting by 'refname'\nbut what we want in branch.c is to sort by 'type' rather. Hence we\nneed to have this small segment of code. About its placement, IDK if\nprint_ref_list() is the right place. But didn't find a better place.\n\n>> - for (i = 0; i < index; i++)\n>> + for (i = 0; i < array.nr; i++)\n>> print_ref_item(array.items[i], maxwidth, filter, remote_prefix);\n>\n> Now that we have show_ref_array_item, it may make sense to rename\n> print_ref_item to something that make the difference between these\n> functions more explicit. Well, ideally, you'd get rid of it an actually\n> use show_ref_array_item, but if you are to keep it, maybe\n> print_ref_item_default_branch_format (or something shorter)?\n>\n\nI guess it'll be converted to a helper for show_ref_array_item()\neventually so keeping that in mind maybe rename it to\nget_format_and_print_ref().\n\n>> --- a/ref-filter.h\n>> +++ b/ref-filter.h\n>> @@ -49,7 +49,6 @@ struct ref_sorting {\n>> struct ref_array_item {\n>> unsigned char objectname[20];\n>> int flag, kind;\n>> - int ignore : 1;\n>\n> You should explain why you needed it and why you don't need it anymore\n> (I guess, because it was used to implement --merge and you now get it\n> from ref-filter).\n>\n\nYeah, I'll add that into the commit message.\n\n>> --- a/t/t1430-bad-ref-name.sh\n>> +++ b/t/t1430-bad-ref-name.sh\n>> @@ -38,7 +38,7 @@ test_expect_success 'fast-import: fail on invalid branch name \"bad[branch]name\"'\n>> test_must_fail git fast-import > '\n>>\n>> -test_expect_success 'git branch shows badly named ref' '\n>> +test_expect_failure 'git branch does not show badly named ref' '\n>\n> I'm not sure what's the convention, but I think the test description\n> should give the expected behavior even with test_expect_failure.\n>\n> And please help the reviewers by saying what's the status wrt this test\n> (any plan on how to fix it?).\n>\n\nWell okay will rename the test description.\nSince we use filter_refs() there's no real fix for this, ref_filter_handler()\nskips over such refs. I'll mention something on these lines in the\ncommit message.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"267048","messageId":"CAOLa=ZR6Cu_AgB4sOBX3Tf_M0w8XN57ej8d_fZo+h7pwFDZs+A@mail.gmail.com","threadId":"39952","inReplyTo":"vpqh9oov017.fsf@anie.imag.fr","subject":"Re: [RFC/PATCH 10/11] branch.c: use 'ref-filter' APIs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-29T15:37:43Z","receivedAt":"2015-07-29T15:37:43Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Jul 28, 2015 at 7:47 PM, Matthieu Moy\n wrote:\n>\n> I'm not sure what's the convention, but I think the test description\n> should give the expected behavior even with test_expect_failure.\n>\n> And please help the reviewers by saying what's the status wrt this test\n> (any plan on how to fix it?).\n>\n\nOn the other hand I wonder if the test is even needed as, we don't\nreally need it\nCause we remove that ability of branch.c by using filter_refs().\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"267049","messageId":"CAOLa=ZTf6QVm-eXYNAk3Tof8DUeP2eZEAcVEGzkDysYwSkE1VA@mail.gmail.com","threadId":"39952","inReplyTo":"CA+P7+xp6FKDw26Qudi+kHi+CQ-rB1BFwOQdk65VyyAwTRW=vGg@mail.gmail.com","subject":"Re: [RFC/PATCH 11/11] branch: add '--points-at' option","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-29T15:44:29Z","receivedAt":"2015-07-29T15:44:29Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Jul 28, 2015 at 1:16 PM, Jacob Keller wrote:\n> On Tue, Jul 28, 2015 at 12:11 AM, Karthik Nayak wrote:\n>> Add the '--points-at' option provided by 'ref-filter'. The option lets\n>> the user to list only branches which points at the given object.\n>>\n>> Add documentation and tests for the same.\n>>\n>> Mentored-by: Christian Couder \n>> Mentored-by: Matthieu Moy \n>> Signed-off-by: Karthik Nayak \n>> ---\n>> Documentation/git-branch.txt | 6 +++++-\n>> builtin/branch.c | 7 ++++++-\n>> t/t3203-branch-output.sh | 9 +++++++++\n>> 3 files changed, 20 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt\n>> index 897cd81..efa23a5 100644\n>> --- a/Documentation/git-branch.txt\n>> +++ b/Documentation/git-branch.txt\n>> @@ -11,7 +11,8 @@ SYNOPSIS\n>> 'git branch' [--color[=] | --no-color] [-r | -a]\n>> [--list] [-v [--abbrev= | --no-abbrev]]\n>> [--column[=] | --no-column]\n>> - [(--merged | --no-merged | --contains) []] [--sort=] [...]\n>> + [(--merged | --no-merged | --contains) []] [--sort=]\n>> + [--points-at ] [...]\n>> 'git branch' [--set-upstream | --track | --no-track] [-l] [-f] []\n>> 'git branch' (--set-upstream-to= | -u ) []\n>> 'git branch' --unset-upstream []\n>> @@ -237,6 +238,9 @@ start-point is either a local or remote-tracking branch.\n>> for-each-ref`. Sort order defaults to sorting based on branch\n>> type.\n>>\n>> +--points-at ::\n>> + Only list tags of the given object.\n>> +\n>\n> s/tags/branches/ ?? Since this is for the branch version, I think this\n> is just a copy-paste oversight.\n>\n>> Examples\n>> --------\n>>\n>> diff --git a/builtin/branch.c b/builtin/branch.c\n>> index 75d8bfd..d25f43b 100644\n>> --- a/builtin/branch.c\n>> +++ b/builtin/branch.c\n>> @@ -26,6 +26,7 @@ static const char * const builtin_branch_usage[] = {\n>> N_(\"git branch [] [-l] [-f] []\"),\n>> N_(\"git branch [] [-r] (-d | -D) ...\"),\n>> N_(\"git branch [] (-m | -M) [] \"),\n>> + N_(\"git branch [] [-r | -a] [--points-at]\"),\n>> NULL\n>> };\n>>\n>> @@ -647,6 +648,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n>> OPT_COLUMN(0, \"column\", &colopts, N_(\"list branches in columns\")),\n>> OPT_CALLBACK(0 , \"sort\", sorting_tail, N_(\"key\"),\n>> N_(\"field name to sort on\"), &parse_opt_ref_sorting),\n>> + {\n>> + OPTION_CALLBACK, 0, \"points-at\", &filter.points_at, N_(\"object\"),\n>> + N_(\"print only tags of the object\"), 0, parse_opt_object_name\n>> + },\n>\n> Same as above. s/tags/branches/\n>\n>> OPT_END(),\n>> };\n>>\n>> @@ -675,7 +680,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n>> if (!delete && !rename && !edit_description && !new_upstream && !unset_upstream && argc == 0)\n>> list = 1;\n>>\n>> - if (filter.with_commit || filter.merge != REF_FILTER_MERGED_NONE)\n>> + if (filter.with_commit || filter.merge != REF_FILTER_MERGED_NONE || filter.points_at.nr)\n>> list = 1;\n>>\n>> if (!!delete + !!rename + !!new_upstream +\n>> diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh\n>> index 38c68bd..1deb7cb 100755\n>> --- a/t/t3203-branch-output.sh\n>> +++ b/t/t3203-branch-output.sh\n>> @@ -154,4 +154,13 @@ EOF\n>> test_i18ncmp expect actual\n>> '\n>>\n>> +test_expect_success 'git branch --points-at option' '\n>> + cat >expect <> + master\n>> + branch-one\n>> +EOF\n>> + git branch --points-at=branch-one >actual &&\n>> + test_cmp expect actual\n>> +'\n>> +\n>> test_done\n>> --\n>> 2.4.6\n>>\n>> --\n>> To unsubscribe from this list: send the line \"unsubscribe git\" in\n>> the body of a message to majordomo@vger.kernel.org\n>> More majordomo info at http://vger.kernel.org/majordomo-info.html\n\nCopy paste, errors, thanks for pointing out.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"267050","messageId":"vpqegjrge63.fsf@anie.imag.fr","threadId":"39952","inReplyTo":"CAOLa=ZT28aFqi3WzTvemA4Yc0i28bavV7+SF2qH5S43Bp5LghQ@mail.gmail.com","subject":"Re: [RFC/PATCH 10/11] branch.c: use 'ref-filter' APIs","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-07-29T15:46:12Z","receivedAt":"2015-07-29T15:46:12Z","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 Tue, Jul 28, 2015 at 7:47 PM, Matthieu Moy\n> wrote:\n>\n>>> - qsort(array.items, index, sizeof(struct ref_array_item *), ref_cmp);\n>>> + if (!sorting) {\n>>> + def_sorting.next = NULL;\n>>> + def_sorting.atom = parse_ref_filter_atom(sort_type,\n>>> + sort_type + strlen(sort_type));\n>>> + sorting = &def_sorting;\n>>> + }\n>>> + ref_array_sort(sorting, &array);\n>>\n>> Does this belong to print_ref_list()? Is it not possible to extract it\n>> to get a code closer to the simple:\n>>\n>> filter_refs(...);\n>> ref_array_sort(...);\n>> print_ref_list(...);\n>>\n>> ?\n>>\n>\n> We have a ref_defaulting_sorting but that defaults to sorting by 'refname'\n> but what we want in branch.c is to sort by 'type' rather. Hence we\n> need to have this small segment of code. About its placement, IDK if\n> print_ref_list() is the right place. But didn't find a better place.\n\nHmm, actually I think I misread the code: print_ref_list() does follow\nthe pattern filter_refs -> sort -> print.\n\nPerhaps the function name is misleading, and you could make it clearer\nthat it computes the list and displays it. I don't know.\n\n>>> --- a/t/t1430-bad-ref-name.sh\n>>> +++ b/t/t1430-bad-ref-name.sh\n>>> @@ -38,7 +38,7 @@ test_expect_success 'fast-import: fail on invalid branch name \"bad[branch]name\"'\n>>> test_must_fail git fast-import >> '\n>>>\n>>> -test_expect_success 'git branch shows badly named ref' '\n>>> +test_expect_failure 'git branch does not show badly named ref' '\n>>\n>> I'm not sure what's the convention, but I think the test description\n>> should give the expected behavior even with test_expect_failure.\n>>\n>> And please help the reviewers by saying what's the status wrt this test\n>> (any plan on how to fix it?).\n>>\n>\n> Well okay will rename the test description.\n> Since we use filter_refs() there's no real fix for this, ref_filter_handler()\n> skips over such refs.\n\nEither there's a good rationale for ignoring these refs, and you should\nchange the test keeping it \"test_expect_success\" (i.e. \"the test\nobserved corresponds to the expected behavior\"), or you should\neventually modify filter_refs() to allow not dropping them.\n\nThis\n\n-test_expect_success '...'\n+test_expect_failure '...'\n\nexplicitly says \"I have a known regression in this patch\".\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"267059","messageId":"CAOLa=ZTQSvFtNUPRNfJMsFVz7TY_u=usopYSiG2QrGuQYyAMDQ@mail.gmail.com","threadId":"39952","inReplyTo":"vpqr3nsnp7o.fsf@anie.imag.fr","subject":"Re: [RFC/PATCH] Port branch.c to use ref-filter APIs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-29T15:54:02Z","receivedAt":"2015-07-29T15:54:02Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Jul 28, 2015 at 11:23 PM, Matthieu Moy\n wrote:\n> Karthik Nayak writes:\n>\n>> On Tue, Jul 28, 2015 at 7:05 PM, Matthieu Moy\n>> wrote:\n>>\n>>> Ideally, you could also have a modifier atom %(alignleft) [...]\n>>\n>> This could work for refname, as each ref_array_item holds the refname,\n>> But other atoms are only filled in after a call to\n>> verify_ref_format().\n>\n> If you swap the order and call verify_ref_format() after filter_refs(),\n> then you could detect %(alignleft) and iterate over the list as needed\n> in verify_ref_format().\n>\n> (You would actually force swapping the order and you would need to adapt\n> other callers in for-each-ref and tag)\n>\n>> But I don't see this as priority for now, so will mark it off for later.\n>\n> Absolutely, that's what I meant by \"Ideally\". I'm just thinking aloud.\n>\n\nMaybe after GSoC ;-)\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"267061","messageId":"vpq3807gdp0.fsf@anie.imag.fr","threadId":"39952","inReplyTo":"CAOLa=ZR6Cu_AgB4sOBX3Tf_M0w8XN57ej8d_fZo+h7pwFDZs+A@mail.gmail.com","subject":"Re: [RFC/PATCH 10/11] branch.c: use 'ref-filter' APIs","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-07-29T15:56:27Z","receivedAt":"2015-07-29T15:56:27Z","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 Tue, Jul 28, 2015 at 7:47 PM, Matthieu Moy\n> wrote:\n>>\n>> I'm not sure what's the convention, but I think the test description\n>> should give the expected behavior even with test_expect_failure.\n>>\n>> And please help the reviewers by saying what's the status wrt this test\n>> (any plan on how to fix it?).\n>>\n>\n> On the other hand I wonder if the test is even needed as, we don't\n> really need it\n> Cause we remove that ability of branch.c by using filter_refs().\n\nPlease read d0f810f (refs.c: allow listing and deleting badly named\nrefs, 2014-09-03). I think the reasoning makes sense, and we should keep\nthis ability.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"267063","messageId":"CAOLa=ZSj-7Gr-QPC8bNJU9+FDkxmw-bhQhzUs91PSFJJQnZNtQ@mail.gmail.com","threadId":"39952","inReplyTo":"xmqqvbd4fe5w.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC/PATCH 01/11] ref-filter: add \"%(objectname:size=X)\" option","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-29T16:02:19Z","receivedAt":"2015-07-29T16:02:19Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Jul 28, 2015 at 9:49 PM, Junio C Hamano wrote:\n> Karthik Nayak writes:\n>\n>> From: Karthik Nayak \n>>\n>> Add support for %(objectname:size=X) where X is a number.\n>> This will print the first X characters of an objectname.\n>> The minimum value for X is 5. Hence any value lesser than\n>> 5 will default to 5 characters.\n>\n> Where does this hardcoded 5 come from?\n>\n> I'd agree that we would want some minimum for sanity (not safety),\n> but I do not think we want random callers of find-unique-abbrev\n> arbitrarily imposing their own minimum, making different codepaths\n> behave inconsistently without a good reason.\n>\n> It seems that the minimum we use for sanity at the core level is\n> MINIMUM_ABBREV. Is there a reason why that value is inappropriate\n> for this codepath?\n>\n\nI don't quite remember, This is definitely wrong. Thanks\nWill use MINIMUM_ABBREV.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"267075","messageId":"CAOLa=ZTUJHnXWJyr0wp0CZgR9S1pBxCEsGRiDPsJdUyzTZaZUw@mail.gmail.com","threadId":"39952","inReplyTo":"xmqq8ua0f9lk.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-29T17:48:16Z","receivedAt":"2015-07-29T17:48:16Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Jul 28, 2015 at 11:27 PM, Junio C Hamano wrote:\n> Karthik Nayak writes:\n>\n>> The 'ifexists' atom allows us to print a required format if the\n>> preceeding atom has a value. If the preceeding atom has no value then\n>> the format given is not printed. e.g. to print \"[]\" we can\n>> now use the format \"%(ifexists:[%s])%(refname)\".\n>\n> A handful of \"huh?\" on the design.\n>\n> - The atom says \"if *exists*\" and explanation says \"has a value\".\n> How are they related? Does an atom whose value is an empty\n> string has a value? Or is \"ifexists\" meant to be used only to\n> ignore meaningless atom, e.g. %(*objectname) applied to a ref that\n> refers to an object that is not an annotated tag?\n\nIt's meant to ignore meaningless atom. atom's whose values are empty\nstrings are ignored.\n\n>\n> - That %s looks ugly. Are there cases where a user may want to say\n> %(ifexists:[%i]) or something other than 's' after that per-cent?\n>\n\nCouldn't think of a better replacer, any suggestions would be welcome :)\n\n> . Is it allowed to have more than one %s there?\n> . Is it allowed to have no %s there?\n>\n\n1. yes its allowed to have multiple %s\n2. yes no %s is also allowed.\n\n> - The syntax makes the reader wonder if [] is part of the\n> construct, or just an example of any arbitrary string, i.e. is\n> \"%(ifexists:the %s can be part of arbitrary string)\" valid?\n>\n\nIts given as example, is that misleading?\n\n> - If an arbitrary string is allowed, is there any quoting mechanism\n> to allow \")\" to be part of that arbitrary string?\n\nNope. Haven't done anything for that. I'll look into that :)\n\n>\n> - What, if anything, is allowed to come between %(ifexists...) and\n> the next atom like %(refname)? For example, are these valid\n> constructs?\n>\n> . %(ifexists...)%(padright:20)%(refname)\n\nDoesn't work with padright, maybe we could eventually support that.\n\n> . %(ifexists...) %(refname) [%(subject)]\n>\n\nNot sure what this is.\n\n> - This syntax does not seem to allow switching on an attribute to\n> show or not to show another, e.g. \"if %(*objectname) makes sense,\n> then show '%(padright:20)%(refname:short) %(*subject)' for it\".\n\nYes this doesn't do that, I can say this is a pretty basic version, we could\nprobably work on and implement more things?\nThis is currently to support 'git branch -vv' where we have the upstream\nin square brackets.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"267077","messageId":"CAOLa=ZSKxw2yA1MWh+AL=rHT8unvZsGUuELWeaCp=vvXUN8S+Q@mail.gmail.com","threadId":"39952","inReplyTo":"vpqwpxjmgnc.fsf@anie.imag.fr","subject":"Re: [RFC/PATCH 06/11] branch: roll show_detached HEAD into regular ref_list","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-29T17:54:49Z","receivedAt":"2015-07-29T17:54:49Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Wed, Jul 29, 2015 at 3:26 PM, Matthieu Moy\n wrote:\n> Karthik Nayak writes:\n>\n>> On Tue, Jul 28, 2015 at 6:31 PM, Matthieu Moy\n>> wrote:\n>>>\n>>>> -static void show_detached(struct ref_list *ref_list, int maxwidth)\n>>>> -{\n>>>> - struct commit *head_commit = lookup_commit_reference_gently(head_sha1, 1);\n>>>> -\n>>>> - if (head_commit && is_descendant_of(head_commit, ref_list->with_commit)) {\n>>>\n>>> I'm not sure what this if was doing, and why you can get rid of it. My\n>>> understanding is that with_commit comes from --contains, and in the\n>>> previous code the filtering was done at display time (detached HEAD was\n>>> not shown if it was not contained in commits specified with --contains).\n>>>\n>>> Eventually, you'll use ref-filter to do this filtering so you won't need\n>>> this check at display time.\n>>>\n>>> But am I correct that for a few commits, you ignore --contains on\n>>> detached HEAD?\n>>>\n>>\n>> No we don't ignore --contains on detached HEAD.\n>>\n>> Since detached HEAD now gets its data from append_ref(). The function\n>> also checks for the --contains option.\n>\n> Ah, OK. Previously, detached HEAD and branches were completely\n> different, each having its own if (is_descendant_of(...)), and you're\n> now using only one in append_ref() before removing it completely in\n> favor of ref-filter.\n>\n> That would deserve an explanation for other reviewers I think.\n>\n\nWill include a small explanation in the commit message :)\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"267084","messageId":"xmqqy4hy973q.fsf@gitster.dls.corp.google.com","threadId":"39952","inReplyTo":"CAOLa=ZTUJHnXWJyr0wp0CZgR9S1pBxCEsGRiDPsJdUyzTZaZUw@mail.gmail.com","subject":"Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-29T18:00:41Z","receivedAt":"2015-07-29T18:00:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n>> A handful of \"huh?\" on the design.\n>>\n>> - The atom says \"if *exists*\" and explanation says \"has a value\".\n>> How are they related? Does an atom whose value is an empty\n>> string has a value? Or is \"ifexists\" meant to be used only to\n>> ignore meaningless atom, e.g. %(*objectname) applied to a ref that\n>> refers to an object that is not an annotated tag?\n>\n> It's meant to ignore meaningless atom. atom's whose values are empty\n> strings are ignored.\n\nThat is a self-contradicting answer.\n\nIf you ask for \"%(*objectname)\" on a commit, that request truly is\nmeaningless, as a commit is not an annotated tag that points at another\nobject whose objectname is being asked for.\n\nBut if a commit has an empty log message (you should be able to\ncreate such an object with commit-tree), then \"%(subject)\" would be\nan empty string. The fact that the commit happens to have an empty\nstring as its message is far from meaningless.\n\nEither you ignore an empty string, or you ignore meaningless one.\nWhich does \"ifexists\" mean?\n\n>> - That %s looks ugly. Are there cases where a user may want to say\n>> %(ifexists:[%i]) or something other than 's' after that per-cent?\n>\n> Couldn't think of a better replacer, any suggestions would be welcome :)\n\nSee below.\n\n> Its given as example, is that misleading?\n\nOthewise I wouldn't be asking.\n\n>> - What, if anything, is allowed to come between %(ifexists...) and\n>> the next atom like %(refname)? For example, are these valid\n>> constructs?\n>>\n>> . %(ifexists...)%(padright:20)%(refname)\n>\n> Doesn't work ...\n> ...\n>> - This syntax does not seem to allow switching on an attribute to\n>> show or not to show another, e.g. \"if %(*objectname) makes sense,\n>> then show '%(padright:20)%(refname:short) %(*subject)' for it\".\n>\n> Yes this doesn't do that,\n\nOne way to do all of the above is to make it\n\n %(ifexists:atom:expansionString)\n\nThat is, for example:\n\n \"%(ifexists:*objectname:tag %(color:blue)%(refname:short)%(color:reset))\"\n\nwould give you a string \"tag v1.0\" with \"v1.0\" painted in blue for\nrefs/tags/v1.0 but nothing for refs/heads/master.\n\nObviously expansionString part needs some escaping mechanism to\nallow it to include an unmatched \")\".\n"},{"id":"267085","messageId":"CAOLa=ZSjjmYOBs1=rg_paon0iUYAwEzoL6FVE5sGeydknXa3tg@mail.gmail.com","threadId":"39952","inReplyTo":"CA+P7+xrGjeXPg3Fbp3A3gJbFeK2E-CP0uXQHWBb3NbNVUHUe2g@mail.gmail.com","subject":"Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-29T18:44:14Z","receivedAt":"2015-07-29T18:44:14Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Wed, Jul 29, 2015 at 6:16 AM, Jacob Keller wrote:\n> On Tue, Jul 28, 2015 at 1:12 PM, Karthik Nayak wrote:\n>> On Tue, Jul 28, 2015 at 6:39 PM, Matthieu Moy\n>> wrote:\n>>> Karthik Nayak writes:\n>>>\n>>>> We check if given ref is the current branch in print_ref_list(). Move\n>>>> this check to print_ref_item() where it is checked right before\n>>>> printing.\n>>>\n>>> This means that the '*' and the different color are coded in C, hence\n>>> it's not possible to mimick this using \"git for-each-ref --format ...\".\n>>>\n>>> I do not consider this as blocking, but I think the ultimate goal should\n>>> be to allow this, so that all the goodies of \"git branch\" can be made\n>>> available to other ref-listing commands.\n>>>\n>>\n>> Not sure what you mean here.\n>>\n>\n> He means to make sure that any feature that was in tag, branch,\n> for-each-ref, etc should be available as part of ref-filter and in all\n> of them\n>\n> Maybe he misunderstood code or maybe you misunderstood the comment here?\n>\n> Regards,\n> Jake\n\nAh, I didn't quite understand the first time. Thanks :)\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"267086","messageId":"CAOLa=ZRUnF12gO=AKg3E373XETqoykqOXSkPvr7DHCVrOeggAA@mail.gmail.com","threadId":"39952","inReplyTo":"vpqk2tjmgeg.fsf@anie.imag.fr","subject":"Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-29T18:52:31Z","receivedAt":"2015-07-29T18:52:31Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Wed, Jul 29, 2015 at 3:31 PM, Matthieu Moy\n wrote:\n> Karthik Nayak writes:\n>\n>> On Tue, Jul 28, 2015 at 6:39 PM, Matthieu Moy\n>> wrote:\n>>> Karthik Nayak writes:\n>>>\n>>>> We check if given ref is the current branch in print_ref_list(). Move\n>>>> this check to print_ref_item() where it is checked right before\n>>>> printing.\n>>>\n>>> This means that the '*' and the different color are coded in C, hence\n>>> it's not possible to mimick this using \"git for-each-ref --format ...\".\n>>>\n>>> I do not consider this as blocking, but I think the ultimate goal should\n>>> be to allow this, so that all the goodies of \"git branch\" can be made\n>>> available to other ref-listing commands.\n>>>\n>>\n>> Not sure what you mean here.\n>\n> What you already know, but probably badly explained ;-).\n>\n> Eventually, the output of \"git branch\" should correspond to a format\n> string (so git branch would be almost an alias for \"git for-each-ref\n> refs/heads/ --format '...'\"). Internally, this would mean using\n> show_ref_array_item instead of print_ref_item. This is what you managed\n> to do for \"git tag\".\n>\n> You already identified one difficulty with sha1 alignment in \"git branch\n> -v\". I'm pointing out another which is that displaying the \"*\" in front\n> of the current branch is currently not possible with a format string.\n> You would need an atom like %(displayAStarIfTheBranchIsTheCurrentOne)\n> (for which you'd need to find a short-and-sweet name ;-) ).\n>\n\nWhat I was thinking of was something like this :\n\nstruct strbuf format = STRBUF_INIT;\nchar c = ' ';\nif (current)\n c = '*';\nstrbuf_addf(&format, \"%c....\", c, other format options...);\nshow_ref_array_item(item, format.buf, quote_style, 0);\nstrbuf_release(&format);\n\nThis would remove the need of making the printing of the \"*\" to be\nneeded by ref-filter. As this is only needed in branch.c\n\nIf going on what you're saying we could have a \"%(starifcurrent)\" atom or\nsomething, but I don't see a general use for this.\n\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"267087","messageId":"xmqqtwsm94j1.fsf@gitster.dls.corp.google.com","threadId":"39952","inReplyTo":"xmqqy4hy973q.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-29T18:56:18Z","receivedAt":"2015-07-29T18:56:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> Couldn't think of a better replacer, any suggestions would be welcome :)\n>\n> See below.\n> ...\n> One way to do all of the above is ...\n\nNote that is just \"one way\", not the only or not necessarily the\nbest. It certainly is not the easiest, I think.\n\n %(if:atom)...%(endif)\n\nmight be easier to implement.\n"},{"id":"267096","messageId":"CAOLa=ZQRWK2c45H7RuFaJSiYuuOBXT5PK__nhWPmA333yGTWyw@mail.gmail.com","threadId":"39952","inReplyTo":"vpq7fpjmg35.fsf@anie.imag.fr","subject":"Re: [RFC/PATCH 09/11] branch.c: use 'ref-filter' data structures","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-29T19:38:37Z","receivedAt":"2015-07-29T19:38:37Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Wed, Jul 29, 2015 at 3:38 PM, Matthieu Moy\n wrote:\n> Karthik Nayak writes:\n>\n>> On Tue, Jul 28, 2015 at 7:18 PM, Matthieu Moy\n>> wrote:\n>>> Christian Couder writes:\n>>>\n>>>> On Tue, Jul 28, 2015 at 8:56 AM, Karthik Nayak wrote:\n>>>>\n>>>>> +static void ref_array_append(struct ref_array *array, const char *refname)\n>>>>> +{\n>>>>> + size_t len = strlen(refname);\n>>>>> + struct ref_array_item *ref = xcalloc(1, sizeof(struct ref_array_item) + len + 1);\n>>>>> + memcpy(ref->refname, refname, len);\n>>>>> + ref->refname[len] = '\\0';\n>>>\n>>> This looks very much like new_ref_array_item, except that the later also\n>>> takes an objectname parameter. I find it suspicious that you leave the\n>>> objectname field uninitialized.\n>>>\n>>\n>> Well the objectname is not required here, the idea is to retain the way branch.c\n>> works.\n>>\n>>> Why is this code not calling new_ref_array_item?\n>>>\n>>\n>> Because no objectname is there.\n>\n> You do have the object_id in the scope of the call-site, so why not use\n> it?\n>\n> (Well, in any case, do as you think is best, it's temporary throw-away\n> code, we shouldn't loose too much time polishing it)\n>\n\nAlso the fact the new_ref_array_item() is static and not shared. Wouldn't make\nsense to make it an API only for a single commit, when the code itself\nis temporary.\n\n>>> The function disapears in the next commit, but I also think that this\n>>> function deserves to exist in ref-filter.{c,h} and remain after the end\n>>> of the series.\n>>>\n>>\n>> Why though? I don't see the need when new_ref_array_item() is present.\n>\n> OK, if the function is specificly for \"append an item but leave the\n> objectname uninitialized\", it's too specific to be useful somewhere\n> else. But then, make it more explicit in the name of the function and/or\n> in a comment.\n>\n\nWill add a comment :)\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"267099","messageId":"CAPig+cTb3U2AHjr4wmQGezqHOED3zyYHPG54BjdCZObt=7Mu3w@mail.gmail.com","threadId":"39952","inReplyTo":"1438066594-5620-2-git-send-email-Karthik.188@gmail.com","subject":"Re: [RFC/PATCH 02/11] ref-filter: add 'colornext' atom","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-07-29T20:10:40Z","receivedAt":"2015-07-29T20:10:40Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tuesday, July 28, 2015, Karthik Nayak wrote:\n> The 'colornext' atom allows us to color only the succeeding atom with\n> a particular color. This resets any previous color preferences set. It\n> is not compatible with `padright`. This is required for printing\n> formats like `git branch -vv` where the upstream is colored in blue.\n\nCan you explain here and in the commit message why %(colornext) is not\ncompatible with %(padright:)? Is it an implementation limitation, or a\nsyntax or semantic limitation, or what? Can the implementation be\nfixed to make it compatible or does it require a redesign?\n\nAlso, please explain here and in the commit message why this highly\nspecialized colorizer ('colornext'), is needed even though a more\ngeneral purpose one ('color') is already available.\n"},{"id":"267106","messageId":"vpqh9omek39.fsf@anie.imag.fr","threadId":"39952","inReplyTo":"xmqqtwsm94j1.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-07-29T21:21:14Z","receivedAt":"2015-07-29T21:21:14Z","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> Junio C Hamano writes:\n>\n>>> Couldn't think of a better replacer, any suggestions would be welcome :)\n>>\n>> See below.\n>> ...\n>> One way to do all of the above is ...\n>\n> Note that is just \"one way\", not the only or not necessarily the\n> best. It certainly is not the easiest, I think.\n>\n> %(if:atom)...%(endif)\n>\n> might be easier to implement.\n\nAnd I find it easier to read or write too. Nested parenthesis in a\nformat string make them really tricky. That removes the need for\nescaping since the content of the if/endif is a format string like the\nothers, it can use the same escaping rules (IIRC, %% to escape a %).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"267107","messageId":"vpq4mkmejsh.fsf@anie.imag.fr","threadId":"39952","inReplyTo":"CAOLa=ZRUnF12gO=AKg3E373XETqoykqOXSkPvr7DHCVrOeggAA@mail.gmail.com","subject":"Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-07-29T21:27:42Z","receivedAt":"2015-07-29T21:27:42Z","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> What I was thinking of was something like this :\n>\n> struct strbuf format = STRBUF_INIT;\n> char c = ' ';\n> if (current)\n> c = '*';\n> strbuf_addf(&format, \"%c....\", c, other format options...);\n> show_ref_array_item(item, format.buf, quote_style, 0);\n> strbuf_release(&format);\n\nI think that would interact badly with verify_ref_format(). Usually, you\nhave just one format string and call verify_ref_format() on it, not a\ndifferent format string for each ref_array_item. That would probably be\nsolvable.\n\n> This would remove the need of making the printing of the \"*\" to be\n> needed by ref-filter. As this is only needed in branch.c\n>\n> If going on what you're saying we could have a \"%(starifcurrent)\" atom or\n> something, but I don't see a general use for this.\n\nTo have a really customizeable format, you would want to allow e.g.\n\n git branch --format '%(starifcurrent) %(objectname) %(refname)'\n\nif the user wants to get the sha1 before the refname, and still have the\nstar in front. It's a bit frustrating to have a hardcoded format that\nthe user can't reproduce with the --format option, since it means you\ncan't easily make a small variation on it.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"267108","messageId":"vpqtwsmd53f.fsf@anie.imag.fr","threadId":"39952","inReplyTo":"CAPig+cTb3U2AHjr4wmQGezqHOED3zyYHPG54BjdCZObt=7Mu3w@mail.gmail.com","subject":"Re: [RFC/PATCH 02/11] ref-filter: add 'colornext' atom","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-07-29T21:30:28Z","receivedAt":"2015-07-29T21:30:28Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Eric Sunshine writes:\n\n> Also, please explain here and in the commit message why this highly\n> specialized colorizer ('colornext'), is needed even though a more\n> general purpose one ('color') is already available.\n\nIt is needed in the current form to allow\n%(colornext:blue)%(ifexists:[%s]) to color only the replacement of %s\nand not the [].\n\nBut I now think that this would be more elegantly solved by Junio's\n%(if) %(endif) idea:\n\n %(if:atom) [ %(color:blue)%(atom)%(color:reset) ] %(endif)\n\n(I added spaces for clarity)\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"267112","messageId":"xmqqwpxi7h6u.fsf@gitster.dls.corp.google.com","threadId":"39952","inReplyTo":"vpqh9omek39.fsf@anie.imag.fr","subject":"Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-29T22:05:45Z","receivedAt":"2015-07-29T22:05:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n> Junio C Hamano writes:\n>\n>> Junio C Hamano writes:\n>>\n>>>> Couldn't think of a better replacer, any suggestions would be welcome :)\n>>>\n>>> See below.\n>>> ...\n>>> One way to do all of the above is ...\n>>\n>> Note that is just \"one way\", not the only or not necessarily the\n>> best. It certainly is not the easiest, I think.\n>>\n>> %(if:atom)...%(endif)\n>>\n>> might be easier to implement.\n>\n> And I find it easier to read or write too. Nested parenthesis in a\n> format string make them really tricky. That removes the need for\n> escaping since the content of the if/endif is a format string like the\n> others, it can use the same escaping rules (IIRC, %% to escape a %).\n\nYeah, that is also a good point.\n"},{"id":"267118","messageId":"CA+P7+xpYg4wsJW8JOGHMsDohCUvcD=X-34wcGGvEQM8mEK3dbg@mail.gmail.com","threadId":"39952","inReplyTo":"vpqtwsmd53f.fsf@anie.imag.fr","subject":"Re: [RFC/PATCH 02/11] ref-filter: add 'colornext' atom","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-07-30T04:27:43Z","receivedAt":"2015-07-30T04:27:43Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, Jul 29, 2015 at 2:30 PM, Matthieu Moy\n wrote:\n> Eric Sunshine writes:\n>\n>> Also, please explain here and in the commit message why this highly\n>> specialized colorizer ('colornext'), is needed even though a more\n>> general purpose one ('color') is already available.\n>\n> It is needed in the current form to allow\n> %(colornext:blue)%(ifexists:[%s]) to color only the replacement of %s\n> and not the [].\n>\n> But I now think that this would be more elegantly solved by Junio's\n> %(if) %(endif) idea:\n>\n> %(if:atom) [ %(color:blue)%(atom)%(color:reset) ] %(endif)\n>\n> (I added spaces for clarity)\n>\n\nI agree, this style seems a lot more elegant and expressive while much\neasier to understand. Same for doing this to the alignment atoms and\nsuch as it solves the same problem(s) there.\n\nI can't speak to how easy it is to implement tho.\n\nRegards,\nJake\n"},{"id":"267123","messageId":"CAOLa=ZTHRLTbmB7iadc3z7=-pshjR0QS8R_StZb7xOyu_gJ=Sw@mail.gmail.com","threadId":"39952","inReplyTo":"vpq3807gdp0.fsf@anie.imag.fr","subject":"Re: [RFC/PATCH 10/11] branch.c: use 'ref-filter' APIs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-07-30T06:37:58Z","receivedAt":"2015-07-30T06:37:58Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Wed, Jul 29, 2015 at 9:26 PM, Matthieu Moy\n wrote:\n> Karthik Nayak writes:\n>\n>> On Tue, Jul 28, 2015 at 7:47 PM, Matthieu Moy\n>> wrote:\n>>>\n>>> I'm not sure what's the convention, but I think the test description\n>>> should give the expected behavior even with test_expect_failure.\n>>>\n>>> And please help the reviewers by saying what's the status wrt this test\n>>> (any plan on how to fix it?).\n>>>\n>>\n>> On the other hand I wonder if the test is even needed as, we don't\n>> really need it\n>> Cause we remove that ability of branch.c by using filter_refs().\n>\n> Please read d0f810f (refs.c: allow listing and deleting badly named\n> refs, 2014-09-03). I think the reasoning makes sense, and we should keep\n> this ability.\n>\n\nThis makes sense, I didn't have a thorough look at this but it breaks\na little in\nref-filter.c while getting object attributes. So is it okay to mark\nthis as TODO?\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"267125","messageId":"vpqbneucddv.fsf@anie.imag.fr","threadId":"39952","inReplyTo":"CAOLa=ZTHRLTbmB7iadc3z7=-pshjR0QS8R_StZb7xOyu_gJ=Sw@mail.gmail.com","subject":"Re: [RFC/PATCH 10/11] branch.c: use 'ref-filter' APIs","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-07-30T07:29:00Z","receivedAt":"2015-07-30T07:29: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> On Wed, Jul 29, 2015 at 9:26 PM, Matthieu Moy\n> wrote:\n>> Karthik Nayak writes:\n>>\n>>> On Tue, Jul 28, 2015 at 7:47 PM, Matthieu Moy\n>>> wrote:\n>>>>\n>>>> I'm not sure what's the convention, but I think the test description\n>>>> should give the expected behavior even with test_expect_failure.\n>>>>\n>>>> And please help the reviewers by saying what's the status wrt this test\n>>>> (any plan on how to fix it?).\n>>>>\n>>>\n>>> On the other hand I wonder if the test is even needed as, we don't\n>>> really need it\n>>> Cause we remove that ability of branch.c by using filter_refs().\n>>\n>> Please read d0f810f (refs.c: allow listing and deleting badly named\n>> refs, 2014-09-03). I think the reasoning makes sense, and we should keep\n>> this ability.\n>>\n>\n> This makes sense, I didn't have a thorough look at this but it breaks\n> a little in\n> ref-filter.c while getting object attributes. So is it okay to mark\n> this as TODO?\n\nSolving this doesn't seem much harder than\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 6c0189f..a4df287 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1117,7 +1117,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n \tstruct commit *commit = NULL;\n \tunsigned int kind;\n \n-\tif (flag & REF_BAD_NAME) {\n+\tif (!filter->show_bad_name_refs && (flag & REF_BAD_NAME)) {\n \t\t warning(\"ignoring ref with broken name %s\", refname);\n \t\t return 0;\n \t}\ndiff --git a/ref-filter.h b/ref-filter.h\nindex 98ebd3b..b9d2bbc 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -79,7 +79,7 @@ struct ref_filter {\n \t\tmatch_as_path : 1;\n \tunsigned int lines, branch_kind;\n \tint abbrev, verbose;\n-\tint detached : 1;\n+\tint detached : 1, show_bad_name_refs : 1;\n };\n \n struct ref_filter_cbdata {\n\nand setting filter->show_bad_name_refs when needed (untested). Did I\nmiss something?\n\nIIRC, historicaly Git allowed some weirdly named refs which made some\ncommands ambiguous (e.g. a branch named after an option like '-d').\nWe're forbidding their creation so people shouldn't have any, but we\nit's important to continue showing them in case some people have old\nbad-named branches lying around.\n\nI'd rather have the code strictly better after your contribution than\nbefore.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"267167","messageId":"xmqqsi857h8d.fsf@gitster.dls.corp.google.com","threadId":"39952","inReplyTo":"CA+P7+xpYg4wsJW8JOGHMsDohCUvcD=X-34wcGGvEQM8mEK3dbg@mail.gmail.com","subject":"Re: [RFC/PATCH 02/11] ref-filter: add 'colornext' atom","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-30T16:17:06Z","receivedAt":"2015-07-30T16:17:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n> On Wed, Jul 29, 2015 at 2:30 PM, Matthieu Moy\n> wrote:\n>> Eric Sunshine writes:\n>>\n>>> Also, please explain here and in the commit message why this highly\n>>> specialized colorizer ('colornext'), is needed even though a more\n>>> general purpose one ('color') is already available.\n>>\n>> It is needed in the current form to allow\n>> %(colornext:blue)%(ifexists:[%s]) to color only the replacement of %s\n>> and not the [].\n>>\n>> But I now think that this would be more elegantly solved by Junio's\n>> %(if) %(endif) idea:\n>>\n>> %(if:atom) [ %(color:blue)%(atom)%(color:reset) ] %(endif)\n>>\n>> (I added spaces for clarity)\n>\n> I agree, this style seems a lot more elegant and expressive while much\n> easier to understand. Same for doing this to the alignment atoms and\n> such as it solves the same problem(s) there.\n\nDo you mean something like these?\n\n %(align:left,20) branch %(refname:short) %(end)\n %(align:middle,20) branch %(refname:short) %(end)\n %(align:right,20) branch %(refname:short) %(end)\n\nto replace and enhance %(padright:20)?\n\n> I can't speak to how easy it is to implement tho.\n\nPerhaps it would go like this:\n\n * Instead of always emitting to the standard output, emit() and\n print_value() will gain an option to append into a strbuf that is\n passed as argument. Alternatively, appending to strbuf could be\n made the only output channel for them; show_ref_array_item() can\n prepare an empty strbuf, call them repeatedly to fill it, and\n then print the resulting strbuf itself.\n\n * Things like %(if) and %(align) would do the following:\n\n (1) Push the currently active strbuf it got from the calling\n show_ref_array_item() to the formatting state;\n\n (2) Create a new strbuf and arrange so that further output would\n be diverted to this new one; and\n\n (3) Push the fact that the diverted output will be processed by\n them (i.e. %(if), %(align), etc.) when the diversion finishes\n to the formatting state.\n\n * When %(end) is seen, the currently active strbuf (i.e. the new\n one created in (2) above for diversion) holds the output made\n since the previously seen %(if), %(align), etc. The formatting\n state knows what processing needs to be performed on that from\n (3) above.\n\n - Pop the strbuf where the output of the entire %(if)...%(end)\n construct needs to go from the formatting state;\n\n - Have the processing popped from (3) above, e.g. %(if:atom) or\n %(align:left,20), do whatever they need to do on the diverted\n output, and emit their result.\n\nBoth %(if) and the hypotherical %(align) can use this same\n\"diversion\" mechanism. And the above would properly nest,\ne.g.\n\n %(align:middle,40)%(if:taggerdate)tag %(end)%(refname:short)%(end)\n"},{"id":"267258","messageId":"CAOLa=ZQ1zShO8tKz9Zug4eUrZxKNGkaqaWd=hcz7KMzJp1pAiQ@mail.gmail.com","threadId":"39952","inReplyTo":"xmqqy4hy973q.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-01T06:44:54Z","receivedAt":"2015-08-01T06:44:54Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Wed, Jul 29, 2015 at 11:30 PM, Junio C Hamano wrote:\n> Karthik Nayak writes:\n>\n>>> A handful of \"huh?\" on the design.\n>>>\n>>> - The atom says \"if *exists*\" and explanation says \"has a value\".\n>>> How are they related? Does an atom whose value is an empty\n>>> string has a value? Or is \"ifexists\" meant to be used only to\n>>> ignore meaningless atom, e.g. %(*objectname) applied to a ref that\n>>> refers to an object that is not an annotated tag?\n>>\n>> It's meant to ignore meaningless atom. atom's whose values are empty\n>> strings are ignored.\n>\n> That is a self-contradicting answer.\n>\n> If you ask for \"%(*objectname)\" on a commit, that request truly is\n> meaningless, as a commit is not an annotated tag that points at another\n> object whose objectname is being asked for.\n>\n> But if a commit has an empty log message (you should be able to\n> create such an object with commit-tree), then \"%(subject)\" would be\n> an empty string. The fact that the commit happens to have an empty\n> string as its message is far from meaningless.\n>\n> Either you ignore an empty string, or you ignore meaningless one.\n> Which does \"ifexists\" mean?\n\nI meant ignore atom values which are empty, sorry for the confusion.\n\n>\n>>> - That %s looks ugly. Are there cases where a user may want to say\n>>> %(ifexists:[%i]) or something other than 's' after that per-cent?\n>>\n>> Couldn't think of a better replacer, any suggestions would be welcome :)\n>\n> See below.\n>\n>> Its given as example, is that misleading?\n>\n> Othewise I wouldn't be asking.\n>\n>>> - What, if anything, is allowed to come between %(ifexists...) and\n>>> the next atom like %(refname)? For example, are these valid\n>>> constructs?\n>>>\n>>> . %(ifexists...)%(padright:20)%(refname)\n>>\n>> Doesn't work ...\n>> ...\n>>> - This syntax does not seem to allow switching on an attribute to\n>>> show or not to show another, e.g. \"if %(*objectname) makes sense,\n>>> then show '%(padright:20)%(refname:short) %(*subject)' for it\".\n>>\n>> Yes this doesn't do that,\n>\n> One way to do all of the above is to make it\n>\n> %(ifexists:atom:expansionString)\n>\n> That is, for example:\n>\n> \"%(ifexists:*objectname:tag %(color:blue)%(refname:short)%(color:reset))\"\n>\n> would give you a string \"tag v1.0\" with \"v1.0\" painted in blue for\n> refs/tags/v1.0 but nothing for refs/heads/master.\n>\n> Obviously expansionString part needs some escaping mechanism to\n> allow it to include an unmatched \")\".\n\nI liked your other idea of if and endif better :)\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"267259","messageId":"CAOLa=ZTC9xJu9WCnF9VVf9wRLSK4=R3576UJ6CqAC7GEGDbCOQ@mail.gmail.com","threadId":"39952","inReplyTo":"vpqh9omek39.fsf@anie.imag.fr","subject":"Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-01T06:46:04Z","receivedAt":"2015-08-01T06:46:04Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Thu, Jul 30, 2015 at 2:51 AM, Matthieu Moy\n wrote:\n> Junio C Hamano writes:\n>\n>> Junio C Hamano writes:\n>>\n>>>> Couldn't think of a better replacer, any suggestions would be welcome :)\n>>>\n>>> See below.\n>>> ...\n>>> One way to do all of the above is ...\n>>\n>> Note that is just \"one way\", not the only or not necessarily the\n>> best. It certainly is not the easiest, I think.\n>>\n>> %(if:atom)...%(endif)\n>>\n>> might be easier to implement.\n>\n> And I find it easier to read or write too. Nested parenthesis in a\n> format string make them really tricky. That removes the need for\n> escaping since the content of the if/endif is a format string like the\n> others, it can use the same escaping rules (IIRC, %% to escape a %).\n>\n\nTHat's a really good point. Will work on this :)\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"267260","messageId":"CAOLa=ZRq7zowwse7OGeU_AwJe_Z0WDn1qoYaH_A6AD5g92e_GQ@mail.gmail.com","threadId":"39952","inReplyTo":"vpq4mkmejsh.fsf@anie.imag.fr","subject":"Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-01T06:48:50Z","receivedAt":"2015-08-01T06:48:50Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Thu, Jul 30, 2015 at 2:57 AM, Matthieu Moy\n wrote:\n> Karthik Nayak writes:\n>\n>> What I was thinking of was something like this :\n>>\n>> struct strbuf format = STRBUF_INIT;\n>> char c = ' ';\n>> if (current)\n>> c = '*';\n>> strbuf_addf(&format, \"%c....\", c, other format options...);\n>> show_ref_array_item(item, format.buf, quote_style, 0);\n>> strbuf_release(&format);\n>\n> I think that would interact badly with verify_ref_format(). Usually, you\n> have just one format string and call verify_ref_format() on it, not a\n> different format string for each ref_array_item. That would probably be\n> solvable.\n>\n\nGood point! I just was wondering if we need another atom just to print a star.\nBut your points certainly are valid. I'll implement this. Thanks :)\n\n>> This would remove the need of making the printing of the \"*\" to be\n>> needed by ref-filter. As this is only needed in branch.c\n>>\n>> If going on what you're saying we could have a \"%(starifcurrent)\" atom or\n>> something, but I don't see a general use for this.\n>\n> To have a really customizeable format, you would want to allow e.g.\n>\n> git branch --format '%(starifcurrent) %(objectname) %(refname)'\n>\n> if the user wants to get the sha1 before the refname, and still have the\n> star in front. It's a bit frustrating to have a hardcoded format that\n> the user can't reproduce with the --format option, since it means you\n> can't easily make a small variation on it.\n>\n\nAgreed. will have a \"starifcurrent\" atom :)\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"267262","messageId":"CA+P7+xoCoFrgT_vspcSqrAfg6kvCGRR9k_1stpf1YiyQUWJeKg@mail.gmail.com","threadId":"39952","inReplyTo":"CAOLa=ZTC9xJu9WCnF9VVf9wRLSK4=R3576UJ6CqAC7GEGDbCOQ@mail.gmail.com","subject":"Re: [RFC/PATCH 04/11] ref-filter: add 'ifexists' atom","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-08-01T07:05:09Z","receivedAt":"2015-08-01T07:05:09Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Fri, Jul 31, 2015 at 11:46 PM, Karthik Nayak wrote:\n> On Thu, Jul 30, 2015 at 2:51 AM, Matthieu Moy\n> wrote:\n>> Junio C Hamano writes:\n>>\n>>> Junio C Hamano writes:\n>>>\n>>>>> Couldn't think of a better replacer, any suggestions would be welcome :)\n>>>>\n>>>> See below.\n>>>> ...\n>>>> One way to do all of the above is ...\n>>>\n>>> Note that is just \"one way\", not the only or not necessarily the\n>>> best. It certainly is not the easiest, I think.\n>>>\n>>> %(if:atom)...%(endif)\n>>>\n>>> might be easier to implement.\n>>\n>> And I find it easier to read or write too. Nested parenthesis in a\n>> format string make them really tricky. That removes the need for\n>> escaping since the content of the if/endif is a format string like the\n>> others, it can use the same escaping rules (IIRC, %% to escape a %).\n>>\n>\n> THat's a really good point. Will work on this :)\n>\n> --\n> Regards,\n> Karthik Nayak\n> --\n\n\nNot sure how much work it would take to extent other atoms to this\nbehavior, such as %(padright) and %(align) and such, that way they\ncould operate on literals and so forth.\n\nMaybe not worth it as part of this GSoC project, as it may be too\ncomplicated, but maybe something to mark as a \"TODO\" for future or\nsomething?\n\nThe only issue being that it might mean we have to keep the old\nimplementation too for backwards compatibility .. Maybe it's easier to\nimplement that I think, or maybe it's far more challenging than I\nthink....\n\nRegards,\nJake\n"},{"id":"267263","messageId":"CA+P7+xqYi9ERzNF1XWMK30_Sbf7y5wnhjWzZGae6wxkTZHDOUQ@mail.gmail.com","threadId":"39952","inReplyTo":"CAOLa=ZRq7zowwse7OGeU_AwJe_Z0WDn1qoYaH_A6AD5g92e_GQ@mail.gmail.com","subject":"Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2015-08-01T07:06:56Z","receivedAt":"2015-08-01T07:06:56Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Fri, Jul 31, 2015 at 11:48 PM, Karthik Nayak wrote:\n> On Thu, Jul 30, 2015 at 2:57 AM, Matthieu Moy\n> wrote:\n>> Karthik Nayak writes:\n>>\n>>> What I was thinking of was something like this :\n>>>\n>>> struct strbuf format = STRBUF_INIT;\n>>> char c = ' ';\n>>> if (current)\n>>> c = '*';\n>>> strbuf_addf(&format, \"%c....\", c, other format options...);\n>>> show_ref_array_item(item, format.buf, quote_style, 0);\n>>> strbuf_release(&format);\n>>\n>> I think that would interact badly with verify_ref_format(). Usually, you\n>> have just one format string and call verify_ref_format() on it, not a\n>> different format string for each ref_array_item. That would probably be\n>> solvable.\n>>\n>\n> Good point! I just was wondering if we need another atom just to print a star.\n> But your points certainly are valid. I'll implement this. Thanks :)\n>\n>>> This would remove the need of making the printing of the \"*\" to be\n>>> needed by ref-filter. As this is only needed in branch.c\n>>>\n>>> If going on what you're saying we could have a \"%(starifcurrent)\" atom or\n>>> something, but I don't see a general use for this.\n>>\n>> To have a really customizeable format, you would want to allow e.g.\n>>\n>> git branch --format '%(starifcurrent) %(objectname) %(refname)'\n>>\n>> if the user wants to get the sha1 before the refname, and still have the\n>> star in front. It's a bit frustrating to have a hardcoded format that\n>> the user can't reproduce with the --format option, since it means you\n>> can't easily make a small variation on it.\n>>\n>\n> Agreed. will have a \"starifcurrent\" atom :)\n>\n> --\n\nWonder if there is some sort of more \"generic\" atom that would work? I\ncan't think of anything obvious at all though... it may be worth\nhaving this even though it definitely seems less useful generically,\nas the reason above where --format should be at least as expressive as\nthe builtin formatting.\n\nRegards,\nJake\n"},{"id":"267264","messageId":"CAPig+cTyK=xDbtsK+q0eL=wmZ87xWue8SMJm=DQOzX22R1AOtQ@mail.gmail.com","threadId":"39952","inReplyTo":"CAOLa=ZRq7zowwse7OGeU_AwJe_Z0WDn1qoYaH_A6AD5g92e_GQ@mail.gmail.com","subject":"Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-01T09:03:56Z","receivedAt":"2015-08-01T09:03:56Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Aug 1, 2015 at 2:48 AM, Karthik Nayak wrote:\n> On Thu, Jul 30, 2015 at 2:57 AM, Matthieu Moy\n> wrote:\n> Good point! I just was wondering if we need another atom just to print a star.\n> But your points certainly are valid. I'll implement this. Thanks :)\n>\n>>> This would remove the need of making the printing of the \"*\" to be\n>>> needed by ref-filter. As this is only needed in branch.c\n>>>\n>>> If going on what you're saying we could have a \"%(starifcurrent)\" atom or\n>>> something, but I don't see a general use for this.\n>>\n>> To have a really customizeable format, you would want to allow e.g.\n>>\n>> git branch --format '%(starifcurrent) %(objectname) %(refname)'\n>>\n>> if the user wants to get the sha1 before the refname, and still have the\n>> star in front. It's a bit frustrating to have a hardcoded format that\n>> the user can't reproduce with the --format option, since it means you\n>> can't easily make a small variation on it.\n>\n> Agreed. will have a \"starifcurrent\" atom :)\n\nPlease don't. It's better to avoid these highly specialized solutions\nand instead seek general purpose ones. For instance, utilizing the\n%(if:) atom suggested elsewhere, you might add an %(iscurrentbranch)\nwhich would allow a format like this:\n\n %(if:iscurrentbranch)*%(endif)\n\nEven more generic would be an %(ifeq:x:y) conditional and a\n%(currentbranch) atom:\n\n %(ifeq:refname:currentbranch)*%(endif)\n\nThose are just a couple ideas. Other variations are possible and\nlikely preferable to the specialized %(starifcurrent).\n"},{"id":"267267","messageId":"CAOLa=ZQbH274PnydmvDQv77QCOQEPfVKViKkh0iEOMcpQ-PcMQ@mail.gmail.com","threadId":"39952","inReplyTo":"xmqqsi857h8d.fsf@gitster.dls.corp.google.com","subject":"Re: [RFC/PATCH 02/11] ref-filter: add 'colornext' atom","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-01T13:06:08Z","receivedAt":"2015-08-01T13:06:08Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Thu, Jul 30, 2015 at 9:47 PM, Junio C Hamano wrote:\n> Jacob Keller writes:\n>\n>> On Wed, Jul 29, 2015 at 2:30 PM, Matthieu Moy\n>> wrote:\n>>> Eric Sunshine writes:\n>>>\n>>>> Also, please explain here and in the commit message why this highly\n>>>> specialized colorizer ('colornext'), is needed even though a more\n>>>> general purpose one ('color') is already available.\n>>>\n>>> It is needed in the current form to allow\n>>> %(colornext:blue)%(ifexists:[%s]) to color only the replacement of %s\n>>> and not the [].\n>>>\n>>> But I now think that this would be more elegantly solved by Junio's\n>>> %(if) %(endif) idea:\n>>>\n>>> %(if:atom) [ %(color:blue)%(atom)%(color:reset) ] %(endif)\n>>>\n>>> (I added spaces for clarity)\n>>\n>> I agree, this style seems a lot more elegant and expressive while much\n>> easier to understand. Same for doing this to the alignment atoms and\n>> such as it solves the same problem(s) there.\n>\n> Do you mean something like these?\n>\n> %(align:left,20) branch %(refname:short) %(end)\n> %(align:middle,20) branch %(refname:short) %(end)\n> %(align:right,20) branch %(refname:short) %(end)\n>\n> to replace and enhance %(padright:20)?\n>\n>> I can't speak to how easy it is to implement tho.\n>\n> Perhaps it would go like this:\n>\n> * Instead of always emitting to the standard output, emit() and\n> print_value() will gain an option to append into a strbuf that is\n> passed as argument. Alternatively, appending to strbuf could be\n> made the only output channel for them; show_ref_array_item() can\n> prepare an empty strbuf, call them repeatedly to fill it, and\n> then print the resulting strbuf itself.\n>\n> * Things like %(if) and %(align) would do the following:\n>\n> (1) Push the currently active strbuf it got from the calling\n> show_ref_array_item() to the formatting state;\n>\n> (2) Create a new strbuf and arrange so that further output would\n> be diverted to this new one; and\n>\n> (3) Push the fact that the diverted output will be processed by\n> them (i.e. %(if), %(align), etc.) when the diversion finishes\n> to the formatting state.\n>\n> * When %(end) is seen, the currently active strbuf (i.e. the new\n> one created in (2) above for diversion) holds the output made\n> since the previously seen %(if), %(align), etc. The formatting\n> state knows what processing needs to be performed on that from\n> (3) above.\n>\n> - Pop the strbuf where the output of the entire %(if)...%(end)\n> construct needs to go from the formatting state;\n>\n> - Have the processing popped from (3) above, e.g. %(if:atom) or\n> %(align:left,20), do whatever they need to do on the diverted\n> output, and emit their result.\n>\n> Both %(if) and the hypotherical %(align) can use this same\n> \"diversion\" mechanism. And the above would properly nest,\n> e.g.\n>\n> %(align:middle,40)%(if:taggerdate)tag %(end)%(refname:short)%(end)\n>\n\nThis actually looks neat and good, I'll try implementing this :)\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"267284","messageId":"CAOLa=ZREZ2M_txEQR9jPET5_+MXx0D6cfW0X7SS4-TP9MShVrA@mail.gmail.com","threadId":"39952","inReplyTo":"CAPig+cTyK=xDbtsK+q0eL=wmZ87xWue8SMJm=DQOzX22R1AOtQ@mail.gmail.com","subject":"Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-02T12:59:08Z","receivedAt":"2015-08-02T12:59:08Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Sat, Aug 1, 2015 at 2:33 PM, Eric Sunshine wrote:\n> On Sat, Aug 1, 2015 at 2:48 AM, Karthik Nayak wrote:\n>> On Thu, Jul 30, 2015 at 2:57 AM, Matthieu Moy\n>> wrote:\n>> Good point! I just was wondering if we need another atom just to print a star.\n>> But your points certainly are valid. I'll implement this. Thanks :)\n>>\n>>>> This would remove the need of making the printing of the \"*\" to be\n>>>> needed by ref-filter. As this is only needed in branch.c\n>>>>\n>>>> If going on what you're saying we could have a \"%(starifcurrent)\" atom or\n>>>> something, but I don't see a general use for this.\n>>>\n>>> To have a really customizeable format, you would want to allow e.g.\n>>>\n>>> git branch --format '%(starifcurrent) %(objectname) %(refname)'\n>>>\n>>> if the user wants to get the sha1 before the refname, and still have the\n>>> star in front. It's a bit frustrating to have a hardcoded format that\n>>> the user can't reproduce with the --format option, since it means you\n>>> can't easily make a small variation on it.\n>>\n>> Agreed. will have a \"starifcurrent\" atom :)\n>\n> Please don't. It's better to avoid these highly specialized solutions\n> and instead seek general purpose ones. For instance, utilizing the\n> %(if:) atom suggested elsewhere, you might add an %(iscurrentbranch)\n> which would allow a format like this:\n>\n> %(if:iscurrentbranch)*%(endif)\n>\n> Even more generic would be an %(ifeq:x:y) conditional and a\n> %(currentbranch) atom:\n>\n> %(ifeq:refname:currentbranch)*%(endif)\n>\n> Those are just a couple ideas. Other variations are possible and\n> likely preferable to the specialized %(starifcurrent).\n\nThis makes sense, thanks. But implementing something like\n\"%(if:)\" seems to not be as easy as I thought it would be.\n\nFirst we need to parse that inner atom, but the used_atom_cnt is based\non how many atoms there are initially, which doesn't count this inner atom.\n\nAlthough we could have a way around that, we'd need to again call populate_value\nfrom itself to get that inner atom's value. This causes more problems.\nEither ways\nI'm looking at ways around this.\nA simple solution would be to do :\n\n%(if)%(atom)%(then).....%(endif)\n\nor just\n\n%(if)%(atom).....%(endif)\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"267292","messageId":"xmqqa8u9a7yc.fsf@gitster.dls.corp.google.com","threadId":"39952","inReplyTo":"CAOLa=ZREZ2M_txEQR9jPET5_+MXx0D6cfW0X7SS4-TP9MShVrA@mail.gmail.com","subject":"Re: [RFC/PATCH 07/11] branch: move 'current' check down to the presentation layer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-02T17:58:19Z","receivedAt":"2015-08-02T17:58:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n>> Even more generic would be an %(ifeq:x:y) conditional and a\n>> %(currentbranch) atom:\n>>\n>> %(ifeq:refname:currentbranch)*%(endif)\n>>\n>> Those are just a couple ideas. Other variations are possible and\n>> likely preferable to the specialized %(starifcurrent).\n>\n> This makes sense, thanks. But implementing something like\n> \"%(if:)\" seems to not be as easy as I thought it would be.\n>\n> First we need to parse that inner atom, but the used_atom_cnt is based\n> on how many atoms there are initially, which doesn't count this inner atom.\n>\n> Although we could have a way around that, we'd need to again call populate_value\n> from itself to get that inner atom's value. This causes more problems.\n> Either ways\n> I'm looking at ways around this.\n> A simple solution would be to do :\n>\n> %(if)%(atom)%(then).....%(endif)\n>\n> or just\n>\n> %(if)%(atom).....%(endif)\n\nMy knee-jerk reaction to the former was \"Eeww, the users is forced\nto keep verbosely typing unnecessarily '%(', ')%(then)' forever,\nonly because the implementor was too lazy to do the job properly in\nparse_atom()\". I do not think the latter a good idea at all.\n\nBut I think the former is worth considering, as it will later allow\nus to extend it to various forms, e.g.\n\n %(if:notempty)%(atom)%(then)...%(end)\n %(if)%(atom)%(then)...%(elif)%(atom1)%(atom2)%(then)...%(end)\n\nTwo points being\n\n (1) the default \"string is not empty\" does not have to be the only\n test criteria, by leaving the door to add %(if:)\n later;\n\n (2) what is tested does not have to be a single atom (that is why\n I do not think the one without %(then) good at all) but can be\n a string that can later be interpreted.\n\nAnd syntactically, something like this even may want to be\nintroduced later:\n\n %(if:expr)master == %(refname:short)%(then)...%(end)\n"},{"id":"267304","messageId":"CAOLa=ZTNv7bRMZL_yAGSKxDyBPUBuMQKsybQBg3sKbLzM=m0YQ@mail.gmail.com","threadId":"39952","inReplyTo":"vpqbneucddv.fsf@anie.imag.fr","subject":"Re: [RFC/PATCH 10/11] branch.c: use 'ref-filter' APIs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-03T10:20:00Z","receivedAt":"2015-08-03T10:20:00Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Thu, Jul 30, 2015 at 12:59 PM, Matthieu Moy\n wrote:\n> Karthik Nayak writes:\n>\n> Solving this doesn't seem much harder than\n>\n> diff --git a/ref-filter.c b/ref-filter.c\n> index 6c0189f..a4df287 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -1117,7 +1117,7 @@ static int ref_filter_handler(const char *refname, const struct object_id *oid,\n> struct commit *commit = NULL;\n> unsigned int kind;\n>\n> - if (flag & REF_BAD_NAME) {\n> + if (!filter->show_bad_name_refs && (flag & REF_BAD_NAME)) {\n> warning(\"ignoring ref with broken name %s\", refname);\n> return 0;\n> }\n> diff --git a/ref-filter.h b/ref-filter.h\n> index 98ebd3b..b9d2bbc 100644\n> --- a/ref-filter.h\n> +++ b/ref-filter.h\n> @@ -79,7 +79,7 @@ struct ref_filter {\n> match_as_path : 1;\n> unsigned int lines, branch_kind;\n> int abbrev, verbose;\n> - int detached : 1;\n> + int detached : 1, show_bad_name_refs : 1;\n> };\n>\n> struct ref_filter_cbdata {\n>\n> and setting filter->show_bad_name_refs when needed (untested). Did I\n> miss something?\n>\n\nThis allows it to be added to ref_array. But it would still fail while\ntrying to obtain\nobject details prior to printing.\n\n> IIRC, historicaly Git allowed some weirdly named refs which made some\n> commands ambiguous (e.g. a branch named after an option like '-d').\n> We're forbidding their creation so people shouldn't have any, but we\n> it's important to continue showing them in case some people have old\n> bad-named branches lying around.\n>\n> I'd rather have the code strictly better after your contribution than\n> before.\n>\n\nAgreed. But then again the warning tells about the broken ref, as in it's name\nSo I think It's ok?\n\nfor e.g. t1430 :\n[trash directory.t1430-bad-ref-name] ../../git branch\nwarning: ignoring ref with broken name refs/heads/broken...ref\n* master\n\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"268323","messageId":"vpq37zf2s84.fsf@anie.imag.fr","threadId":"39952","inReplyTo":"CAOLa=ZTNv7bRMZL_yAGSKxDyBPUBuMQKsybQBg3sKbLzM=m0YQ@mail.gmail.com","subject":"Re: [RFC/PATCH 10/11] branch.c: use 'ref-filter' APIs","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-08-19T15:49:47Z","receivedAt":"2015-08-19T15:49: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> On Thu, Jul 30, 2015 at 12:59 PM, Matthieu Moy\n> wrote:\n>\n>> IIRC, historicaly Git allowed some weirdly named refs which made some\n>> commands ambiguous (e.g. a branch named after an option like '-d').\n>> We're forbidding their creation so people shouldn't have any, but we\n>> it's important to continue showing them in case some people have old\n>> bad-named branches lying around.\n[...]\n> Agreed. But then again the warning tells about the broken ref, as in it's name\n> So I think It's ok?\n>\n> for e.g. t1430 :\n> [trash directory.t1430-bad-ref-name] ../../git branch\n> warning: ignoring ref with broken name refs/heads/broken...ref\n> * master\n\n[ Late answer, I'm still catching up with my holiday's emails ;-) ]\n\nOK, the warning gives a different interface but it seems as good as the\nold one.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"268325","messageId":"CAOLa=ZQqewSJtf-4WWY=gr3e0sU5TyBK3gcy-b4YXdS8jXCDZg@mail.gmail.com","threadId":"39952","inReplyTo":"vpq37zf2s84.fsf@anie.imag.fr","subject":"Re: [RFC/PATCH 10/11] branch.c: use 'ref-filter' APIs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2015-08-19T15:52:50Z","receivedAt":"2015-08-19T15:52:50Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Wed, Aug 19, 2015 at 9:19 PM, Matthieu Moy\n wrote:\n> Karthik Nayak writes:\n>\n>> On Thu, Jul 30, 2015 at 12:59 PM, Matthieu Moy\n>> wrote:\n>>\n>>> IIRC, historicaly Git allowed some weirdly named refs which made some\n>>> commands ambiguous (e.g. a branch named after an option like '-d').\n>>> We're forbidding their creation so people shouldn't have any, but we\n>>> it's important to continue showing them in case some people have old\n>>> bad-named branches lying around.\n> [...]\n>> Agreed. But then again the warning tells about the broken ref, as in it's name\n>> So I think It's ok?\n>>\n>> for e.g. t1430 :\n>> [trash directory.t1430-bad-ref-name] ../../git branch\n>> warning: ignoring ref with broken name refs/heads/broken...ref\n>> * master\n>\n> [ Late answer, I'm still catching up with my holiday's emails ;-) ]\n>\n> OK, the warning gives a different interface but it seems as good as the\n> old one.\n>\n\nAh! yes :)\n\n-- \nRegards,\nKarthik Nayak\n"}]}