{"thread":{"id":"34376","subject":"[PATCH 0/4] Make \"git name-rev $(git rev-parse v1.8.3)\" work","startedAt":"2013-07-07T22:33:40Z","lastAt":"2013-07-09T18:53:38Z","messageCount":20,"participants":["Junio C Hamano","Michael Haggerty","Ramkumar Ramachandra","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"222783","messageId":"1373236424-25617-1-git-send-email-gitster@pobox.com","threadId":"34376","inReplyTo":null,"subject":"[PATCH 0/4] Make \"git name-rev $(git rev-parse v1.8.3)\" work","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-07T22:33:40Z","receivedAt":"2013-07-07T22:33:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"So here is a set of small preparatory steps to help the other topic\nto allow \"git describe -contains v1.8.3\" omit trailing \"^0\" from its\noutput. We do not want to prevent people from allowing \"name-rev\"\nto convert object names other than commit-ishes.\n\nThe series should apply on 96ffd4ca (Merge branch\n'nk/name-rev-abbreviated-refs', 2013-06-30).\n\nJunio C Hamano (4):\n name-ref: factor out name shortening logic from name_ref()\n name-rev: allow converting the exact object name at the tip of a ref\n describe: use argv-array\n describe/name-rev: tell name-rev to peel the incoming object to commit first\n\n builtin/describe.c | 32 ++++++++-------\n builtin/name-rev.c | 113 ++++++++++++++++++++++++++++++++++++++++++++---------\n 2 files changed, 112 insertions(+), 33 deletions(-)\n\n-- \n1.8.3.2-853-ga8cbcc9\n"},{"id":"222784","messageId":"1373236424-25617-2-git-send-email-gitster@pobox.com","threadId":"34376","inReplyTo":"1373236424-25617-1-git-send-email-gitster@pobox.com","subject":"[PATCH 1/4] name-ref: factor out name shortening logic from name_ref()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-07T22:33:41Z","receivedAt":"2013-07-07T22:33:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The logic will be used in a new codepath for showing exact matches.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/name-rev.c | 19 ++++++++++++-------\n 1 file changed, 12 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/name-rev.c b/builtin/name-rev.c\nindex 87d4854..1234ebb 100644\n--- a/builtin/name-rev.c\n+++ b/builtin/name-rev.c\n@@ -96,6 +96,17 @@ static int subpath_matches(const char *path, const char *filter)\n \treturn -1;\n }\n \n+static const char *name_ref_abbrev(const char *refname, int shorten_unambiguous)\n+{\n+\tif (shorten_unambiguous)\n+\t\trefname = shorten_unambiguous_ref(refname, 0);\n+\telse if (!prefixcmp(refname, \"refs/heads/\"))\n+\t\trefname = refname + 11;\n+\telse if (!prefixcmp(refname, \"refs/\"))\n+\t\trefname = refname + 5;\n+\treturn refname;\n+}\n+\n struct name_ref_data {\n \tint tags_only;\n \tint name_only;\n@@ -134,13 +145,7 @@ static int name_ref(const char *path, const unsigned char *sha1, int flags, void\n \tif (o && o->type == OBJ_COMMIT) {\n \t\tstruct commit *commit = (struct commit *)o;\n \n-\t\tif (can_abbreviate_output)\n-\t\t\tpath = shorten_unambiguous_ref(path, 0);\n-\t\telse if (!prefixcmp(path, \"refs/heads/\"))\n-\t\t\tpath = path + 11;\n-\t\telse if (!prefixcmp(path, \"refs/\"))\n-\t\t\tpath = path + 5;\n-\n+\t\tpath = name_ref_abbrev(path, can_abbreviate_output);\n \t\tname_rev(commit, xstrdup(path), 0, 0, deref);\n \t}\n \treturn 0;\n-- \n1.8.3.2-853-ga8cbcc9\n"},{"id":"222785","messageId":"1373236424-25617-3-git-send-email-gitster@pobox.com","threadId":"34376","inReplyTo":"1373236424-25617-1-git-send-email-gitster@pobox.com","subject":"[PATCH 2/4] name-rev: allow converting the exact object name at the tip of a ref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-07T22:33:42Z","receivedAt":"2013-07-07T22:33:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"git name-rev\" is supposed to convert 40-hex object names into\nstrings that name the same objects based on refs, that can be fed to\n\"git rev-parse\" to get the same object names back, so\n\n $ git rev-parse v1.8.3 v1.8.3^0 | git name-rev --stdin\n 8af06057d0c31a24e8737ae846ac2e116e8bafb9\n edca4152560522a431a51fc0a06147fc680b5b18 (tags/v1.8.3^0)\n\nhas to have \"^0\" at the end, as \"edca41\" is a commit, not the tag\nthat references it.\n\nThe command however did not bother to see if the object is at the\ntip of some ref, and failed to convert a tag object.\n\nTeach it to show this instead:\n\n $ git rev-parse v1.8.3 v1.8.3^0 | git name-rev --stdin\n 8af06057d0c31a24e8737ae846ac2e116e8bafb9 (tags/v1.8.3)\n edca4152560522a431a51fc0a06147fc680b5b18 (tags/v1.8.3^0)\n\nSigned-off-by: Junio C Hamano \n---\n builtin/name-rev.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 58 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/name-rev.c b/builtin/name-rev.c\nindex 1234ebb..29a6f56 100644\n--- a/builtin/name-rev.c\n+++ b/builtin/name-rev.c\n@@ -4,6 +4,7 @@\n #include \"tag.h\"\n #include \"refs.h\"\n #include \"parse-options.h\"\n+#include \"sha1-lookup.h\"\n \n #define CUTOFF_DATE_SLOP 86400 /* one day */\n \n@@ -113,6 +114,34 @@ struct name_ref_data {\n \tconst char *ref_filter;\n };\n \n+static struct tip_table {\n+\tstruct tip_table_entry {\n+\t\tunsigned char sha1[20];\n+\t\tconst char *refname;\n+\t} *table;\n+\tint nr;\n+\tint alloc;\n+\tint sorted;\n+} tip_table;\n+\n+static void add_to_tip_table(const unsigned char *sha1, const char *refname,\n+\t\t\t int shorten_unambiguous)\n+{\n+\trefname = name_ref_abbrev(refname, shorten_unambiguous);\n+\n+\tALLOC_GROW(tip_table.table, tip_table.nr + 1, tip_table.alloc);\n+\thashcpy(tip_table.table[tip_table.nr].sha1, sha1);\n+\ttip_table.table[tip_table.nr].refname = xstrdup(refname);\n+\ttip_table.nr++;\n+\ttip_table.sorted = 0;\n+}\n+\n+static int tipcmp(const void *a_, const void *b_)\n+{\n+\tconst struct tip_table_entry *a = a_, *b = b_;\n+\treturn hashcmp(a->sha1, b->sha1);\n+}\n+\n static int name_ref(const char *path, const unsigned char *sha1, int flags, void *cb_data)\n {\n \tstruct object *o = parse_object(sha1);\n@@ -135,6 +164,8 @@ static int name_ref(const char *path, const unsigned char *sha1, int flags, void\n \t\t}\n \t}\n \n+\tadd_to_tip_table(sha1, path, can_abbreviate_output);\n+\n \twhile (o && o->type == OBJ_TAG) {\n \t\tstruct tag *t = (struct tag *) o;\n \t\tif (!t->tagged)\n@@ -151,6 +182,32 @@ static int name_ref(const char *path, const unsigned char *sha1, int flags, void\n \treturn 0;\n }\n \n+static const unsigned char *nth_tip_table_ent(size_t ix, void *table_)\n+{\n+\tstruct tip_table_entry *table = table_;\n+\treturn table[ix].sha1;\n+}\n+\n+static const char *get_exact_ref_match(const struct object *o)\n+{\n+\tint found;\n+\n+\tif (!tip_table.table || !tip_table.nr)\n+\t\treturn NULL;\n+\n+\tif (!tip_table.sorted) {\n+\t\tqsort(tip_table.table, tip_table.nr, sizeof(*tip_table.table),\n+\t\t tipcmp);\n+\t\ttip_table.sorted = 1;\n+\t}\n+\n+\tfound = sha1_pos(o->sha1, tip_table.table, tip_table.nr,\n+\t\t\t nth_tip_table_ent);\n+\tif (0 <= found)\n+\t\treturn tip_table.table[found].refname;\n+\treturn NULL;\n+}\n+\n /* returns a static buffer */\n static const char *get_rev_name(const struct object *o)\n {\n@@ -159,7 +216,7 @@ static const char *get_rev_name(const struct object *o)\n \tstruct commit *c;\n \n \tif (o->type != OBJ_COMMIT)\n-\t\treturn NULL;\n+\t\treturn get_exact_ref_match(o);\n \tc = (struct commit *) o;\n \tn = c->util;\n \tif (!n)\n-- \n1.8.3.2-853-ga8cbcc9\n"},{"id":"222787","messageId":"1373236424-25617-4-git-send-email-gitster@pobox.com","threadId":"34376","inReplyTo":"1373236424-25617-1-git-send-email-gitster@pobox.com","subject":"[PATCH 3/4] describe: use argv-array","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-07T22:33:43Z","receivedAt":"2013-07-07T22:33:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Instead of using a hand allocated args[] array, use argv-array API\nto manage the dynamically created list of arguments when invoking\nname-rev.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/describe.c | 31 ++++++++++++++++---------------\n 1 file changed, 16 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex 4e675c3..b5434e4 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -7,6 +7,7 @@\n #include \"parse-options.h\"\n #include \"diff.h\"\n #include \"hash.h\"\n+#include \"argv-array.h\"\n \n #define SEEN\t\t(1u<<0)\n #define MAX_TAGS\t(FLAG_BITS - 1)\n@@ -442,24 +443,24 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"--long is incompatible with --abbrev=0\"));\n \n \tif (contains) {\n-\t\tconst char **args = xmalloc((7 + argc) * sizeof(char *));\n-\t\tint i = 0;\n-\t\targs[i++] = \"name-rev\";\n-\t\targs[i++] = \"--name-only\";\n-\t\targs[i++] = \"--no-undefined\";\n+\t\tstruct argv_array args;\n+\n+\t\targv_array_init(&args);\n+\t\targv_array_push(&args, \"name-rev\");\n+\t\targv_array_push(&args, \"--name-only\");\n+\t\targv_array_push(&args, \"--no-undefined\");\n \t\tif (always)\n-\t\t\targs[i++] = \"--always\";\n+\t\t\targv_array_push(&args, \"--always\");\n \t\tif (!all) {\n-\t\t\targs[i++] = \"--tags\";\n-\t\t\tif (pattern) {\n-\t\t\t\tchar *s = xmalloc(strlen(\"--refs=refs/tags/\") + strlen(pattern) + 1);\n-\t\t\t\tsprintf(s, \"--refs=refs/tags/%s\", pattern);\n-\t\t\t\targs[i++] = s;\n-\t\t\t}\n+\t\t\targv_array_push(&args, \"--tags\");\n+\t\t\tif (pattern)\n+\t\t\t\targv_array_pushf(&args, \"--refs=refs/tags/%s\", pattern);\n+\t\t}\n+\t\twhile (*argv) {\n+\t\t\targv_array_push(&args, *argv);\n+\t\t\targv++;\n \t\t}\n-\t\tmemcpy(args + i, argv, argc * sizeof(char *));\n-\t\targs[i + argc] = NULL;\n-\t\treturn cmd_name_rev(i + argc, args, prefix);\n+\t\treturn cmd_name_rev(args.argc, args.argv, prefix);\n \t}\n \n \tinit_hash(&names);\n-- \n1.8.3.2-853-ga8cbcc9\n"},{"id":"222786","messageId":"1373236424-25617-5-git-send-email-gitster@pobox.com","threadId":"34376","inReplyTo":"1373236424-25617-1-git-send-email-gitster@pobox.com","subject":"[PATCH 4/4] describe/name-rev: tell name-rev to peel the incoming object to commit first","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-07T22:33:44Z","receivedAt":"2013-07-07T22:33:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"With this on top of the other patches in this series, you would get:\n\n $ git describe --contains $(git rev-parse v1.8.3 v1.8.3^0)\n v1.8.3\n v1.8.3\n\nwhile you can still differentiate tags and the commits they point at\nwith:\n\n $ git name-rev --refs=tags/\\* --name-only $(git rev-parse v1.8.3 v1.8.3^0)\n v1.8.3\n v1.8.3^0\n\nThe difference in these two behaviours is achieved by adding --peel-to-commit\noption to \"name-rev\" and using it when \"describe\" internally calls it.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/describe.c | 1 +\n builtin/name-rev.c | 35 +++++++++++++++++++++++++----------\n 2 files changed, 26 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex b5434e4..f7adda6 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -447,6 +447,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \n \t\targv_array_init(&args);\n \t\targv_array_push(&args, \"name-rev\");\n+\t\targv_array_push(&args, \"--peel-to-commit\");\n \t\targv_array_push(&args, \"--name-only\");\n \t\targv_array_push(&args, \"--no-undefined\");\n \t\tif (always)\ndiff --git a/builtin/name-rev.c b/builtin/name-rev.c\nindex 29a6f56..fa37731 100644\n--- a/builtin/name-rev.c\n+++ b/builtin/name-rev.c\n@@ -15,6 +15,7 @@ typedef struct rev_name {\n } rev_name;\n \n static long cutoff = LONG_MAX;\n+static int peel_to_commit;\n \n /* How many generations are maximally preferred over _one_ merge traversal? */\n #define MERGE_TRAVERSAL_WEIGHT 65535\n@@ -33,7 +34,7 @@ static void name_rev(struct commit *commit,\n \tif (commit->date < cutoff)\n \t\treturn;\n \n-\tif (deref) {\n+\tif (deref && !peel_to_commit) {\n \t\tchar *new_name = xmalloc(strlen(tip_name)+3);\n \t\tstrcpy(new_name, tip_name);\n \t\tstrcat(new_name, \"^0\");\n@@ -320,6 +321,8 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOLEAN(0, \"undefined\", &allow_undefined, N_(\"allow to print `undefined` names\")),\n \t\tOPT_BOOLEAN(0, \"always\", &always,\n \t\t\t N_(\"show abbreviated commit object as fallback\")),\n+\t\tOPT_BOOLEAN(0, \"peel-to-commit\", &peel_to_commit,\n+\t\t\t N_(\"peel tag object names in the input to a commmit\")),\n \t\tOPT_END(),\n \t};\n \n@@ -334,7 +337,7 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)\n \n \tfor (; argc; argc--, argv++) {\n \t\tunsigned char sha1[20];\n-\t\tstruct object *o;\n+\t\tstruct object *object;\n \t\tstruct commit *commit;\n \n \t\tif (get_sha1(*argv, sha1)) {\n@@ -343,17 +346,29 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)\n \t\t\tcontinue;\n \t\t}\n \n-\t\to = deref_tag(parse_object(sha1), *argv, 0);\n-\t\tif (!o || o->type != OBJ_COMMIT) {\n+\t\tcommit = NULL;\n+\t\tobject = parse_object(sha1);\n+\t\tif (object) {\n+\t\t\tstruct object *peeled = deref_tag(object, *argv, 0);\n+\t\t\tif (peeled && peeled->type == OBJ_COMMIT)\n+\t\t\t\tcommit = (struct commit *) peeled;\n+\t\t}\n+\n+\t\tif (!object) {\n+\t\t\tfprintf(stderr, \"Could not get object for %s. Skipping.\\n\",\n+\t\t\t\t*argv);\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (peel_to_commit && !commit) {\n \t\t\tfprintf(stderr, \"Could not get commit for %s. Skipping.\\n\",\n-\t\t\t\t\t*argv);\n+\t\t\t\t*argv);\n \t\t\tcontinue;\n \t\t}\n-\n-\t\tcommit = (struct commit *)o;\n-\t\tif (cutoff > commit->date)\n-\t\t\tcutoff = commit->date;\n-\t\tadd_object_array((struct object *)commit, *argv, &revs);\n+\t\tif (commit) {\n+\t\t\tif (cutoff > commit->date)\n+\t\t\t\tcutoff = commit->date;\n+\t\t}\n+\t\tadd_object_array(object, *argv, &revs);\n \t}\n \n \tif (cutoff)\n-- \n1.8.3.2-853-ga8cbcc9\n"},{"id":"222800","messageId":"51DA7DBD.7070405@alum.mit.edu","threadId":"34376","inReplyTo":"1373236424-25617-2-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 1/4] name-ref: factor out name shortening logic from name_ref()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-07-08T08:52:13Z","receivedAt":"2013-07-08T08:52:13Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 07/08/2013 12:33 AM, Junio C Hamano wrote:\n> The logic will be used in a new codepath for showing exact matches.\n> \n> Signed-off-by: Junio C Hamano \n> ---\n> builtin/name-rev.c | 19 ++++++++++++-------\n> 1 file changed, 12 insertions(+), 7 deletions(-)\n> \n> diff --git a/builtin/name-rev.c b/builtin/name-rev.c\n> index 87d4854..1234ebb 100644\n> --- a/builtin/name-rev.c\n> +++ b/builtin/name-rev.c\n> @@ -96,6 +96,17 @@ static int subpath_matches(const char *path, const char *filter)\n> \treturn -1;\n> }\n> \n> +static const char *name_ref_abbrev(const char *refname, int shorten_unambiguous)\n> +{\n> +\tif (shorten_unambiguous)\n> +\t\trefname = shorten_unambiguous_ref(refname, 0);\n> +\telse if (!prefixcmp(refname, \"refs/heads/\"))\n> +\t\trefname = refname + 11;\n> +\telse if (!prefixcmp(refname, \"refs/\"))\n> +\t\trefname = refname + 5;\n> +\treturn refname;\n> +}\n> +\n\nIn my opinion this would be a tad clearer if each of the branches of the\n\"if\" returned the value directly rather than setting refname and relying\non the \"return\" statement that follows. But it's probably a matter of\ntaste.\n\n> struct name_ref_data {\n> \tint tags_only;\n> \tint name_only;\n> @@ -134,13 +145,7 @@ static int name_ref(const char *path, const unsigned char *sha1, int flags, void\n> \tif (o && o->type == OBJ_COMMIT) {\n> \t\tstruct commit *commit = (struct commit *)o;\n> \n> -\t\tif (can_abbreviate_output)\n> -\t\t\tpath = shorten_unambiguous_ref(path, 0);\n> -\t\telse if (!prefixcmp(path, \"refs/heads/\"))\n> -\t\t\tpath = path + 11;\n> -\t\telse if (!prefixcmp(path, \"refs/\"))\n> -\t\t\tpath = path + 5;\n> -\n> +\t\tpath = name_ref_abbrev(path, can_abbreviate_output);\n> \t\tname_rev(commit, xstrdup(path), 0, 0, deref);\n> \t}\n> \treturn 0;\n> \n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"222813","messageId":"CALkWK0kSqaMrD_YuT8OxLo8yLEc0APd2Ca7FapKstqqmcYbC7A@mail.gmail.com","threadId":"34376","inReplyTo":"1373236424-25617-3-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 2/4] name-rev: allow converting the exact object name at the tip of a ref","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-07-08T12:20:16Z","receivedAt":"2013-07-08T12:20:16Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> \"git name-rev\" is supposed to convert 40-hex object names into\n> strings that name the same objects based on refs, that can be fed to\n> \"git rev-parse\" to get the same object names back, so\n>\n> $ git rev-parse v1.8.3 v1.8.3^0 | git name-rev --stdin\n> 8af06057d0c31a24e8737ae846ac2e116e8bafb9\n> edca4152560522a431a51fc0a06147fc680b5b18 (tags/v1.8.3^0)\n\nWait, what?\n\n $ git name-rev 8af060\n 8af060 tags/v1.8.3^0\n\nIsn't this a failure specific to --stdin?\n\n> Teach it to show this instead:\n>\n> $ git rev-parse v1.8.3 v1.8.3^0 | git name-rev --stdin\n> 8af06057d0c31a24e8737ae846ac2e116e8bafb9 (tags/v1.8.3)\n> edca4152560522a431a51fc0a06147fc680b5b18 (tags/v1.8.3^0)\n\nWait, what is name-rev?\n\n Finds symbolic names suitable for human digestion for revisions\n given in any format parsable by git rev-parse.\n\nIt is meant to name _revisions_ (aka. commits): in that context, what\nsense does it make to distinguish between tags/v1.8.3 and\ntags/v1.8.3^0?\n"},{"id":"222818","messageId":"CALkWK0nMmQO=cML-s9rgku+_4fZnYwtGC+daYPOBPn6-RFZj=g@mail.gmail.com","threadId":"34376","inReplyTo":"1373236424-25617-5-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 4/4] describe/name-rev: tell name-rev to peel the incoming object to commit first","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-07-08T13:08:32Z","receivedAt":"2013-07-08T13:08:32Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> With this on top of the other patches in this series, you would get:\n>\n> $ git describe --contains $(git rev-parse v1.8.3 v1.8.3^0)\n> v1.8.3\n> v1.8.3\n>\n> while you can still differentiate tags and the commits they point at\n> with:\n>\n> $ git name-rev --refs=tags/\\* --name-only $(git rev-parse v1.8.3 v1.8.3^0)\n> v1.8.3\n> v1.8.3^0\n>\n> The difference in these two behaviours is achieved by adding --peel-to-commit\n> option to \"name-rev\" and using it when \"describe\" internally calls it.\n\nEssentially a revert of [2/4] for describe-purposes, achieved by\nadding an ugly command-line option to name-rev. Before we argue any\nfurther, let me ask: who uses name-rev (and depends strongly on its\noutput)?! Our very own testsuite does not exercise it. There are\nexactly two users of describe/name-rev:\n\n1. prompt, obviously.\n2. DAG-tests, for simplification.\n\nI really can't imagine it being useful elsewhere.\n"},{"id":"222831","messageId":"7v7gh12iuk.fsf@alter.siamese.dyndns.org","threadId":"34376","inReplyTo":"51DA7DBD.7070405@alum.mit.edu","subject":"Re: [PATCH 1/4] name-ref: factor out name shortening logic from name_ref()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-08T15:04:51Z","receivedAt":"2013-07-08T15:04:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> On 07/08/2013 12:33 AM, Junio C Hamano wrote:\n>> The logic will be used in a new codepath for showing exact matches.\n>> \n>> Signed-off-by: Junio C Hamano \n>> ---\n>> builtin/name-rev.c | 19 ++++++++++++-------\n>> 1 file changed, 12 insertions(+), 7 deletions(-)\n>> \n>> diff --git a/builtin/name-rev.c b/builtin/name-rev.c\n>> index 87d4854..1234ebb 100644\n>> --- a/builtin/name-rev.c\n>> +++ b/builtin/name-rev.c\n>> @@ -96,6 +96,17 @@ static int subpath_matches(const char *path, const char *filter)\n>> \treturn -1;\n>> }\n>> \n>> +static const char *name_ref_abbrev(const char *refname, int shorten_unambiguous)\n>> +{\n>> +\tif (shorten_unambiguous)\n>> +\t\trefname = shorten_unambiguous_ref(refname, 0);\n>> +\telse if (!prefixcmp(refname, \"refs/heads/\"))\n>> +\t\trefname = refname + 11;\n>> +\telse if (!prefixcmp(refname, \"refs/\"))\n>> +\t\trefname = refname + 5;\n>> +\treturn refname;\n>> +}\n>> +\n>\n> In my opinion this would be a tad clearer if each of the branches of the\n> \"if\" returned the value directly rather than setting refname and relying\n> on the \"return\" statement that follows. But it's probably a matter of\n> taste.\n\nI tend to agree; this is a straight-forward code movement (with the\nvariable name changed to conform to your recent refs.c update to\ncall these things \"refname\"), and that was the primary reason I kept\nthem like so.\n\n>\n>> struct name_ref_data {\n>> \tint tags_only;\n>> \tint name_only;\n>> @@ -134,13 +145,7 @@ static int name_ref(const char *path, const unsigned char *sha1, int flags, void\n>> \tif (o && o->type == OBJ_COMMIT) {\n>> \t\tstruct commit *commit = (struct commit *)o;\n>> \n>> -\t\tif (can_abbreviate_output)\n>> -\t\t\tpath = shorten_unambiguous_ref(path, 0);\n>> -\t\telse if (!prefixcmp(path, \"refs/heads/\"))\n>> -\t\t\tpath = path + 11;\n>> -\t\telse if (!prefixcmp(path, \"refs/\"))\n>> -\t\t\tpath = path + 5;\n>> -\n>> +\t\tpath = name_ref_abbrev(path, can_abbreviate_output);\n>> \t\tname_rev(commit, xstrdup(path), 0, 0, deref);\n>> \t}\n>> \treturn 0;\n>> \n>\n> Michael\n"},{"id":"222832","messageId":"7v38rp2ihg.fsf@alter.siamese.dyndns.org","threadId":"34376","inReplyTo":"CALkWK0kSqaMrD_YuT8OxLo8yLEc0APd2Ca7FapKstqqmcYbC7A@mail.gmail.com","subject":"Re: [PATCH 2/4] name-rev: allow converting the exact object name at the tip of a ref","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-08T15:12:43Z","receivedAt":"2013-07-08T15:12:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> Finds symbolic names suitable for human digestion for revisions\n> given in any format parsable by git rev-parse.\n>\n> It is meant to name _revisions_ (aka. commits):\n\nThat is a mistaken documentation, written based on a half-baked\nimplementation that conflated \"the current code only can handle\ncommits\" with \"we need to only handle commit and nothing, ever\".\n\nWe had to fix a similar misguided/short-sighted implementation in\nthe \"git fetch\" codepath when adding \"git pull/merge $tag\" (the code\npeeled a tag object too early when writing FETCH_HEAD out, losing\ninformation if what we were given was a tag or a commit).\n\nI do not want to see us making the mistake worse unnecessarily.\nWhen we see a commit object that is pointed by tag v1.8.3, it is the\nright thing to do to return v1.8.3^0 as its string representation so\nthat \"git rev-parse v1.8.3^0\" give us the same thing back. name-rev\nthat is fed a tag object v1.8.3 should give v1.8.3 (not v1.8.3^0)\nback, otherwise feeding its output to \"git rev-parse\" will not give\nus the same thing we fed as the input to name-rev.\n"},{"id":"222877","messageId":"20130709045151.GE27903@sigill.intra.peff.net","threadId":"34376","inReplyTo":"1373236424-25617-4-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 3/4] describe: use argv-array","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-07-09T04:51:51Z","receivedAt":"2013-07-09T04:51:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jul 07, 2013 at 03:33:43PM -0700, Junio C Hamano wrote:\n\n> +\t\targv_array_init(&args);\n> +\t\targv_array_push(&args, \"name-rev\");\n> +\t\targv_array_push(&args, \"--name-only\");\n> +\t\targv_array_push(&args, \"--no-undefined\");\n> [...]\n> -\t\tmemcpy(args + i, argv, argc * sizeof(char *));\n> -\t\targs[i + argc] = NULL;\n> -\t\treturn cmd_name_rev(i + argc, args, prefix);\n> +\t\treturn cmd_name_rev(args.argc, args.argv, prefix);\n\nThis leaks the memory allocated by \"args\". The original did, too, and it\nis probably not that big a deal (we exit right after anyway). The fix\nwould be something like:\n\n rc = cmd_name_rev(args.argc, args.argv, prefix);\n argv_array_clear(&args);\n return rc;\n\n-Peff\n"},{"id":"222878","messageId":"20130709050615.GF27903@sigill.intra.peff.net","threadId":"34376","inReplyTo":"1373236424-25617-5-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 4/4] describe/name-rev: tell name-rev to peel the incoming object to commit first","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-07-09T05:06:15Z","receivedAt":"2013-07-09T05:06:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jul 07, 2013 at 03:33:44PM -0700, Junio C Hamano wrote:\n\n> With this on top of the other patches in this series, you would get:\n> \n> $ git describe --contains $(git rev-parse v1.8.3 v1.8.3^0)\n> v1.8.3\n> v1.8.3\n> \n> while you can still differentiate tags and the commits they point at\n> with:\n> \n> $ git name-rev --refs=tags/\\* --name-only $(git rev-parse v1.8.3 v1.8.3^0)\n> v1.8.3\n> v1.8.3^0\n> \n> The difference in these two behaviours is achieved by adding --peel-to-commit\n> option to \"name-rev\" and using it when \"describe\" internally calls it.\n\nI am somewhat mixed on this.\n\nYou are changing the default behavior of name-rev and adding a new\noption to restore it, so I wonder who (if anyone) might be broken. The\ndocumentation is now also out of date; not only does it not mention\n\"peel-to-commit\", but it claims the argument to name-rev is a\ncommittish, which is not really true without that option.\n\nOn the other hand, the new default behavior seems way more sane to me.\nIn general, I would expect name-rev to:\n\n 1. Behave more or less the same between \"git name-rev $sha1\" and \"echo\n $sha1 | git name-rev --stdin\". Your patch improves that. Though I\n note that --peel-to-commit does not affect --stdin at all. Should\n it? And of course the two differ in that the command line will take\n any rev-parse expression, and --stdin only looks for full sha1s.\n\n 2. If name-rev prints \"$X $Y\", I would expect \"git rev-parse $X\" to\n equal \"git rev-parse $Y\". With peeling, that is not the case, and\n you get the misleading example that Ram showed:\n\n $ git name-rev 8af0605\n 8af0605 tags/v1.8.3^0\n\n or more obviously weird:\n\n $ git name-rev v1.8.3\n v1.8.3 tags/v1.8.3^0\n\nSo I think your series moves in a good direction, but I would just worry\nthat it is breaking backwards compatibility (but like I said, I am not\nclear on who is affected and what it means for them).\n\n-Peff\n"},{"id":"222879","messageId":"20130709051252.GG27903@sigill.intra.peff.net","threadId":"34376","inReplyTo":"CALkWK0nMmQO=cML-s9rgku+_4fZnYwtGC+daYPOBPn6-RFZj=g@mail.gmail.com","subject":"Re: [PATCH 4/4] describe/name-rev: tell name-rev to peel the incoming object to commit first","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-07-09T05:12:52Z","receivedAt":"2013-07-09T05:12:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 08, 2013 at 06:38:32PM +0530, Ramkumar Ramachandra wrote:\n\n> Junio C Hamano wrote:\n> > With this on top of the other patches in this series, you would get:\n> >\n> > $ git describe --contains $(git rev-parse v1.8.3 v1.8.3^0)\n> > v1.8.3\n> > v1.8.3\n> >\n> > while you can still differentiate tags and the commits they point at\n> > with:\n> >\n> > $ git name-rev --refs=tags/\\* --name-only $(git rev-parse v1.8.3 v1.8.3^0)\n> > v1.8.3\n> > v1.8.3^0\n> >\n> > The difference in these two behaviours is achieved by adding --peel-to-commit\n> > option to \"name-rev\" and using it when \"describe\" internally calls it.\n> \n> Essentially a revert of [2/4] for describe-purposes, achieved by\n> adding an ugly command-line option to name-rev.\n\nI don't think it is a revert. The two patches complement each other.\n\n2/4 is basically \"if we have a non-commit object which is pointed at\ndirectly by a tip, make sure we name it by that tip\". But you can only\nget such an object by \"name-rev --stdin\", since name-rev peels its\ncommand-line arguments.\n\n4/4 is \"stop peeling command line objects, so we can find their exact\ntips\". IOW, it lets the command line do the same thing that --stdin was\nable to do in 2/4.\n\n> Before we argue any further, let me ask: who uses name-rev (and\n> depends strongly on its output)?! Our very own testsuite does not\n> exercise it. There are exactly two users of describe/name-rev:\n> \n> 1. prompt, obviously.\n> 2. DAG-tests, for simplification.\n\nYeah, I'm not clear on who we are breaking with the change in default\npeeling behavior, nor why the \"describe --contains\" wrapper wants to\nkeep it.\n\n-Peff\n"},{"id":"222881","messageId":"7va9lwxpp5.fsf@alter.siamese.dyndns.org","threadId":"34376","inReplyTo":"20130709050615.GF27903@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] describe/name-rev: tell name-rev to peel the incoming object to commit first","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-09T05:33:26Z","receivedAt":"2013-07-09T05:33:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> 1. Behave more or less the same between \"git name-rev $sha1\" and \"echo\n> $sha1 | git name-rev --stdin\". Your patch improves that. Though I\n> note that --peel-to-commit does not affect --stdin at all. Should\n> it? And of course the two differ in that the command line will take\n> any rev-parse expression, and --stdin only looks for full sha1s.\n\nTo \"Should it?\", I do not deeply care. \"--peel-to-commit\" is an\nexception that only is needed to support \"describe\".\n\nI could instead have tucked \"^0\" at the end of each argument when\n\"describe\" calls out to \"name-rev\" without adding this new option,\nwhich is much much closer to what is really going on.\n\nAnd that will alleviate your #2 below.\n\n> 2. If name-rev prints \"$X $Y\", I would expect \"git rev-parse $X\" to\n> equal \"git rev-parse $Y\". With peeling, that is not the case, and\n> you get the misleading example that Ram showed:\n>\n> $ git name-rev 8af0605\n> 8af0605 tags/v1.8.3^0\n>\n> or more obviously weird:\n>\n> $ git name-rev v1.8.3\n> v1.8.3 tags/v1.8.3^0\n>\n> So I think your series moves in a good direction, but I would just worry\n> that it is breaking backwards compatibility (but like I said, I am not\n> clear on who is affected and what it means for them).\n"},{"id":"222882","messageId":"20130709053533.GA4395@sigill.intra.peff.net","threadId":"34376","inReplyTo":"7va9lwxpp5.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 4/4] describe/name-rev: tell name-rev to peel the incoming object to commit first","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-07-09T05:35:33Z","receivedAt":"2013-07-09T05:35:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 08, 2013 at 10:33:26PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > 1. Behave more or less the same between \"git name-rev $sha1\" and \"echo\n> > $sha1 | git name-rev --stdin\". Your patch improves that. Though I\n> > note that --peel-to-commit does not affect --stdin at all. Should\n> > it? And of course the two differ in that the command line will take\n> > any rev-parse expression, and --stdin only looks for full sha1s.\n> \n> To \"Should it?\", I do not deeply care. \"--peel-to-commit\" is an\n> exception that only is needed to support \"describe\".\n> \n> I could instead have tucked \"^0\" at the end of each argument when\n> \"describe\" calls out to \"name-rev\" without adding this new option,\n> which is much much closer to what is really going on.\n\nYeah, I tend to think that is a more sane interface, even though it is a\nlittle more work in git-describe.\n\nAlthough I am still not clear on why it would not be up to the caller of\ngit-describe in the first place to decide which they wanted.\n\n-Peff\n"},{"id":"222916","messageId":"7vr4f8vtwc.fsf@alter.siamese.dyndns.org","threadId":"34376","inReplyTo":"20130709053533.GA4395@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] describe/name-rev: tell name-rev to peel the incoming object to commit first","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-09T11:45:39Z","receivedAt":"2013-07-09T11:45:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Although I am still not clear on why it would not be up to the caller of\n> git-describe in the first place to decide which they wanted.\n\nThanks for a dose of sanity.\n\nEven though the part of the miniseries that makes sure that \"X (Y)\"\noutput from \"name-rev\" always satisfies that \"rev-parse\" on X and Y\ngive the same thing is an improvement, the whole thing about\n\"describe\" is misguided and wrong, I think.\n\nIt started from the observation that these do not match:\n\n $ git describe $(git rev-parse v1.8.3)\n v1.8.3\n $ git describe --contains $(git rev-parse v1.8.3)\n v1.8.3^0\n\nand the miniseries veered in a wrong direction of \"fixing\" the\nlatter to match the former.\n\nBut the thing is, what is incosistent from the rest of the world is\nthe describe output without \"--contains\" for a commit that is\nexactly at a tag (i.e. the former), and there is no need to \"fix\"\nthis \"inconsistency\", as we see below.\n\nThe form without \"--contains\" in general reads like this:\n\n $ git describe --long $(git rev-parse v1.8.3) a717d9e\n v1.8.3-0-gedca415\n v1.8.3-2-ga717d9e\n\nThey both name a commit object, but that is sort of an afterthought;\nthe support for describe name came late at 7dd45e15 (sha1_name.c:\nunderstand \"describe\" output as a valid object name, 2006-09-20).\n\nThe primary purpose of \"git describe\" without \"--contains\" is to\ngive a string that is suitable for a version number to be embedded\nin an executable. For that purpose, \"v1.8.3\" is more convenient\nthan \"v1.8.3-0-gedca415\".\n\nBut this convenient format breaks the consistency. While any other\ndescribe name for a commmit names a commit, the output for a commit\nthat is exactly at a tag does not (in ancient times, describe output\nwere not even extended SHA-1 expressions, so this inconsistency did\nnot matter, but the \"afterthought\" brought the consistency to the\nforeground). The user chooses the convenience over the consistency\nby not using \"--long\".\n\nAnd the short form cannot be \"v1.8.3^0\" or \"v1.8.3~0\" for the sake of\n\"consistency\", as these are no more suitable as a version number\nthan a short and sweet \"v1.8.3\".\n\nThe \"--contains\" form does not even aim to come up with a pleasant\nlooking version string without using funny line noise characters, so\nit is perfectly fine for it to say:\n\n $ git describe --contains $(git rev-parse v1.8.3) a717d9e\n v1.8.3^0\n v1.8.3.1~9\n\nand these are internally consistent (they both roundtrip via\nrev-parse). Stripping \"^0\" from the former will break the\nconsistency, even though it may make the output look prettier, but\nthe \"--contains\" output is not even meant to be \"pretty\" in the\nfirst place.\n\nSo let's drop 4/4; it is breaking the system by trying to solve a\nproblem that does not exist.\n"},{"id":"222921","messageId":"CALkWK0mJCGN3SzXTBksgeDZnA=bvEi-yeLn0mT_X9Z0o805+bw@mail.gmail.com","threadId":"34376","inReplyTo":"7vr4f8vtwc.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 4/4] describe/name-rev: tell name-rev to peel the incoming object to commit first","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-07-09T12:42:14Z","receivedAt":"2013-07-09T12:42:14Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> $ git describe $(git rev-parse v1.8.3)\n> v1.8.3\n> $ git describe --contains $(git rev-parse v1.8.3)\n> v1.8.3^0\n\nThis is a correct observation, and I've already submitted the correct\nfix: \"name-rev: strip trailing ^0 in when --name-only\".\n\n> $ git describe --contains $(git rev-parse v1.8.3) a717d9e\n> v1.8.3^0\n> v1.8.3.1~9\n>\n> and these are internally consistent (they both roundtrip via\n> rev-parse). Stripping \"^0\" from the former will break the\n> consistency, even though it may make the output look prettier, but\n> the \"--contains\" output is not even meant to be \"pretty\" in the\n> first place.\n\nIncorrect. The \"--contains\" output _is_ meant to be pretty. That's\nthe whole reason name-rev --name-only was invented.\n\n[2/4] is correct in that it fixes --stdin for annotated tags (although\nthe implementation could be simpler, and the commit message is\ncompletely misleading).\n"},{"id":"222927","messageId":"7vip0jwzou.fsf@alter.siamese.dyndns.org","threadId":"34376","inReplyTo":"20130709045151.GE27903@sigill.intra.peff.net","subject":"Re: [PATCH 3/4] describe: use argv-array","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-09T14:55:13Z","receivedAt":"2013-07-09T14:55:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sun, Jul 07, 2013 at 03:33:43PM -0700, Junio C Hamano wrote:\n>\n>> +\t\targv_array_init(&args);\n>> +\t\targv_array_push(&args, \"name-rev\");\n>> +\t\targv_array_push(&args, \"--name-only\");\n>> +\t\targv_array_push(&args, \"--no-undefined\");\n>> [...]\n>> -\t\tmemcpy(args + i, argv, argc * sizeof(char *));\n>> -\t\targs[i + argc] = NULL;\n>> -\t\treturn cmd_name_rev(i + argc, args, prefix);\n>> +\t\treturn cmd_name_rev(args.argc, args.argv, prefix);\n>\n> This leaks the memory allocated by \"args\". The original did, too, and it\n> is probably not that big a deal (we exit right after anyway). The fix\n> would be something like:\n>\n> rc = cmd_name_rev(args.argc, args.argv, prefix);\n> argv_array_clear(&args);\n> return rc;\n\nYes; this was meant as a straight rewrite and I did not bother, but\nI should have cleaned it up as I meant to build on top.\n\nWill amend, even though I do not think we need to build anything on\ntop.\n"},{"id":"222933","messageId":"7vehb7wwob.fsf@alter.siamese.dyndns.org","threadId":"34376","inReplyTo":"7vip0jwzou.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/4] describe: use argv-array","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-09T16:00:20Z","receivedAt":"2013-07-09T16:00:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> On Sun, Jul 07, 2013 at 03:33:43PM -0700, Junio C Hamano wrote:\n>>\n>>> +\t\targv_array_init(&args);\n>>> +\t\targv_array_push(&args, \"name-rev\");\n>>> +\t\targv_array_push(&args, \"--name-only\");\n>>> +\t\targv_array_push(&args, \"--no-undefined\");\n>>> [...]\n>>> -\t\tmemcpy(args + i, argv, argc * sizeof(char *));\n>>> -\t\targs[i + argc] = NULL;\n>>> -\t\treturn cmd_name_rev(i + argc, args, prefix);\n>>> +\t\treturn cmd_name_rev(args.argc, args.argv, prefix);\n>>\n>> This leaks the memory allocated by \"args\". The original did, too, and it\n>> is probably not that big a deal (we exit right after anyway). The fix\n>> would be something like:\n>>\n>> rc = cmd_name_rev(args.argc, args.argv, prefix);\n>> argv_array_clear(&args);\n>> return rc;\n>\n> Yes; this was meant as a straight rewrite and I did not bother, but\n> I should have cleaned it up as I meant to build on top.\n>\n> Will amend, even though I do not think we need to build anything on\n> top.\n\nHeh, you fooled me. cmd_name_rev() uses the usual parse-options\nmachinery that updates args.argv[]. Dashed options that were\nconsumed will not remain in args.argv[] and argv_array_clear() will\nnot have a chance to free them, and besides, args.argc and args.argv\nwill be out of sync and wreaks havoc in argv_array_clear().\n\nWe could expose argv_array_push_nodup() and use it in this caller\nand then free the args.argv[] but not its contents, but I do not\nthink it is worth it.\n"},{"id":"222946","messageId":"20130709185338.GA5153@sigill.intra.peff.net","threadId":"34376","inReplyTo":"7vehb7wwob.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/4] describe: use argv-array","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-07-09T18:53:38Z","receivedAt":"2013-07-09T18:53:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 09, 2013 at 09:00:20AM -0700, Junio C Hamano wrote:\n\n> >>> +\t\treturn cmd_name_rev(args.argc, args.argv, prefix);\n> >>\n> >> This leaks the memory allocated by \"args\". The original did, too, and it\n> >> is probably not that big a deal (we exit right after anyway). The fix\n> >> would be something like:\n> >>\n> >> rc = cmd_name_rev(args.argc, args.argv, prefix);\n> >> argv_array_clear(&args);\n> >> return rc;\n> >\n> > Yes; this was meant as a straight rewrite and I did not bother, but\n> > I should have cleaned it up as I meant to build on top.\n> >\n> > Will amend, even though I do not think we need to build anything on\n> > top.\n> \n> Heh, you fooled me. cmd_name_rev() uses the usual parse-options\n> machinery that updates args.argv[]. Dashed options that were\n> consumed will not remain in args.argv[] and argv_array_clear() will\n> not have a chance to free them, and besides, args.argc and args.argv\n> will be out of sync and wreaks havoc in argv_array_clear().\n\nIck, yeah, I forgot about that. Let's just leave it as a leak, then. We\nare exiting immediately afterwards, anyway.\n\n-Peff\n"}]}