{"thread":{"id":"29557","subject":"[RFC/PATCH] tag: add --points-at list option","startedAt":"2012-02-05T22:28:07Z","lastAt":"2012-02-09T04:33:51Z","messageCount":53,"participants":["Tom Grennan","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"183934","messageId":"1328480887-27463-1-git-send-email-tmgrennan@gmail.com","threadId":"29557","inReplyTo":null,"subject":"[RFC/PATCH] tag: add --points-at list option","fromName":"Tom Grennan","fromEmail":"tmgrennan@gmail.com","sentAt":"2012-02-05T22:28:07Z","receivedAt":"2012-02-05T22:28:07Z","isPatch":true,"sender":{"key":"tmgrennan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/422771?v=4"},"body":"This filters the list for annotated|signed tags of the given object.\nExample,\n\n   john$ git tag -s v1.0-john v1.0\n   john$ git tag -l --points-at v1.0\n   v1.0-john\n\nSigned-off-by: Tom Grennan <tmgrennan@gmail.com>\n---\n Documentation/git-tag.txt |    5 +++-\n builtin/tag.c             |   59 ++++++++++++++++++++++++++++++++++++++-------\n 2 files changed, 54 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex 53ff5f6..b9ec75c 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -12,7 +12,7 @@ SYNOPSIS\n 'git tag' [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]\n \t<tagname> [<commit> | <object>]\n 'git tag' -d <tagname>...\n-'git tag' [-n[<num>]] -l [--contains <commit>] [<pattern>...]\n+'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>] [<pattern>...]\n 'git tag' -v <tagname>...\n \n DESCRIPTION\n@@ -86,6 +86,9 @@ OPTIONS\n --contains <commit>::\n \tOnly list tags which contain the specified commit.\n \n+--points-at <object>::\n+\tOnly list annotated or signed tags of the given object.\n+\n -m <msg>::\n --message=<msg>::\n \tUse the given tag message (instead of prompting).\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 31f02e8..7568d6c 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -19,7 +19,8 @@\n static const char * const git_tag_usage[] = {\n \t\"git tag [-a|-s|-u <key-id>] [-f] [-m <msg>|-F <file>] <tagname> [<head>]\",\n \t\"git tag -d <tagname>...\",\n-\t\"git tag -l [-n[<num>]] [<pattern>...]\",\n+\t\"git tag -l [-n[<num>]] [<pattern>...] \\\\\\n\\t\\t\"\n+\t\t\"[--contains <commit>] [--points-at <object>]\",\n \t\"git tag -v <tagname>...\",\n \tNULL\n };\n@@ -28,6 +29,7 @@ struct tag_filter {\n \tconst char **patterns;\n \tint lines;\n \tstruct commit_list *with_commit;\n+\tconst unsigned char *points_at;\n };\n \n static int match_pattern(const char **patterns, const char *ref)\n@@ -105,16 +107,28 @@ static int show_reference(const char *refname, const unsigned char *sha1,\n \t\t\t\treturn 0;\n \t\t}\n \n+\t\tbuf = read_sha1_file(sha1, &type, &size);\n+\t\tif (!buf || !size)\n+\t\t\treturn 0;\n+\n+\t\tif (filter->points_at) {\n+\t\t\tunsigned char tagged_sha1[20];\n+\t\t\tif (memcmp(\"object \", buf, 7) \\\n+\t\t\t    || buf[47] != '\\n' \\\n+\t\t\t    || get_sha1_hex(buf + 7, tagged_sha1) \\\n+\t\t\t    || memcmp(filter->points_at, tagged_sha1, 20)) {\n+\t\t\t\tfree(buf);\n+\t\t\t\treturn 0;\n+\t\t\t}\n+\t\t}\n+\n \t\tif (!filter->lines) {\n \t\t\tprintf(\"%s\\n\", refname);\n+\t\t\tfree(buf);\n \t\t\treturn 0;\n \t\t}\n \t\tprintf(\"%-15s \", refname);\n \n-\t\tbuf = read_sha1_file(sha1, &type, &size);\n-\t\tif (!buf || !size)\n-\t\t\treturn 0;\n-\n \t\t/* skip header */\n \t\tsp = strstr(buf, \"\\n\\n\");\n \t\tif (!sp) {\n@@ -143,16 +157,20 @@ static int show_reference(const char *refname, const unsigned char *sha1,\n }\n \n static int list_tags(const char **patterns, int lines,\n-\t\t\tstruct commit_list *with_commit)\n+\t\t\tstruct commit_list *with_commit,\n+\t\t\tunsigned char *points_at)\n {\n \tstruct tag_filter filter;\n \n \tfilter.patterns = patterns;\n \tfilter.lines = lines;\n \tfilter.with_commit = with_commit;\n+\tfilter.points_at = points_at;\n \n \tfor_each_tag_ref(show_reference, (void *) &filter);\n \n+\tif (points_at)\n+\t\tfree(points_at);\n \treturn 0;\n }\n \n@@ -375,12 +393,28 @@ static int strbuf_check_tag_ref(struct strbuf *sb, const char *name)\n \treturn check_refname_format(sb->buf, 0);\n }\n \n+int parse_opt_points_at(const struct option *opt, const char *arg, int unset)\n+{\n+\tunsigned char *sha1;\n+\n+\tif (!arg)\n+\t\treturn -1;\n+\tsha1 = xmalloc(20);\n+\tif (get_sha1(arg, sha1)) {\n+\t\tfree(sha1);\n+\t\treturn error(\"malformed object name %s\", arg);\n+\t}\n+\t*(unsigned char **)opt->value = sha1;\n+\treturn 0;\n+}\n+\n int cmd_tag(int argc, const char **argv, const char *prefix)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct strbuf ref = STRBUF_INIT;\n \tunsigned char object[20], prev[20];\n \tconst char *object_ref, *tag;\n+\tunsigned char *points_at;\n \tstruct ref_lock *lock;\n \tstruct create_tag_options opt;\n \tchar *cleanup_arg = NULL;\n@@ -417,6 +451,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\t\tPARSE_OPT_LASTARG_DEFAULT,\n \t\t\tparse_opt_with_commit, (intptr_t)\"HEAD\",\n \t\t},\n+\t\t{\n+\t\t\tOPTION_CALLBACK, 0, \"points-at\", &points_at, \"object\",\n+\t\t\t\"print only annotated|signed tags of the object\",\n+\t\t\tPARSE_OPT_LASTARG_DEFAULT,\n+\t\t\tparse_opt_points_at, (intptr_t)\"HEAD\",\n+\t\t},\n \t\tOPT_END()\n \t};\n \n@@ -443,11 +483,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tusage_with_options(git_tag_usage, options);\n \tif (list)\n \t\treturn list_tags(argv, lines == -1 ? 0 : lines,\n-\t\t\t\t with_commit);\n+\t\t\t\t with_commit, points_at);\n \tif (lines != -1)\n \t\tdie(_(\"-n option is only allowed with -l.\"));\n-\tif (with_commit)\n-\t\tdie(_(\"--contains option is only allowed with -l.\"));\n+\tif (with_commit || points_at)\n+\t\tdie(_(\"--contains and --points-at options \"\n+\t\t      \"are only allowed with -l.\"));\n \tif (delete)\n \t\treturn for_each_tag_name(argv, delete_tag);\n \tif (verify)\n-- \n1.7.8\n"},{"id":"183941","messageId":"7vvcnkeu2i.fsf@alter.siamese.dyndns.org","threadId":"29557","inReplyTo":"1328480887-27463-1-git-send-email-tmgrennan@gmail.com","subject":"Re: [RFC/PATCH] tag: add --points-at list option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-05T23:31:17Z","receivedAt":"2012-02-05T23:31:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tom Grennan <tmgrennan@gmail.com> writes:\n\n> @@ -105,16 +107,28 @@ static int show_reference(const char *refname, const unsigned char *sha1,\n>  \t\t\t\treturn 0;\n>  \t\t}\n>  \n> +\t\tbuf = read_sha1_file(sha1, &type, &size);\n> +\t\tif (!buf || !size)\n> +\t\t\treturn 0;\n> +\n> +\t\tif (filter->points_at) {\n> +\t\t\tunsigned char tagged_sha1[20];\n> +\t\t\tif (memcmp(\"object \", buf, 7) \\\n> +\t\t\t    || buf[47] != '\\n' \\\n> +\t\t\t    || get_sha1_hex(buf + 7, tagged_sha1) \\\n> +\t\t\t    || memcmp(filter->points_at, tagged_sha1, 20)) {\n\nDo we need these backslashes at the end of these lines?\n\n> @@ -143,16 +157,20 @@ static int show_reference(const char *refname, const unsigned char *sha1,\n>  }\n>  \n>  static int list_tags(const char **patterns, int lines,\n> -\t\t\tstruct commit_list *with_commit)\n> +\t\t\tstruct commit_list *with_commit,\n> +\t\t\tunsigned char *points_at)\n>  {\n\nIt strikes me somewhat odd that you can give a list of commits to filter\nwhen using \"--contains\" (e.g. \"--contains v1.7.9 --contains 1.7.8.4\"), but\nyou can only ask for a single object with \"--points-at\" from the UI point\nof view.\n\n> @@ -375,12 +393,28 @@ static int strbuf_check_tag_ref(struct strbuf *sb, const char *name)\n>  \treturn check_refname_format(sb->buf, 0);\n>  }\n>  \n> +int parse_opt_points_at(const struct option *opt, const char *arg, int unset)\n> +{\n> +\tunsigned char *sha1;\n> +\n> +\tif (!arg)\n> +\t\treturn -1;\n> +\tsha1 = xmalloc(20);\n> +\tif (get_sha1(arg, sha1)) {\n> +\t\tfree(sha1);\n> +\t\treturn error(\"malformed object name %s\", arg);\n> +\t}\n> +\t*(unsigned char **)opt->value = sha1;\n> +\treturn 0;\n> +}\n\nWe are ignoring earlier --points-at argument without telling the user that\nwe do not support more than one.\n\nWould it become too much unnecessary addition of new code if you supported\nmultiple --points-at on the command line for the sake of consistency?\n\n> @@ -417,6 +451,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n>  \t\t\tPARSE_OPT_LASTARG_DEFAULT,\n>  \t\t\tparse_opt_with_commit, (intptr_t)\"HEAD\",\n>  \t\t},\n> +\t\t{\n> +\t\t\tOPTION_CALLBACK, 0, \"points-at\", &points_at, \"object\",\n> +\t\t\t\"print only annotated|signed tags of the object\",\n> +\t\t\tPARSE_OPT_LASTARG_DEFAULT,\n> +\t\t\tparse_opt_points_at, (intptr_t)\"HEAD\",\n> +\t\t},\n\nI wonder if defaulting to HEAD even makes sense for --points-at. When you\nare chasing a bug and checked out an old version that originally had\nproblem, \"git tag --contains\" that defaults to HEAD does have a value. It\ntells us what releases are potentially contaminated with the buggy commit.\n\nBut does a similar use case support points-at that defaults to HEAD?\n\nOther than that, thanks for a pleasant read.\n"},{"id":"183944","messageId":"20120206000420.GC28735@sigill.intra.peff.net","threadId":"29557","inReplyTo":"1328480887-27463-1-git-send-email-tmgrennan@gmail.com","subject":"Re: [RFC/PATCH] tag: add --points-at list option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-06T00:04:21Z","receivedAt":"2012-02-06T00:04:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 05, 2012 at 02:28:07PM -0800, Tom Grennan wrote:\n\n> This filters the list for annotated|signed tags of the given object.\n> Example,\n> \n>    john$ git tag -s v1.0-john v1.0\n>    john$ git tag -l --points-at v1.0\n>    v1.0-john\n\nI really like this approach. One big question, and a few small comments:\n\n> +--points-at <object>::\n> +\tOnly list annotated or signed tags of the given object.\n> +\n\nIt is unclear to me from this documentation if we will only peel a\nsingle level, or if we will peel indefinitely. E.g., what will this\nshow:\n\n  $ git tag one v1.0\n  $ git tag two one\n  $ git tag --points-at=v1.0\n\nIt will clearly show \"one\", but will it also show \"two\" (from reading\nthe code, I think the answer is \"no\")? If not, should it?\n\n> +\t\tbuf = read_sha1_file(sha1, &type, &size);\n> +\t\tif (!buf || !size)\n> +\t\t\treturn 0;\n\nBefore your patch, a tag whose sha1 could not be read would get its name\nprinted, and then we would later return without printing anything more.\nNow it won't get even the first bit printed.\n\nHowever, I'm not sure the old behavior wasn't buggy; it would print part\nof the line, but never actually print the newline.\n\n> +\t\tif (filter->points_at) {\n> +\t\t\tunsigned char tagged_sha1[20];\n> +\t\t\tif (memcmp(\"object \", buf, 7) \\\n> +\t\t\t    || buf[47] != '\\n' \\\n> +\t\t\t    || get_sha1_hex(buf + 7, tagged_sha1) \\\n> +\t\t\t    || memcmp(filter->points_at, tagged_sha1, 20)) {\n> +\t\t\t\tfree(buf);\n> +\t\t\t\treturn 0;\n> +\t\t\t}\n> +\t\t}\n\nHmm, I would have expected to use parse_tag_buffer instead of doing it\nby hand. This is probably a tiny bit more efficient, but I wonder if the\ncode complexity is worth it.\n\n>  static int list_tags(const char **patterns, int lines,\n> -\t\t\tstruct commit_list *with_commit)\n> +\t\t\tstruct commit_list *with_commit,\n> +\t\t\tunsigned char *points_at)\n\nLike Junio, I was surprised this did not allow a list.\n\n-Peff\n"},{"id":"183978","messageId":"20120206054819.GB10489@tgrennan-laptop","threadId":"29557","inReplyTo":"7vvcnkeu2i.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH] tag: add --points-at list option","fromName":"Tom Grennan","fromEmail":"tmgrennan@gmail.com","sentAt":"2012-02-06T05:48:20Z","receivedAt":"2012-02-06T05:48:20Z","isPatch":true,"sender":{"key":"tmgrennan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/422771?v=4"},"body":"On Sun, Feb 05, 2012 at 03:31:17PM -0800, Junio C Hamano wrote:\n>Tom Grennan <tmgrennan@gmail.com> writes:\n>\n>> @@ -105,16 +107,28 @@ static int show_reference(const char *refname, const unsigned char *sha1,\n>>  \t\t\t\treturn 0;\n>>  \t\t}\n>>  \n>> +\t\tbuf = read_sha1_file(sha1, &type, &size);\n>> +\t\tif (!buf || !size)\n>> +\t\t\treturn 0;\n>> +\n>> +\t\tif (filter->points_at) {\n>> +\t\t\tunsigned char tagged_sha1[20];\n>> +\t\t\tif (memcmp(\"object \", buf, 7) \\\n>> +\t\t\t    || buf[47] != '\\n' \\\n>> +\t\t\t    || get_sha1_hex(buf + 7, tagged_sha1) \\\n>> +\t\t\t    || memcmp(filter->points_at, tagged_sha1, 20)) {\n>\n>Do we need these backslashes at the end of these lines?\n\nNo, just an old habit. Thanks.\n\n>> @@ -143,16 +157,20 @@ static int show_reference(const char *refname, const unsigned char *sha1,\n>>  }\n>>  \n>>  static int list_tags(const char **patterns, int lines,\n>> -\t\t\tstruct commit_list *with_commit)\n>> +\t\t\tstruct commit_list *with_commit,\n>> +\t\t\tunsigned char *points_at)\n>>  {\n>\n>It strikes me somewhat odd that you can give a list of commits to filter\n>when using \"--contains\" (e.g. \"--contains v1.7.9 --contains 1.7.8.4\"), but\n>you can only ask for a single object with \"--points-at\" from the UI point\n>of view.\n>\n>> @@ -375,12 +393,28 @@ static int strbuf_check_tag_ref(struct strbuf *sb, const char *name)\n>>  \treturn check_refname_format(sb->buf, 0);\n>>  }\n>>  \n>> +int parse_opt_points_at(const struct option *opt, const char *arg, int unset)\n>> +{\n>> +\tunsigned char *sha1;\n>> +\n>> +\tif (!arg)\n>> +\t\treturn -1;\n>> +\tsha1 = xmalloc(20);\n>> +\tif (get_sha1(arg, sha1)) {\n>> +\t\tfree(sha1);\n>> +\t\treturn error(\"malformed object name %s\", arg);\n>> +\t}\n>> +\t*(unsigned char **)opt->value = sha1;\n>> +\treturn 0;\n>> +}\n>\n>We are ignoring earlier --points-at argument without telling the user that\n>we do not support more than one.\n>\n>Would it become too much unnecessary addition of new code if you supported\n>multiple --points-at on the command line for the sake of consistency?\n\nOK, I'll implement multiple args to be consistent with contains and patterns.\n\n>> @@ -417,6 +451,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n>>  \t\t\tPARSE_OPT_LASTARG_DEFAULT,\n>>  \t\t\tparse_opt_with_commit, (intptr_t)\"HEAD\",\n>>  \t\t},\n>> +\t\t{\n>> +\t\t\tOPTION_CALLBACK, 0, \"points-at\", &points_at, \"object\",\n>> +\t\t\t\"print only annotated|signed tags of the object\",\n>> +\t\t\tPARSE_OPT_LASTARG_DEFAULT,\n>> +\t\t\tparse_opt_points_at, (intptr_t)\"HEAD\",\n>> +\t\t},\n>\n>I wonder if defaulting to HEAD even makes sense for --points-at. When you\n>are chasing a bug and checked out an old version that originally had\n>problem, \"git tag --contains\" that defaults to HEAD does have a value. It\n>tells us what releases are potentially contaminated with the buggy commit.\n>\n>But does a similar use case support points-at that defaults to HEAD?\n\nYes, the usage, \"--points-at <object>...\" implies that there is no\ndefault. So, I suppose that NULL more appropriate than \"HEAD\".\n\nShould I make the \"contains\" usage indicate that \"commit\" is optional\nlike this?\n\t\"[--contains [<commit>...]] [--points-at <object>..]\"\n\n>Other than that, thanks for a pleasant read.\n\nThanks,\nTomG\n"},{"id":"183981","messageId":"7v8vkga370.fsf@alter.siamese.dyndns.org","threadId":"29557","inReplyTo":"20120206054819.GB10489@tgrennan-laptop","subject":"Re: [RFC/PATCH] tag: add --points-at list option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-06T06:25:23Z","receivedAt":"2012-02-06T06:25:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tom Grennan <tmgrennan@gmail.com> writes:\n\n>>I wonder if defaulting to HEAD even makes sense for --points-at. When you\n>>are chasing a bug and checked out an old version that originally had\n>>problem, \"git tag --contains\" that defaults to HEAD does have a value. It\n>>tells us what releases are potentially contaminated with the buggy commit.\n>>\n>>But does a similar use case support points-at that defaults to HEAD?\n>\n> Yes, the usage, \"--points-at <object>...\" implies that there is no\n> default. So, I suppose that NULL more appropriate than \"HEAD\".\n\nThat's a circular logic.\n\nThe usage could very well say \"--points-at <object>\" and forbid missing\n<object>.  I think that would make a lot _more_ sense, because I did not\nthink of offhand any good reason that --points-at should default to HEAD\nto support some common usage, and you also seem to be unable to.\n"},{"id":"183985","messageId":"20120206063213.GC10489@tgrennan-laptop","threadId":"29557","inReplyTo":"20120206000420.GC28735@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] tag: add --points-at list option","fromName":"Tom Grennan","fromEmail":"tmgrennan@gmail.com","sentAt":"2012-02-06T06:32:13Z","receivedAt":"2012-02-06T06:32:13Z","isPatch":true,"sender":{"key":"tmgrennan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/422771?v=4"},"body":"On Sun, Feb 05, 2012 at 07:04:21PM -0500, Jeff King wrote:\n>On Sun, Feb 05, 2012 at 02:28:07PM -0800, Tom Grennan wrote:\n>\n>> This filters the list for annotated|signed tags of the given object.\n>> Example,\n>> \n>>    john$ git tag -s v1.0-john v1.0\n>>    john$ git tag -l --points-at v1.0\n>>    v1.0-john\n>\n>I really like this approach. One big question, and a few small comments:\n>\n>> +--points-at <object>::\n>> +\tOnly list annotated or signed tags of the given object.\n>> +\n>\n>It is unclear to me from this documentation if we will only peel a\n>single level, or if we will peel indefinitely. E.g., what will this\n>show:\n>\n>  $ git tag one v1.0\n>  $ git tag two one\n>  $ git tag --points-at=v1.0\n>\n>It will clearly show \"one\", but will it also show \"two\" (from reading\n>the code, I think the answer is \"no\")? If not, should it?\n\nActually, neither one nor two would be listed as these are lightweight\ntags.  In the modified example,\n\n  $ git tag -a -m One one v1.0\n  $ git tag -a -m Two two one\n  $ git tag --points-at v1.0\n  one\n  $ git tag --points-at one\n  two\n\none's object is v1.0 whereas two's object is one\n\n>> +\t\tbuf = read_sha1_file(sha1, &type, &size);\n>> +\t\tif (!buf || !size)\n>> +\t\t\treturn 0;\n>\n>Before your patch, a tag whose sha1 could not be read would get its name\n>printed, and then we would later return without printing anything more.\n>Now it won't get even the first bit printed.\n>\n>However, I'm not sure the old behavior wasn't buggy; it would print part\n>of the line, but never actually print the newline.\n\nIf you prefer, I can restore the old behavior just moving the\ncondition/return back below the refname print; then add \"buf\" qualifier\nto the following fragment and at each intermediate free.\n\n>> +\t\tif (filter->points_at) {\n>> +\t\t\tunsigned char tagged_sha1[20];\n>> +\t\t\tif (memcmp(\"object \", buf, 7) \\\n>> +\t\t\t    || buf[47] != '\\n' \\\n>> +\t\t\t    || get_sha1_hex(buf + 7, tagged_sha1) \\\n>> +\t\t\t    || memcmp(filter->points_at, tagged_sha1, 20)) {\n>> +\t\t\t\tfree(buf);\n>> +\t\t\t\treturn 0;\n>> +\t\t\t}\n>> +\t\t}\n>\n>Hmm, I would have expected to use parse_tag_buffer instead of doing it\n>by hand. This is probably a tiny bit more efficient, but I wonder if the\n>code complexity is worth it.\n\nI didn't see how to get the object sha out of parse_tag_buffer() to\ncompare with \"point_at\". The inline conditions seem simple enough.\n\n>\n>>  static int list_tags(const char **patterns, int lines,\n>> -\t\t\tstruct commit_list *with_commit)\n>> +\t\t\tstruct commit_list *with_commit,\n>> +\t\t\tunsigned char *points_at)\n>\n>Like Junio, I was surprised this did not allow a list.\n\nI agree and will change it.\n\nThanks,\nTomG\n"},{"id":"183986","messageId":"20120206064550.GE10489@tgrennan-laptop","threadId":"29557","inReplyTo":"7v8vkga370.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH] tag: add --points-at list option","fromName":"Tom Grennan","fromEmail":"tmgrennan@gmail.com","sentAt":"2012-02-06T06:45:50Z","receivedAt":"2012-02-06T06:45:50Z","isPatch":true,"sender":{"key":"tmgrennan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/422771?v=4"},"body":"On Sun, Feb 05, 2012 at 10:25:23PM -0800, Junio C Hamano wrote:\n>Tom Grennan <tmgrennan@gmail.com> writes:\n>\n>>>I wonder if defaulting to HEAD even makes sense for --points-at. When you\n>>>are chasing a bug and checked out an old version that originally had\n>>>problem, \"git tag --contains\" that defaults to HEAD does have a value. It\n>>>tells us what releases are potentially contaminated with the buggy commit.\n>>>\n>>>But does a similar use case support points-at that defaults to HEAD?\n>>\n>> Yes, the usage, \"--points-at <object>...\" implies that there is no\n>> default. So, I suppose that NULL more appropriate than \"HEAD\".\n>\n>That's a circular logic.\n>\n>The usage could very well say \"--points-at <object>\" and forbid missing\n><object>.  I think that would make a lot _more_ sense, because I did not\n>think of offhand any good reason that --points-at should default to HEAD\n>to support some common usage, and you also seem to be unable to.\n\nSorry for the miss-communication. I agreed with you - at least I thought I did.\nSo, \"--points-at <object>\" should forbid a missing <object>.\nI think I can do so by using defval = (intptr_t)NULL instead of \"HEAD\",\nright?\n\n-- \nTomG\n"},{"id":"183988","messageId":"20120206070424.GC9931@sigill.intra.peff.net","threadId":"29557","inReplyTo":"20120206063213.GC10489@tgrennan-laptop","subject":"Re: [RFC/PATCH] tag: add --points-at list option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-06T07:04:24Z","receivedAt":"2012-02-06T07:04:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 05, 2012 at 10:32:13PM -0800, Tom Grennan wrote:\n\n> >> +--points-at <object>::\n> >> +\tOnly list annotated or signed tags of the given object.\n> >> +\n> >\n> >It is unclear to me from this documentation if we will only peel a\n> >single level, or if we will peel indefinitely. E.g., what will this\n> >show:\n> >\n> >  $ git tag one v1.0\n> >  $ git tag two one\n> >  $ git tag --points-at=v1.0\n> >\n> >It will clearly show \"one\", but will it also show \"two\" (from reading\n> >the code, I think the answer is \"no\")? If not, should it?\n> \n> Actually, neither one nor two would be listed as these are lightweight\n> tags.  In the modified example,\n> \n>   $ git tag -a -m One one v1.0\n>   $ git tag -a -m Two two one\n>   $ git tag --points-at v1.0\n>   one\n>   $ git tag --points-at one\n>   two\n\nHmm. Yeah, I see that now. And re-reading your description, I see that\nit is explicit only to read from tag objects. Somehow the name\n\"points-at\" seems a bit misleading to me, then, as it implies being more\ninclusive of all pointing, including lightweight tags.\n\nI know that has nothing to do with your use-case though; I just wonder\nif there could be a better name. I can't think of one, though, and I'm\nnot sure we will ever want a more inclusive --points-at, so maybe it is\nnot worth caring about.\n\n> >Before your patch, a tag whose sha1 could not be read would get its name\n> >printed, and then we would later return without printing anything more.\n> >Now it won't get even the first bit printed.\n> >\n> >However, I'm not sure the old behavior wasn't buggy; it would print part\n> >of the line, but never actually print the newline.\n> \n> If you prefer, I can restore the old behavior just moving the\n> condition/return back below the refname print; then add \"buf\" qualifier\n> to the following fragment and at each intermediate free.\n\nThinking on it more, your behavior is at least as good as the old. And\nit only comes up in a broken repo, anyway, so trying to come up with\nsome kind of useful outcome is pointless.\n\n> >> +\t\tif (filter->points_at) {\n> >> +\t\t\tunsigned char tagged_sha1[20];\n> >> +\t\t\tif (memcmp(\"object \", buf, 7) \\\n> >> +\t\t\t    || buf[47] != '\\n' \\\n> >> +\t\t\t    || get_sha1_hex(buf + 7, tagged_sha1) \\\n> >> +\t\t\t    || memcmp(filter->points_at, tagged_sha1, 20)) {\n> >> +\t\t\t\tfree(buf);\n> >> +\t\t\t\treturn 0;\n> >> +\t\t\t}\n> >> +\t\t}\n> >\n> >Hmm, I would have expected to use parse_tag_buffer instead of doing it\n> >by hand. This is probably a tiny bit more efficient, but I wonder if the\n> >code complexity is worth it.\n> \n> I didn't see how to get the object sha out of parse_tag_buffer() to\n> compare with \"point_at\". The inline conditions seem simple enough.\n\nI think it would be:\n\n  struct tag *t = lookup_tag(sha1);\n  if (parse_tag_buffer(t, buf, size) < 0)\n          return 0; /* error, possibly should die() */\n  if (!hashcmp(filter->points_at, t->tagged.sha1))\n          /* matches */\n\nThat might bear a little bit of explanation. Git keeps a struct in\nmemory for each object, each of which contains a \"struct object\" at the\nbeginning. By calling lookup_tag, we either find an existing reference\nto the tag with this sha1, or create a new \"struct tag\". And then we\nparse it using the data we've read, storing it in the \"struct tag\" (for\nour use, or for later use). The \"tagged\" member points to the tagged\nobject. Which again is a struct object; it may or may not have been\nread and parsed, but we definitely know its sha1.\n\nIf this seems a little cumbersome, it is because the usual usage is more\nlike:\n\n  struct object *obj = parse_object(sha1);\n  if (!obj)\n          die(\"unable to read %s\", sha1_to_hex(sha1));\n  if (obj->type == OBJ_TAG) {\n          struct tag *t = (struct tag *)obj;\n          if (!hashcmp(filter->points_at, t->tagged.sha1))\n                  /* matched */\n\nAnd then you don't have to bother with calling read_sha1_file at all.\n\nBTW, writing that helped me notice two bugs in your patch:\n\n  1. You read up to 47 bytes into the buffer without ever checking\n     whether size >= 47.\n\n  2. You never check whether the object you read from read_sha1_file is\n     actually a tag.\n\nSo your patch would read random heap memory on something like:\n\n  blob=`echo foo | git hash-object --stdin -w`\n  git tag foo $blob\n\n-Peff\n"},{"id":"183990","messageId":"20120206071302.GA10447@sigill.intra.peff.net","threadId":"29557","inReplyTo":"20120206070424.GC9931@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] tag: add --points-at list option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-06T07:13:02Z","receivedAt":"2012-02-06T07:13:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 06, 2012 at 02:04:24AM -0500, Jeff King wrote:\n\n> > >Before your patch, a tag whose sha1 could not be read would get its name\n> > >printed, and then we would later return without printing anything more.\n> > >Now it won't get even the first bit printed.\n> > >\n> > >However, I'm not sure the old behavior wasn't buggy; it would print part\n> > >of the line, but never actually print the newline.\n> > \n> > If you prefer, I can restore the old behavior just moving the\n> > condition/return back below the refname print; then add \"buf\" qualifier\n> > to the following fragment and at each intermediate free.\n> \n> Thinking on it more, your behavior is at least as good as the old. And\n> it only comes up in a broken repo, anyway, so trying to come up with\n> some kind of useful outcome is pointless.\n\nSorry to reverse myself, but I just peeked at the show_reference\nfunction one more time. Unconditionally moving the buffer-reading up\nabove the \"if (!filter->lines)\" conditional is not a good idea.\n\nIf I do \"git tag -l\", right now git doesn't have to actually read and\nparse each object that has been tagged (lightweight or not). If I use\n\"git tag -n10\", then obviously we do need to read it (and we do). And if\nwe use your new \"--points-at\", we also do. But if neither of those\noptions are in use, it would be nice to avoid the object lookup (it may\nnot seem like much, but if you have a repo with an insane number of\ntags, it can add up).\n\n> BTW, writing that helped me notice two bugs in your patch:\n> \n>   1. You read up to 47 bytes into the buffer without ever checking\n>      whether size >= 47.\n> \n>   2. You never check whether the object you read from read_sha1_file is\n>      actually a tag.\n\nHmm, the \"filter->lines\" code for \"git tag -n\" makes a similar error. It\nshould probably print nothing for objects that are not tags.\n\n-Peff\n"},{"id":"183994","messageId":"20120206074558.GA24535@sigill.intra.peff.net","threadId":"29557","inReplyTo":"20120206071302.GA10447@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] tag: add --points-at list option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-06T07:45:58Z","receivedAt":"2012-02-06T07:45:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 06, 2012 at 02:13:02AM -0500, Jeff King wrote:\n\n> > BTW, writing that helped me notice two bugs in your patch:\n> > \n> >   1. You read up to 47 bytes into the buffer without ever checking\n> >      whether size >= 47.\n> > \n> >   2. You never check whether the object you read from read_sha1_file is\n> >      actually a tag.\n> \n> Hmm, the \"filter->lines\" code for \"git tag -n\" makes a similar error. It\n> should probably print nothing for objects that are not tags.\n\nUgh, this part of builtin/tag.c is riddled with small bugs. I'm\npreparing a series that will fix them, and hopefully it should make\nbuilding your points-at patch on top much more pleasant.\n\n-Peff\n"},{"id":"183996","messageId":"20120206081119.GA3939@sigill.intra.peff.net","threadId":"29557","inReplyTo":"20120206074558.GA24535@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] tag: add --points-at list option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-06T08:11:19Z","receivedAt":"2012-02-06T08:11:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 06, 2012 at 02:45:58AM -0500, Jeff King wrote:\n\n> > Hmm, the \"filter->lines\" code for \"git tag -n\" makes a similar error. It\n> > should probably print nothing for objects that are not tags.\n> \n> Ugh, this part of builtin/tag.c is riddled with small bugs. I'm\n> preparing a series that will fix them, and hopefully it should make\n> building your points-at patch on top much more pleasant.\n\nSo here's what I ended up with:\n\n  [1/3]: tag: fix output of \"tag -n\" when errors occur\n  [2/3]: tag: die when listing missing or corrupt objects\n  [3/3]: tag: don't show non-tag contents with \"-n\"\n\nI had hoped to have a 4th patch teach \"tag -n\" to use parse_object\ninstead of read_sha1_file directly. That way we could avoid reading tag\nobjects multiple times when things like \"--contains\" or \"--points-at\"\nare used.  But we don't actually cache the body of an annotated tag,\nonly its headers. So the \"tag -n\" code has to read the object fresh.\n\nI do still think it's worth using the parse_object interface for the\n\"--points-at\" feature.\n\n-Peff\n"},{"id":"183997","messageId":"20120206081312.GA3966@sigill.intra.peff.net","threadId":"29557","inReplyTo":"20120206081119.GA3939@sigill.intra.peff.net","subject":"[PATCH 1/3] tag: fix output of \"tag -n\" when errors occur","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-06T08:13:12Z","receivedAt":"2012-02-06T08:13:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When \"git tag\" is instructed to print lines from annotated\ntags via \"-n\", it first prints the tag name, then attempts\nto parse and print the lines of the tag object, and then\nfinally adds a trailing newline.\n\nIf an error occurs, we return early from the function and\nnever print the newline, screwing up the output for the next\ntag. Let's factor the line-printing into its own function so\nwe can manage the early returns better, and make sure that\nwe always terminate the line.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/tag.c |   66 +++++++++++++++++++++++++++++---------------------------\n 1 files changed, 34 insertions(+), 32 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 31f02e8..2250915 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -83,18 +83,45 @@ static int contains(struct commit *candidate, const struct commit_list *want)\n \treturn contains_recurse(candidate, want);\n }\n \n+static void show_tag_lines(const unsigned char *sha1, int lines)\n+{\n+\tint i;\n+\tunsigned long size;\n+\tenum object_type type;\n+\tchar *buf, *sp, *eol;\n+\tsize_t len;\n+\n+\tbuf = read_sha1_file(sha1, &type, &size);\n+\tif (!buf || !size)\n+\t\treturn;\n+\n+\t/* skip header */\n+\tsp = strstr(buf, \"\\n\\n\");\n+\tif (!sp) {\n+\t\tfree(buf);\n+\t\treturn;\n+\t}\n+\t/* only take up to \"lines\" lines, and strip the signature */\n+\tsize = parse_signature(buf, size);\n+\tfor (i = 0, sp += 2; i < lines && sp < buf + size; i++) {\n+\t\tif (i)\n+\t\t\tprintf(\"\\n    \");\n+\t\teol = memchr(sp, '\\n', size - (sp - buf));\n+\t\tlen = eol ? eol - sp : size - (sp - buf);\n+\t\tfwrite(sp, len, 1, stdout);\n+\t\tif (!eol)\n+\t\t\tbreak;\n+\t\tsp = eol + 1;\n+\t}\n+\tfree(buf);\n+}\n+\n static int show_reference(const char *refname, const unsigned char *sha1,\n \t\t\t  int flag, void *cb_data)\n {\n \tstruct tag_filter *filter = cb_data;\n \n \tif (match_pattern(filter->patterns, refname)) {\n-\t\tint i;\n-\t\tunsigned long size;\n-\t\tenum object_type type;\n-\t\tchar *buf, *sp, *eol;\n-\t\tsize_t len;\n-\n \t\tif (filter->with_commit) {\n \t\t\tstruct commit *commit;\n \n@@ -110,33 +137,8 @@ static int show_reference(const char *refname, const unsigned char *sha1,\n \t\t\treturn 0;\n \t\t}\n \t\tprintf(\"%-15s \", refname);\n-\n-\t\tbuf = read_sha1_file(sha1, &type, &size);\n-\t\tif (!buf || !size)\n-\t\t\treturn 0;\n-\n-\t\t/* skip header */\n-\t\tsp = strstr(buf, \"\\n\\n\");\n-\t\tif (!sp) {\n-\t\t\tfree(buf);\n-\t\t\treturn 0;\n-\t\t}\n-\t\t/* only take up to \"lines\" lines, and strip the signature */\n-\t\tsize = parse_signature(buf, size);\n-\t\tfor (i = 0, sp += 2;\n-\t\t\t\ti < filter->lines && sp < buf + size;\n-\t\t\t\ti++) {\n-\t\t\tif (i)\n-\t\t\t\tprintf(\"\\n    \");\n-\t\t\teol = memchr(sp, '\\n', size - (sp - buf));\n-\t\t\tlen = eol ? eol - sp : size - (sp - buf);\n-\t\t\tfwrite(sp, len, 1, stdout);\n-\t\t\tif (!eol)\n-\t\t\t\tbreak;\n-\t\t\tsp = eol + 1;\n-\t\t}\n+\t\tshow_tag_lines(sha1, filter->lines);\n \t\tputchar('\\n');\n-\t\tfree(buf);\n \t}\n \n \treturn 0;\n-- \n1.7.9.rc1.29.g43677\n"},{"id":"183998","messageId":"20120206081342.GB3966@sigill.intra.peff.net","threadId":"29557","inReplyTo":"20120206081119.GA3939@sigill.intra.peff.net","subject":"[PATCH 2/3] tag: die when listing missing or corrupt objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-06T08:13:42Z","receivedAt":"2012-02-06T08:13:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We don't usually bother looking at tagged objects at all\nwhen listing. However, if \"-n\" is specified, we open the\nobjects to read the annotations of the tags.  If we fail to\nread an object, or if the object has zero length, we simply\nsilently return.\n\nThe first case is an indication of a broken or corrupt repo,\nand we should notify the user of the error.\n\nThe second case is OK to silently ignore; however, the\nexisting code leaked the buffer returned by read_sha1_file.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/tag.c |    6 +++++-\n 1 files changed, 5 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 2250915..1bb42a4 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -92,8 +92,12 @@ static void show_tag_lines(const unsigned char *sha1, int lines)\n \tsize_t len;\n \n \tbuf = read_sha1_file(sha1, &type, &size);\n-\tif (!buf || !size)\n+\tif (!buf)\n+\t\tdie_errno(\"unable to read object %s\", sha1_to_hex(sha1));\n+\tif (!size) {\n+\t\tfree(buf);\n \t\treturn;\n+\t}\n \n \t/* skip header */\n \tsp = strstr(buf, \"\\n\\n\");\n-- \n1.7.9.rc1.29.g43677\n"},{"id":"183999","messageId":"20120206081456.GC3966@sigill.intra.peff.net","threadId":"29557","inReplyTo":"20120206081119.GA3939@sigill.intra.peff.net","subject":"[PATCH 3/3] tag: don't show non-tag contents with \"-n\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-06T08:14:56Z","receivedAt":"2012-02-06T08:14:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When given \"-n\", tag will show one or more lines of the body\nof an annotated tag. However, we never actually checked to\nsee that what we got was a tag; we might end up showing\nrandom lines from a lightweight-tagged blob or commit.\n\nWith this patch, we'll show lines only from tag objects (but\nstill include non-tag objects in the listing). It might make\nmore sense to omit lightweight tags from the listing\nentirely when \"-n\" is in effect. I stuck with this behavior\nbecause it is slightly more compatible with the original\nbehavior.\n\nThis might be seen as a regression for people with\nlightweight tags to commit, who would previously get the\nsubject line of the commit. The code seems to indicate that\nis not expected (since it does things like parsing off\ngpg signatures), but it's possible somebody has been relying\non it.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThe regression comment above makes me a little nervous. Still, if we\nwant to handle commits, we should do so explicitly and not munge them\nwith parse_signature. So I think it's a step in the right direction, and\nwe should let it cook for a bit and see if anybody complains.\n\n builtin/tag.c  |    2 +-\n t/t7004-tag.sh |   13 +++++++++++++\n 2 files changed, 14 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 1bb42a4..0a7c174 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -94,7 +94,7 @@ static void show_tag_lines(const unsigned char *sha1, int lines)\n \tbuf = read_sha1_file(sha1, &type, &size);\n \tif (!buf)\n \t\tdie_errno(\"unable to read object %s\", sha1_to_hex(sha1));\n-\tif (!size) {\n+\tif (!size || type != OBJ_TAG) {\n \t\tfree(buf);\n \t\treturn;\n \t}\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex e93ac73..0db0f6a 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -586,6 +586,19 @@ test_expect_success \\\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'annotations for non-tags are empty' '\n+\tblob=$(git hash-object -w --stdin <<-\\EOF\n+\tBlob paragraph 1.\n+\n+\tBlob paragraph 2.\n+\tEOF\n+\t) &&\n+\tgit tag tag-blob $blob &&\n+\techo \"tag-blob        \" >expect &&\n+\tgit tag -n1 -l tag-blob >actual &&\n+\ttest_cmp expect actual\n+'\n+\n # trying to verify annotated non-signed tags:\n \n test_expect_success GPG \\\n-- \n1.7.9.rc1.29.g43677\n"},{"id":"184000","messageId":"7vk4408ir6.fsf@alter.siamese.dyndns.org","threadId":"29557","inReplyTo":"20120206081342.GB3966@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] tag: die when listing missing or corrupt objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-06T08:32:13Z","receivedAt":"2012-02-06T08:32:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> The first case is an indication of a broken or corrupt repo,\n> and we should notify the user of the error.\n>\n> The second case is OK to silently ignore; however, the\n> existing code leaked the buffer returned by read_sha1_file.\n> ...  \n>  \tbuf = read_sha1_file(sha1, &type, &size);\n> -\tif (!buf || !size)\n> +\tif (!buf)\n> +\t\tdie_errno(\"unable to read object %s\", sha1_to_hex(sha1));\n> +\tif (!size) {\n> +\t\tfree(buf);\n>  \t\treturn;\n> +\t}\n>  \n>  \t/* skip header */\n>  \tsp = strstr(buf, \"\\n\\n\");\n\nHmm, a pedant in me says a tag object cannot have zero length, so the\nsecond case is also an indication of a corrupt repository, unless the tag\nhappens to be a lightweight one that refers directly to a blob object that\nis empty.\n\nFor that matter, shouldn't we make sure that the type is OBJ_TAG? It might\nmake sense to allow OBJ_COMMIT (i.e. lightweight tag to a commit) as well,\nbecause the definition of \"first N lines\" is compatible between tag and\ncommit for the purpose of the -n option.\n\nFor example, in the kernel repository, what would this do, I have to\nwonder:\n\n    $ git tag c2.6.12 v2.6.12^{commit}\n    $ git tag t2.6.12 v2.6.12^{tree}\n    $ git tag -l -n 12 c2.6.12 t2.6.12\n"},{"id":"184001","messageId":"20120206083407.GA9287@sigill.intra.peff.net","threadId":"29557","inReplyTo":"7vk4408ir6.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/3] tag: die when listing missing or corrupt objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-06T08:34:07Z","receivedAt":"2012-02-06T08:34:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 06, 2012 at 12:32:13AM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > The first case is an indication of a broken or corrupt repo,\n> > and we should notify the user of the error.\n> >\n> > The second case is OK to silently ignore; however, the\n> > existing code leaked the buffer returned by read_sha1_file.\n> > ...  \n> >  \tbuf = read_sha1_file(sha1, &type, &size);\n> > -\tif (!buf || !size)\n> > +\tif (!buf)\n> > +\t\tdie_errno(\"unable to read object %s\", sha1_to_hex(sha1));\n> > +\tif (!size) {\n> > +\t\tfree(buf);\n> >  \t\treturn;\n> > +\t}\n> >  \n> >  \t/* skip header */\n> >  \tsp = strstr(buf, \"\\n\\n\");\n> \n> Hmm, a pedant in me says a tag object cannot have zero length, so the\n> second case is also an indication of a corrupt repository, unless the tag\n> happens to be a lightweight one that refers directly to a blob object that\n> is empty.\n\nYes. Or alternatively, it should just be caught in the strstr() case\nbelow (which would silently ignore it).\n\n> For that matter, shouldn't we make sure that the type is OBJ_TAG? It might\n> make sense to allow OBJ_COMMIT (i.e. lightweight tag to a commit) as well,\n> because the definition of \"first N lines\" is compatible between tag and\n> commit for the purpose of the -n option.\n\nYup. See patch 3. :)\n\n-Peff\n"},{"id":"184002","messageId":"7vfweo8ikq.fsf@alter.siamese.dyndns.org","threadId":"29557","inReplyTo":"7vk4408ir6.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/3] tag: die when listing missing or corrupt objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-06T08:36:05Z","receivedAt":"2012-02-06T08:36:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Hmm, a pedant in me says a tag object cannot have zero length, so the\n> second case is also an indication of a corrupt repository, unless the tag\n> happens to be a lightweight one that refers directly to a blob object that\n> is empty.\n>\n> For that matter, shouldn't we make sure that the type is OBJ_TAG? It might\n> make sense to allow OBJ_COMMIT (i.e. lightweight tag to a commit) as well,\n> because the definition of \"first N lines\" is compatible between tag and\n> commit for the purpose of the -n option.\n\nAhh, Ok, your 3/3 addresses this exact issue.\n\nI do not object to silently return when the object is not OBJ_TAG (even\nthough I slightly prefer showing the first N lines of commit log contents\nfor OBJ_COMMIT lightweight tag), but I still think it should be warned\njust like a corruption when we see (type == OBJ_TAG && !size).\n"},{"id":"184004","messageId":"20120206083832.GA9425@sigill.intra.peff.net","threadId":"29557","inReplyTo":"7vfweo8ikq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/3] tag: die when listing missing or corrupt objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-06T08:38:32Z","receivedAt":"2012-02-06T08:38:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 06, 2012 at 12:36:05AM -0800, Junio C Hamano wrote:\n\n> > For that matter, shouldn't we make sure that the type is OBJ_TAG? It might\n> > make sense to allow OBJ_COMMIT (i.e. lightweight tag to a commit) as well,\n> > because the definition of \"first N lines\" is compatible between tag and\n> > commit for the purpose of the -n option.\n> \n> Ahh, Ok, your 3/3 addresses this exact issue.\n> \n> I do not object to silently return when the object is not OBJ_TAG (even\n> though I slightly prefer showing the first N lines of commit log contents\n> for OBJ_COMMIT lightweight tag), but I still think it should be warned\n> just like a corruption when we see (type == OBJ_TAG && !size).\n\nOK, that's easy enough to do. Should we show lightweight tags to commits\nfor backwards compatibility (and just drop the parse_signature junk in\nthat case)? The showing of blobs or trees is the really bad thing, I\nthink.\n\n-Peff\n"},{"id":"184039","messageId":"7vy5sf7s9n.fsf@alter.siamese.dyndns.org","threadId":"29557","inReplyTo":"20120206083832.GA9425@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] tag: die when listing missing or corrupt objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-06T18:04:20Z","receivedAt":"2012-02-06T18:04:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> OK, that's easy enough to do. Should we show lightweight tags to commits\n> for backwards compatibility (and just drop the parse_signature junk in\n> that case)? The showing of blobs or trees is the really bad thing, I\n> think.\n\nI think that is a sensible thing to do.  I see many end-user documents on\nthe Interweb that uses lightweight \"git tag\", and I do not think they are\nshooting for brevity of their illustration.  The authors of these pages do\nprimarily use lightweight tags because they do not have anything more to\nadd in the message more than the log message commit objects they point at.\nAnd it is a huge regression if we stop showing them if they are used to\nuse \"tag -n\".\n"},{"id":"184040","messageId":"7vty337rug.fsf@alter.siamese.dyndns.org","threadId":"29557","inReplyTo":"20120206083832.GA9425@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] tag: die when listing missing or corrupt objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-06T18:13:27Z","receivedAt":"2012-02-06T18:13:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> OK, that's easy enough to do. Should we show lightweight tags to commits\n> for backwards compatibility (and just drop the parse_signature junk in\n> that case)? The showing of blobs or trees is the really bad thing, I\n> think.\n\nFor now, dropping 3/3 and queuing this instead...\n\n---\nSubject: tag: do not show non-tag contents with \"-n\"\n\n\"git tag -n\" did not check the type of the object it is reading the top n\nlines from. At least, avoid showing the beginning of trees and blobs when\ndealing with lightweight tags that point at them.\n\nAs the payload of a tag and a commit look similar in that they both start\nwith a header block, which is skipped for the purpose of \"-n\" output,\nfollowed by human readable text, allow the message of commit objects to be\nshown just like the contents of tag objects. This avoids regression for\npeople who have been using \"tag -n\" to show the log messages of commits\nthat are pointed at by lightweight tags.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/tag.c |   22 ++++++++++++----------\n 1 file changed, 12 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 1e27f5c..6d6ae88 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -95,19 +95,20 @@ static void show_tag_lines(const unsigned char *sha1, int lines)\n \tbuf = read_sha1_file(sha1, &type, &size);\n \tif (!buf)\n \t\tdie_errno(\"unable to read object %s\", sha1_to_hex(sha1));\n-\tif (!size) {\n-\t\tfree(buf);\n-\t\treturn;\n-\t}\n+\tif (type != OBJ_COMMIT || type != OBJ_TAG)\n+\t\tgoto free_return;\n+\tif (!size)\n+\t\tdie(\"an empty %s object %s?\",\n+\t\t    typename(type), sha1_to_hex(sha1));\n \n \t/* skip header */\n \tsp = strstr(buf, \"\\n\\n\");\n-\tif (!sp) {\n-\t\tfree(buf);\n-\t\treturn;\n-\t}\n-\t/* only take up to \"lines\" lines, and strip the signature */\n-\tsize = parse_signature(buf, size);\n+\tif (!sp)\n+\t\tgoto free_return;\n+\n+\t/* only take up to \"lines\" lines, and strip the signature from a tag */\n+\tif (type == OBJ_TAG)\n+\t\tsize = parse_signature(buf, size);\n \tfor (i = 0, sp += 2; i < lines && sp < buf + size; i++) {\n \t\tif (i)\n \t\t\tprintf(\"\\n    \");\n@@ -118,6 +119,7 @@ static void show_tag_lines(const unsigned char *sha1, int lines)\n \t\t\tbreak;\n \t\tsp = eol + 1;\n \t}\n+free_return:\n \tfree(buf);\n }\n \n"},{"id":"184049","messageId":"20120206201245.GA30776@sigill.intra.peff.net","threadId":"29557","inReplyTo":"7vty337rug.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/3] tag: die when listing missing or corrupt objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-06T20:12:45Z","receivedAt":"2012-02-06T20:12:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 06, 2012 at 10:13:27AM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > OK, that's easy enough to do. Should we show lightweight tags to commits\n> > for backwards compatibility (and just drop the parse_signature junk in\n> > that case)? The showing of blobs or trees is the really bad thing, I\n> > think.\n> \n> For now, dropping 3/3 and queuing this instead...\n> \n> ---\n> Subject: tag: do not show non-tag contents with \"-n\"\n\nLooks perfect. Thanks.\n\n-Peff\n"},{"id":"184077","messageId":"7vr4y74jup.fsf@alter.siamese.dyndns.org","threadId":"29557","inReplyTo":"20120206201245.GA30776@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] tag: die when listing missing or corrupt objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-06T23:34:22Z","receivedAt":"2012-02-06T23:34:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> Subject: tag: do not show non-tag contents with \"-n\"\n>\n> Looks perfect. Thanks.\n>\n> -Peff\n\nI was an idiot and you were being too polite to point it out X-<.\n\n+\tif (type != OBJ_COMMIT || type != OBJ_TAG)\n+\t\tgoto free_return;\n\nWhen will I ever get any output from this crap?  What kind of object\nshould I craft to pass through this stupid gate? ;-)\n\nFixed and requeued.\n"},{"id":"184101","messageId":"1328598076-7773-1-git-send-email-tmgrennan@gmail.com","threadId":"29557","inReplyTo":"20120206081119.GA3939@sigill.intra.peff.net","subject":"[PATCHv2] tag: add --points-at list option","fromName":"Tom Grennan","fromEmail":"tmgrennan@gmail.com","sentAt":"2012-02-07T07:01:15Z","receivedAt":"2012-02-07T07:01:15Z","isPatch":false,"sender":{"key":"tmgrennan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/422771?v=4"},"body":"The following is version 2 of the \"points-at\" feature.  I think that I've\naddressed all of the comments on this discussion other than the name objection.\nI suggest \"of\" instead, as in: \"Show me the tags of...\".\nBut, this may be too terse, so I welcome any other suggestions.\n\nNote that this has been rebased onto pu for integration of 'jk/maint-tag-show-fixes'.\n\nThanks,\n\nTom Grennan (1):\n  tag: add --points-at list option\n\n Documentation/git-tag.txt |    5 ++-\n builtin/tag.c             |   87 ++++++++++++++++++++++++++++++++++++++++++---\n 2 files changed, 86 insertions(+), 6 deletions(-)\n\n-- \n1.7.8\n"},{"id":"184102","messageId":"1328598076-7773-2-git-send-email-tmgrennan@gmail.com","threadId":"29557","inReplyTo":"1328598076-7773-1-git-send-email-tmgrennan@gmail.com","subject":"[PATCHv2] tag: add --points-at list option","fromName":"Tom Grennan","fromEmail":"tmgrennan@gmail.com","sentAt":"2012-02-07T07:01:16Z","receivedAt":"2012-02-07T07:01:16Z","isPatch":false,"sender":{"key":"tmgrennan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/422771?v=4"},"body":"This filters the list for annotated|signed tags of the given object.\nExample,\n\n   john$ git tag -s v1.0-john v1.0\n   john$ git tag -l --points-at v1.0\n   v1.0-john\n\nSigned-off-by: Tom Grennan <tmgrennan@gmail.com>\n---\n Documentation/git-tag.txt |    5 ++-\n builtin/tag.c             |   86 ++++++++++++++++++++++++++++++++++++++++++---\n 2 files changed, 85 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex 5ead91e..97bedec 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -12,7 +12,7 @@ SYNOPSIS\n 'git tag' [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]\n \t<tagname> [<commit> | <object>]\n 'git tag' -d <tagname>...\n-'git tag' [-n[<num>]] -l [--contains <commit>]\n+'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]\n \t[--column[=<options>] | --no-column] [<pattern>...]\n 'git tag' -v <tagname>...\n \n@@ -95,6 +95,9 @@ This option is only applicable when listing tags without annotation lines.\n --contains <commit>::\n \tOnly list tags which contain the specified commit.\n \n+--points-at <object>::\n+\tOnly list annotated or signed tags of the given object.\n+\n -m <msg>::\n --message=<msg>::\n \tUse the given tag message (instead of prompting).\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 5fbd62c..a1d3a04 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -20,17 +20,34 @@\n static const char * const git_tag_usage[] = {\n \t\"git tag [-a|-s|-u <key-id>] [-f] [-m <msg>|-F <file>] <tagname> [<head>]\",\n \t\"git tag -d <tagname>...\",\n-\t\"git tag -l [-n[<num>]] [<pattern>...]\",\n+\t\"git tag -l [-n[<num>]] [--contains <commit>] [--points-at <object>] \\\\\"\n+\t\t\"\\n\\t\\t[<pattern>...]\",\n \t\"git tag -v <tagname>...\",\n \tNULL\n };\n \n+struct points_at {\n+\tstruct points_at *next;\n+\tunsigned char *sha1;\n+};\n+\n struct tag_filter {\n \tconst char **patterns;\n \tint lines;\n \tstruct commit_list *with_commit;\n+\tstruct points_at *points_at;\n };\n \n+static void free_points_at (struct points_at *points_at)\n+{\n+\twhile (points_at) {\n+\t\tstruct points_at *next = points_at->next;\n+\t\tfree(points_at->sha1);\n+\t\tfree(points_at);\n+\t\tpoints_at = next;\n+\t}\n+}\n+\n static unsigned int colopts;\n \n static int match_pattern(const char **patterns, const char *ref)\n@@ -44,6 +61,29 @@ static int match_pattern(const char **patterns, const char *ref)\n \treturn 0;\n }\n \n+static struct points_at *match_points_at(struct points_at *points_at,\n+\t\t\t\t\t const unsigned char *sha1)\n+{\n+\tchar *buf;\n+\tstruct tag *tag;\n+\tunsigned long size;\n+\tenum object_type type;\n+\n+\tbuf = read_sha1_file(sha1, &type, &size);\n+\tif (!buf)\n+\t\treturn NULL;\n+\tif (type != OBJ_TAG\n+\t    || (tag = lookup_tag(sha1), !tag)\n+\t    || parse_tag_buffer(tag, buf, size) < 0) {\n+\t\tfree(buf);\n+\t\treturn NULL;\n+\t}\n+\twhile (points_at && hashcmp(points_at->sha1, tag->tagged->sha1))\n+\t\tpoints_at = points_at->next;\n+\tfree(buf);\n+\treturn points_at;\n+}\n+\n static int in_commit_list(const struct commit_list *want, struct commit *c)\n {\n \tfor (; want; want = want->next)\n@@ -141,6 +181,10 @@ static int show_reference(const char *refname, const unsigned char *sha1,\n \t\t\t\treturn 0;\n \t\t}\n \n+\t\tif (filter->points_at\n+\t\t    && !match_points_at(filter->points_at, sha1))\n+\t\t\treturn 0;\n+\n \t\tif (!filter->lines) {\n \t\t\tprintf(\"%s\\n\", refname);\n \t\t\treturn 0;\n@@ -154,16 +198,19 @@ static int show_reference(const char *refname, const unsigned char *sha1,\n }\n \n static int list_tags(const char **patterns, int lines,\n-\t\t\tstruct commit_list *with_commit)\n+\t\t\tstruct commit_list *with_commit,\n+\t\t\tstruct points_at *points_at)\n {\n \tstruct tag_filter filter;\n \n \tfilter.patterns = patterns;\n \tfilter.lines = lines;\n \tfilter.with_commit = with_commit;\n+\tfilter.points_at = points_at;\n \n \tfor_each_tag_ref(show_reference, (void *) &filter);\n \n+\tfree_points_at(points_at);\n \treturn 0;\n }\n \n@@ -389,12 +436,33 @@ static int strbuf_check_tag_ref(struct strbuf *sb, const char *name)\n \treturn check_refname_format(sb->buf, 0);\n }\n \n+int parse_opt_points_at(const struct option *opt, const char *arg, int unset)\n+{\n+\tstruct points_at *new, **opt_value = (struct points_at **)opt->value;\n+\tunsigned char *sha1;\n+\n+\tif (!arg)\n+\t\treturn error(_(\"missing <object>\"));\n+\tnew = xmalloc(sizeof(struct points_at));\n+\tsha1 = xmalloc(20);\n+\tif (get_sha1(arg, sha1)) {\n+\t\tfree(new);\n+\t\tfree(sha1);\n+\t\treturn error(_(\"malformed object name '%s'\"), arg);\n+\t}\n+\tnew->sha1 = sha1;\n+\tnew->next = *opt_value;\n+\t*opt_value = new;\n+\treturn 0;\n+}\n+\n int cmd_tag(int argc, const char **argv, const char *prefix)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct strbuf ref = STRBUF_INIT;\n \tunsigned char object[20], prev[20];\n \tconst char *object_ref, *tag;\n+\tstruct points_at *points_at = NULL;\n \tstruct ref_lock *lock;\n \tstruct create_tag_options opt;\n \tchar *cleanup_arg = NULL;\n@@ -432,6 +500,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\t\tPARSE_OPT_LASTARG_DEFAULT,\n \t\t\tparse_opt_with_commit, (intptr_t)\"HEAD\",\n \t\t},\n+\t\t{\n+\t\t\tOPTION_CALLBACK, 0, \"points-at\", &points_at, \"object\",\n+\t\t\t\"print only annotated|signed tags of the object\",\n+\t\t\tPARSE_OPT_LASTARG_DEFAULT,\n+\t\t\tparse_opt_points_at, (intptr_t)NULL,\n+\t\t},\n \t\tOPT_END()\n \t};\n \n@@ -471,15 +545,17 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\t\tcopts.padding = 2;\n \t\t\trun_column_filter(colopts, &copts);\n \t\t}\n-\t\tret = list_tags(argv, lines == -1 ? 0 : lines, with_commit);\n+\t\tret = list_tags(argv, lines == -1 ? 0 : lines, with_commit,\n+\t\t\t\tpoints_at);\n \t\tif (lines == -1 && colopts & COL_ENABLED)\n \t\t\tstop_column_filter();\n \t\treturn ret;\n \t}\n \tif (lines != -1)\n \t\tdie(_(\"-n option is only allowed with -l.\"));\n-\tif (with_commit)\n-\t\tdie(_(\"--contains option is only allowed with -l.\"));\n+\tif (with_commit || points_at)\n+\t\tdie(_(\"--contains and --points-at options \"\n+\t\t      \"are only allowed with -l.\"));\n \tif (delete)\n \t\treturn for_each_tag_name(argv, delete_tag);\n \tif (verify)\n-- \n1.7.8\n"},{"id":"184109","messageId":"7vd39r2g8o.fsf@alter.siamese.dyndns.org","threadId":"29557","inReplyTo":"1328598076-7773-2-git-send-email-tmgrennan@gmail.com","subject":"Re: [PATCHv2] tag: add --points-at list option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-07T08:35:19Z","receivedAt":"2012-02-07T08:35:19Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tom Grennan <tmgrennan@gmail.com> writes:\n\n> +struct points_at {\n> +\tstruct points_at *next;\n> +\tunsigned char *sha1;\n> +};\n\nstruct points_at {\n\tstruct points_at *next;\n        unsigned char sha1[20];\n};\n\nwould save you from having to allocate and free always in pairs, no?\n\n> +static void free_points_at (struct points_at *points_at)\n\nPlease lose the SP before (.\n\n> +\tif (type != OBJ_TAG\n> +\t    || (tag = lookup_tag(sha1), !tag)\n> +\t    || parse_tag_buffer(tag, buf, size) < 0) {\n\nEven though I personally prefer to cascade a long expression like this, so\nthat you see a parse tree when you tilt your head 90-degrees to the left,\nI think the prevalent style in Git codebase is\n\n\tif (A-long-long-expression ||\n            B-long-long-expression ||\n            C-long-long-expression) {\n\nAlso we try to avoid assignment in the conditional.\n\n> @@ -432,6 +500,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n>  \t\t\tPARSE_OPT_LASTARG_DEFAULT,\n>  \t\t\tparse_opt_with_commit, (intptr_t)\"HEAD\",\n>  \t\t},\n> +\t\t{\n> +\t\t\tOPTION_CALLBACK, 0, \"points-at\", &points_at, \"object\",\n> +\t\t\t\"print only annotated|signed tags of the object\",\n> +\t\t\tPARSE_OPT_LASTARG_DEFAULT,\n> +\t\t\tparse_opt_points_at, (intptr_t)NULL,\n> +\t\t},\n\nIf you are going to reject NULL anyway, do you still need to mark this as\nlastarg-default?\n\nLooking for example in parse-options.h, I found this:\n\n        #define OPT_STRING_LIST(s, l, v, a, h) \\\n                    { OPTION_CALLBACK, (s), (l), (v), (a), \\\n                      (h), 0, &parse_opt_string_list }\n\nwhich is used by \"git clone\" to mark its -c option.\n\nRunning \"git clone -c\" gives me\n\n\terror: switch 'c' requires a value\n\nwithout any extra code in the caller of parse_options().\n\nOther than that, looks cleanly done.\n\nThanks. I'll take another look after I wake up in the morning.\n"},{"id":"184135","messageId":"20120207160527.GC14773@sigill.intra.peff.net","threadId":"29557","inReplyTo":"1328598076-7773-2-git-send-email-tmgrennan@gmail.com","subject":"Re: [PATCHv2] tag: add --points-at list option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-07T16:05:27Z","receivedAt":"2012-02-07T16:05:27Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 06, 2012 at 11:01:16PM -0800, Tom Grennan wrote:\n\n> +struct points_at {\n> +\tstruct points_at *next;\n> +\tunsigned char *sha1;\n> +};\n\nWould using sha1_array save us from having to create our own data\nstructure? As a bonus, it can do O(lg n) lookups, though I seriously\ndoubt anyone will provide a large number of \"--points-at\".\n\n> +static void free_points_at (struct points_at *points_at)\n> +{\n> +\twhile (points_at) {\n> +\t\tstruct points_at *next = points_at->next;\n> +\t\tfree(points_at->sha1);\n> +\t\tfree(points_at);\n> +\t\tpoints_at = next;\n> +\t}\n> +}\n\nThen this could go away in favor of sha1_array_clear.\n\n> +int parse_opt_points_at(const struct option *opt, const char *arg, int unset)\n> +{\n> +\tstruct points_at *new, **opt_value = (struct points_at **)opt->value;\n> +\tunsigned char *sha1;\n> +\n> +\tif (!arg)\n> +\t\treturn error(_(\"missing <object>\"));\n> +\tnew = xmalloc(sizeof(struct points_at));\n> +\tsha1 = xmalloc(20);\n> +\tif (get_sha1(arg, sha1)) {\n> +\t\tfree(new);\n> +\t\tfree(sha1);\n> +\t\treturn error(_(\"malformed object name '%s'\"), arg);\n> +\t}\n> +\tnew->sha1 = sha1;\n> +\tnew->next = *opt_value;\n> +\t*opt_value = new;\n> +\treturn 0;\n> +}\n\nAnd this can drop all of the memory management bits, like:\n\n  unsigned char sha1[20];\n\n  if (!arg)\n          return error(_(\"missing <object>\"));\n  if (get_sha1(arg, sha1))\n          return error(_(\"malformed object name '%s'\"), arg);\n  sha1_array_append(opt->value, sha1);\n  return 0;\n\nAlso, should you check \"unset\"? When we have options that build a list,\nusually doing \"--no-foo\" will clear the list. E.g., this:\n\n  git tag --points-at=foo --points-at=bar --no-points-at --points-at=baz\n\nshould look only for \"baz\".\n\n> +static struct points_at *match_points_at(struct points_at *points_at,\n> +\t\t\t\t\t const unsigned char *sha1)\n> +{\n> +\tchar *buf;\n> +\tstruct tag *tag;\n> +\tunsigned long size;\n> +\tenum object_type type;\n> +\n> +\tbuf = read_sha1_file(sha1, &type, &size);\n> +\tif (!buf)\n> +\t\treturn NULL;\n> +\tif (type != OBJ_TAG\n> +\t    || (tag = lookup_tag(sha1), !tag)\n> +\t    || parse_tag_buffer(tag, buf, size) < 0) {\n> +\t\tfree(buf);\n> +\t\treturn NULL;\n> +\t}\n> +\twhile (points_at && hashcmp(points_at->sha1, tag->tagged->sha1))\n> +\t\tpoints_at = points_at->next;\n> +\tfree(buf);\n> +\treturn points_at;\n> +}\n\nSorry, I threw a lot of object lookup code at you last time, so I think\nmy point may have been lost in the noise. But I think this is slightly\nnicer as:\n\n  static int tag_points_at(struct sha1_array *sa,\n                           const unsigned char *sha1)\n  {\n          struct object *obj = parse_object(sha1);\n          if (!obj)\n                  return 0; /* or probably we should even just die() */\n          if (obj->type != OBJ_TAG)\n                  return 0;\n          if (sha1_array_lookup(sa, ((struct tag *)obj)->tagged->sha1) < 0)\n                  return 0;\n          return 1;\n  }\n\nI.e., using parse_object lets you avoid dealing with memory management\nyourself. And as a bonus, it will reuse the cached information if you\nhappen to have already parsed that object (not likely in typical\nrepositories, but a huge win in certain pathological cases, like repos\nstoring shared objects and refs for a large number of forks).\n\n> +\t\t{\n> +\t\t\tOPTION_CALLBACK, 0, \"points-at\", &points_at, \"object\",\n> +\t\t\t\"print only annotated|signed tags of the object\",\n> +\t\t\tPARSE_OPT_LASTARG_DEFAULT,\n> +\t\t\tparse_opt_points_at, (intptr_t)NULL,\n> +\t\t},\n\nI think you can drop the LASTARG_DEFAULT here, as it is no longer\noptional, no?\n\n-Peff\n"},{"id":"184146","messageId":"20120207180522.GA6264@tgrennan-laptop","threadId":"29557","inReplyTo":"7vd39r2g8o.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv2] tag: add --points-at list option","fromName":"Tom Grennan","fromEmail":"tmgrennan@gmail.com","sentAt":"2012-02-07T18:05:22Z","receivedAt":"2012-02-07T18:05:22Z","isPatch":false,"sender":{"key":"tmgrennan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/422771?v=4"},"body":"On Tue, Feb 07, 2012 at 12:35:19AM -0800, Junio C Hamano wrote:\n>Tom Grennan <tmgrennan@gmail.com> writes:\n>\n>> +struct points_at {\n>> +\tstruct points_at *next;\n>> +\tunsigned char *sha1;\n>> +};\n>\n>struct points_at {\n>\tstruct points_at *next;\n>        unsigned char sha1[20];\n>};\n>\n>would save you from having to allocate and free always in pairs, no?\n\nYep\n\n>> +static void free_points_at (struct points_at *points_at)\n>\n>Please lose the SP before (.\n\nOops\n\n>> +\tif (type != OBJ_TAG\n>> +\t    || (tag = lookup_tag(sha1), !tag)\n>> +\t    || parse_tag_buffer(tag, buf, size) < 0) {\n>\n>Even though I personally prefer to cascade a long expression like this, so\n>that you see a parse tree when you tilt your head 90-degrees to the left,\n>I think the prevalent style in Git codebase is\n>\n>\tif (A-long-long-expression ||\n>            B-long-long-expression ||\n>            C-long-long-expression) {\n>\n>Also we try to avoid assignment in the conditional.\n\nI like to compact multiple conditions to a common exit but also appreciate\nthe fear and loathing of comma's.\n\nWhile rearranging this I finally understand how to include lightweight tags.\n\n\tstruct points_at *pa;\n\tconst unsigned char *tagged_sha1 = (const unsigned char *)\"\";\n\n\t/* First look for lightweight tags - those with matching sha's\n\t * but different names */\n\tfor (pa = points_at; pa; pa = pa->next)\n\t\tif (!hashcmp(pa->sha1, sha1) && strcmp(pa->refname, refname))\n\t\t\treturn pa;\n\tbuf = read_sha1_file(sha1, &type, &size);\n\tif (buf) {\n\t\tif (type == OBJ_TAG) {\n\t\t\ttag = lookup_tag(sha1);\n\t\t\tif (parse_tag_buffer(tag, buf, size) >= 0)\n\t\t\t\ttagged_sha1 = tag->tagged->sha1;\n\t\t}\n\t\tfree(buf);\n\t}\n\twhile (points_at && hashcmp(points_at->sha1, tagged_sha1))\n\t\tpoints_at = points_at->next;\n\treturn points_at;\n\nFor example,\n$ ./git-tag tomg-lw-v1.7.9 v1.7.9\n$ ./git-tag -a tomg-lw-v1.7.9 v1.7.9\n$ ./git-tag -s tomg-lw-v1.7.9 v1.7.9\n$ ./git-tag -s tomg-README HEAD:README\n$ ./git-tag -l --points-at v1.7.9 --points-at HEAD:README\ntomg-README\ntomg-annotate-v1.7.9\ntomg-lw-v1.7.9\ntomg-signed-v1.7.9\n$ ./git-tag -l --points-at v1.7.9 --points-at HEAD:README \\*v1.7.9\ntomg-annotate-v1.7.9\ntomg-lw-v1.7.9\ntomg-signed-v1.7.9\n\n>> @@ -432,6 +500,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n>>  \t\t\tPARSE_OPT_LASTARG_DEFAULT,\n>>  \t\t\tparse_opt_with_commit, (intptr_t)\"HEAD\",\n>>  \t\t},\n>> +\t\t{\n>> +\t\t\tOPTION_CALLBACK, 0, \"points-at\", &points_at, \"object\",\n>> +\t\t\t\"print only annotated|signed tags of the object\",\n>> +\t\t\tPARSE_OPT_LASTARG_DEFAULT,\n>> +\t\t\tparse_opt_points_at, (intptr_t)NULL,\n>> +\t\t},\n>\n>If you are going to reject NULL anyway, do you still need to mark this as\n>lastarg-default?\n>\n>Looking for example in parse-options.h, I found this:\n>\n>        #define OPT_STRING_LIST(s, l, v, a, h) \\\n>                    { OPTION_CALLBACK, (s), (l), (v), (a), \\\n>                      (h), 0, &parse_opt_string_list }\n>\n>which is used by \"git clone\" to mark its -c option.\n>\n>Running \"git clone -c\" gives me\n>\n>\terror: switch 'c' requires a value\n>\n>without any extra code in the caller of parse_options().\n\nCool\n\n>Other than that, looks cleanly done.\n>\n>Thanks. I'll take another look after I wake up in the morning.\n\nThanks, I'll send v3 later today.\n\n-- \nTomG\n"},{"id":"184154","messageId":"20120207190228.GB6264@tgrennan-laptop","threadId":"29557","inReplyTo":"20120207160527.GC14773@sigill.intra.peff.net","subject":"Re: [PATCHv2] tag: add --points-at list option","fromName":"Tom Grennan","fromEmail":"tmgrennan@gmail.com","sentAt":"2012-02-07T19:02:28Z","receivedAt":"2012-02-07T19:02:28Z","isPatch":false,"sender":{"key":"tmgrennan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/422771?v=4"},"body":"On Tue, Feb 07, 2012 at 11:05:27AM -0500, Jeff King wrote:\n>On Mon, Feb 06, 2012 at 11:01:16PM -0800, Tom Grennan wrote:\n>\n>> +struct points_at {\n>> +\tstruct points_at *next;\n>> +\tunsigned char *sha1;\n>> +};\n>\n>Would using sha1_array save us from having to create our own data\n>structure? As a bonus, it can do O(lg n) lookups, though I seriously\n>doubt anyone will provide a large number of \"--points-at\".\n\nThanks, but I now realize that I also need to save the pointed at\nrefname to detect lightweight tags that have matching sha's but\ndifferent names.\n\n>> +static void free_points_at (struct points_at *points_at)\n>> +{\n>> +\twhile (points_at) {\n>> +\t\tstruct points_at *next = points_at->next;\n>> +\t\tfree(points_at->sha1);\n>> +\t\tfree(points_at);\n>> +\t\tpoints_at = next;\n>> +\t}\n>> +}\n>\n>Then this could go away in favor of sha1_array_clear.\n>\n>> +int parse_opt_points_at(const struct option *opt, const char *arg, int unset)\n>> +{\n>> +\tstruct points_at *new, **opt_value = (struct points_at **)opt->value;\n>> +\tunsigned char *sha1;\n>> +\n>> +\tif (!arg)\n>> +\t\treturn error(_(\"missing <object>\"));\n>> +\tnew = xmalloc(sizeof(struct points_at));\n>> +\tsha1 = xmalloc(20);\n>> +\tif (get_sha1(arg, sha1)) {\n>> +\t\tfree(new);\n>> +\t\tfree(sha1);\n>> +\t\treturn error(_(\"malformed object name '%s'\"), arg);\n>> +\t}\n>> +\tnew->sha1 = sha1;\n>> +\tnew->next = *opt_value;\n>> +\t*opt_value = new;\n>> +\treturn 0;\n>> +}\n>\n>And this can drop all of the memory management bits, like:\n>\n>  unsigned char sha1[20];\n>\n>  if (!arg)\n>          return error(_(\"missing <object>\"));\n>  if (get_sha1(arg, sha1))\n>          return error(_(\"malformed object name '%s'\"), arg);\n>  sha1_array_append(opt->value, sha1);\n>  return 0;\n>\n>Also, should you check \"unset\"? When we have options that build a list,\n>usually doing \"--no-foo\" will clear the list. E.g., this:\n>\n>  git tag --points-at=foo --points-at=bar --no-points-at --points-at=baz\n>\n>should look only for \"baz\".\n\nAhh, so I just need to:\n\tif (unset) {\n\t\tif (*opt_value)\n\t\t\tfree_points_at(*opt_value);\n\t\t*opt_value = NULL;\n\t\treturn 0;\n\t}\n\t\n>> +static struct points_at *match_points_at(struct points_at *points_at,\n>> +\t\t\t\t\t const unsigned char *sha1)\n>> +{\n>> +\tchar *buf;\n>> +\tstruct tag *tag;\n>> +\tunsigned long size;\n>> +\tenum object_type type;\n>> +\n>> +\tbuf = read_sha1_file(sha1, &type, &size);\n>> +\tif (!buf)\n>> +\t\treturn NULL;\n>> +\tif (type != OBJ_TAG\n>> +\t    || (tag = lookup_tag(sha1), !tag)\n>> +\t    || parse_tag_buffer(tag, buf, size) < 0) {\n>> +\t\tfree(buf);\n>> +\t\treturn NULL;\n>> +\t}\n>> +\twhile (points_at && hashcmp(points_at->sha1, tag->tagged->sha1))\n>> +\t\tpoints_at = points_at->next;\n>> +\tfree(buf);\n>> +\treturn points_at;\n>> +}\n>\n>Sorry, I threw a lot of object lookup code at you last time, so I think\n>my point may have been lost in the noise. But I think this is slightly\n>nicer as:\n>\n>  static int tag_points_at(struct sha1_array *sa,\n>                           const unsigned char *sha1)\n>  {\n>          struct object *obj = parse_object(sha1);\n>          if (!obj)\n>                  return 0; /* or probably we should even just die() */\n>          if (obj->type != OBJ_TAG)\n>                  return 0;\n>          if (sha1_array_lookup(sa, ((struct tag *)obj)->tagged->sha1) < 0)\n>                  return 0;\n>          return 1;\n>  }\n>\n>I.e., using parse_object lets you avoid dealing with memory management\n>yourself. And as a bonus, it will reuse the cached information if you\n>happen to have already parsed that object (not likely in typical\n>repositories, but a huge win in certain pathological cases, like repos\n>storing shared objects and refs for a large number of forks).\n\nAye\n\n>> +\t\t{\n>> +\t\t\tOPTION_CALLBACK, 0, \"points-at\", &points_at, \"object\",\n>> +\t\t\t\"print only annotated|signed tags of the object\",\n>> +\t\t\tPARSE_OPT_LASTARG_DEFAULT,\n>> +\t\t\tparse_opt_points_at, (intptr_t)NULL,\n>> +\t\t},\n>\n>I think you can drop the LASTARG_DEFAULT here, as it is no longer\n>optional, no?\n\nYou mean flags = 0 instead of PARSE_OPT_LASTARG_DEFAULT, right?\n\nThanks,\nTomG\n"},{"id":"184155","messageId":"20120207191202.GA496@sigill.intra.peff.net","threadId":"29557","inReplyTo":"20120207190228.GB6264@tgrennan-laptop","subject":"Re: [PATCHv2] tag: add --points-at list option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-07T19:12:02Z","receivedAt":"2012-02-07T19:12:02Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 07, 2012 at 11:02:28AM -0800, Tom Grennan wrote:\n\n> >Would using sha1_array save us from having to create our own data\n> >structure? As a bonus, it can do O(lg n) lookups, though I seriously\n> >doubt anyone will provide a large number of \"--points-at\".\n> \n> Thanks, but I now realize that I also need to save the pointed at\n> refname to detect lightweight tags that have matching sha's but\n> different names.\n\nI'm not sure I understand. Wouldn't you match lightweight tags by the\nsha1 they point at? Something like:\n\n  static int tag_points_at(struct sha1_array *sa,\n                           const unsigned char *sha1)\n  {\n          struct object *obj;\n\n          /* Lightweight tag of an interesting sha1? */\n          if (sha1_array_lookup(sa, sha1) >= 0)\n                  return 1;\n\n          /* Otherwise, maybe a tag object pointing to an interesting sha1 */\n          obj = parse_object(sha1);\n          if (!obj)\n                 return 0; /* or probably we should even just die() */\n          if (obj->type != OBJ_TAG)\n                 return 0;\n          if (sha1_array_lookup(sa, ((struct tag *)obj)->tagged->sha1) < 0)\n                 return 0;\n          return 1;\n }\n\n> >Also, should you check \"unset\"? When we have options that build a list,\n> >usually doing \"--no-foo\" will clear the list. E.g., this:\n> >\n> >  git tag --points-at=foo --points-at=bar --no-points-at --points-at=baz\n> >\n> >should look only for \"baz\".\n> \n> Ahh, so I just need to:\n> \tif (unset) {\n> \t\tif (*opt_value)\n> \t\t\tfree_points_at(*opt_value);\n> \t\t*opt_value = NULL;\n> \t\treturn 0;\n> \t}\n\nYes, exactly.\n\n> >> +\t\t{\n> >> +\t\t\tOPTION_CALLBACK, 0, \"points-at\", &points_at, \"object\",\n> >> +\t\t\t\"print only annotated|signed tags of the object\",\n> >> +\t\t\tPARSE_OPT_LASTARG_DEFAULT,\n> >> +\t\t\tparse_opt_points_at, (intptr_t)NULL,\n> >> +\t\t},\n> >\n> >I think you can drop the LASTARG_DEFAULT here, as it is no longer\n> >optional, no?\n> \n> You mean flags = 0 instead of PARSE_OPT_LASTARG_DEFAULT, right?\n\nRight. Though without flags, you can probably just use the OPT_CALLBACK\nwrapper, like:\n\n  OPT_CALLBACK(0, \"points-at\", &points_at, \"object\",\n               \"print only annotated|signed tags of the object\",\n               parse_opt_points_at)\n\nNote that if you are going to handle lightweight tags, that description\nshould probably be updated.\n\n-Peff\n"},{"id":"184158","messageId":"20120207192135.GC6264@tgrennan-laptop","threadId":"29557","inReplyTo":"20120207191202.GA496@sigill.intra.peff.net","subject":"Re: [PATCHv2] tag: add --points-at list option","fromName":"Tom Grennan","fromEmail":"tmgrennan@gmail.com","sentAt":"2012-02-07T19:22:50Z","receivedAt":"2012-02-07T19:22:50Z","isPatch":false,"sender":{"key":"tmgrennan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/422771?v=4"},"body":"On Tue, Feb 07, 2012 at 02:12:02PM -0500, Jeff King wrote:\n>On Tue, Feb 07, 2012 at 11:02:28AM -0800, Tom Grennan wrote:\n>\n>> >Would using sha1_array save us from having to create our own data\n>> >structure? As a bonus, it can do O(lg n) lookups, though I seriously\n>> >doubt anyone will provide a large number of \"--points-at\".\n>> \n>> Thanks, but I now realize that I also need to save the pointed at\n>> refname to detect lightweight tags that have matching sha's but\n>> different names.\n>\n>I'm not sure I understand. Wouldn't you match lightweight tags by the\n>sha1 they point at? Something like:\n\nI think the following would show the pointed at tag too.\n  $ git tag my-v1.7.9 v1.7.9\n  $ ./git-tag -l --points-at v1.7.9\n  my-v1.7.9\n  v1.7.9\n\nvs.\n\n  $ ./git-tag -l --points-at v1.7.9\n  my-v1.7.9\n\nI found that I had to filter matching refnames.\n\n>  static int tag_points_at(struct sha1_array *sa,\n>                           const unsigned char *sha1)\n>  {\n>          struct object *obj;\n>\n>          /* Lightweight tag of an interesting sha1? */\n>          if (sha1_array_lookup(sa, sha1) >= 0)\n>                  return 1;\n>\n>          /* Otherwise, maybe a tag object pointing to an interesting sha1 */\n>          obj = parse_object(sha1);\n>          if (!obj)\n>                 return 0; /* or probably we should even just die() */\n>          if (obj->type != OBJ_TAG)\n>                 return 0;\n>          if (sha1_array_lookup(sa, ((struct tag *)obj)->tagged->sha1) < 0)\n>                 return 0;\n>          return 1;\n> }\n>\n>> >Also, should you check \"unset\"? When we have options that build a list,\n>> >usually doing \"--no-foo\" will clear the list. E.g., this:\n>> >\n>> >  git tag --points-at=foo --points-at=bar --no-points-at --points-at=baz\n>> >\n>> >should look only for \"baz\".\n>> \n>> Ahh, so I just need to:\n>> \tif (unset) {\n>> \t\tif (*opt_value)\n>> \t\t\tfree_points_at(*opt_value);\n>> \t\t*opt_value = NULL;\n>> \t\treturn 0;\n>> \t}\n>\n>Yes, exactly.\n>\n>> >> +\t\t{\n>> >> +\t\t\tOPTION_CALLBACK, 0, \"points-at\", &points_at, \"object\",\n>> >> +\t\t\t\"print only annotated|signed tags of the object\",\n>> >> +\t\t\tPARSE_OPT_LASTARG_DEFAULT,\n>> >> +\t\t\tparse_opt_points_at, (intptr_t)NULL,\n>> >> +\t\t},\n>> >\n>> >I think you can drop the LASTARG_DEFAULT here, as it is no longer\n>> >optional, no?\n>> \n>> You mean flags = 0 instead of PARSE_OPT_LASTARG_DEFAULT, right?\n>\n>Right. Though without flags, you can probably just use the OPT_CALLBACK\n>wrapper, like:\n>\n>  OPT_CALLBACK(0, \"points-at\", &points_at, \"object\",\n>               \"print only annotated|signed tags of the object\",\n>               parse_opt_points_at)\n>\n>Note that if you are going to handle lightweight tags, that description\n>should probably be updated.\n>\n>-Peff\n\n-- \nTomG\n"},{"id":"184159","messageId":"20120207193632.GC32367@sigill.intra.peff.net","threadId":"29557","inReplyTo":"20120207192135.GC6264@tgrennan-laptop","subject":"Re: [PATCHv2] tag: add --points-at list option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-07T19:36:33Z","receivedAt":"2012-02-07T19:36:33Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 07, 2012 at 11:22:50AM -0800, Tom Grennan wrote:\n\n> >> Thanks, but I now realize that I also need to save the pointed at\n> >> refname to detect lightweight tags that have matching sha's but\n> >> different names.\n> >\n> >I'm not sure I understand. Wouldn't you match lightweight tags by the\n> >sha1 they point at? Something like:\n> \n> I think the following would show the pointed at tag too.\n>   $ git tag my-v1.7.9 v1.7.9\n>   $ ./git-tag -l --points-at v1.7.9\n>   my-v1.7.9\n>   v1.7.9\n> \n> vs.\n> \n>   $ ./git-tag -l --points-at v1.7.9\n>   my-v1.7.9\n> \n> I found that I had to filter matching refnames.\n\nAh, so you are trying _not_ to show lightweight tags (I thought you\nmeant you also wanted to show them)? But I still don't see why the code\nI posted before wouldn't work in that case. The \"object\" field of v1.7.9\nis not the sha1 of the v1.7.9 tag object, but rather some commit, so it\nwould not match.\n\nMaybe I don't understand what you mean.  Can you show a test case that\nis buggy with the v2 version of the patch that you sent? I'm not sure in\nthe example above what is different between the two \"git-tag\"\ninvocations.\n\n-Peff\n"},{"id":"184164","messageId":"7v1uq61jkz.fsf@alter.siamese.dyndns.org","threadId":"29557","inReplyTo":"20120207193632.GC32367@sigill.intra.peff.net","subject":"Re: [PATCHv2] tag: add --points-at list option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-07T20:20:44Z","receivedAt":"2012-02-07T20:20:44Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> I think the following would show the pointed at tag too.\n>>   $ git tag my-v1.7.9 v1.7.9\n>>   $ ./git-tag -l --points-at v1.7.9\n>>   my-v1.7.9\n>>   v1.7.9\n>> \n>> vs.\n>> \n>>   $ ./git-tag -l --points-at v1.7.9\n>>   my-v1.7.9\n>> \n>> I found that I had to filter matching refnames.\n>\n> Ah, so you are trying _not_ to show lightweight tags (I thought you\n> meant you also wanted to show them)? But I still don't see why the code\n> I posted before wouldn't work in that case. The \"object\" field of v1.7.9\n> is not the sha1 of the v1.7.9 tag object, but rather some commit, so it\n> would not match.\n\nI think he is trying to avoid saying \"v1.7.9 points at itself\", and wants\nto know not just the value of $(rev-parse v1.7.9) but the refname.\n"},{"id":"184167","messageId":"20120207213012.GA5846@sigill.intra.peff.net","threadId":"29557","inReplyTo":"7v1uq61jkz.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv2] tag: add --points-at list option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-07T21:30:12Z","receivedAt":"2012-02-07T21:30:12Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 07, 2012 at 12:20:44PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> >> I think the following would show the pointed at tag too.\n> >>   $ git tag my-v1.7.9 v1.7.9\n> >>   $ ./git-tag -l --points-at v1.7.9\n> >>   my-v1.7.9\n> >>   v1.7.9\n> >> \n> >> vs.\n> >> \n> >>   $ ./git-tag -l --points-at v1.7.9\n> >>   my-v1.7.9\n> >> \n> >> I found that I had to filter matching refnames.\n> >\n> > Ah, so you are trying _not_ to show lightweight tags (I thought you\n> > meant you also wanted to show them)? But I still don't see why the code\n> > I posted before wouldn't work in that case. The \"object\" field of v1.7.9\n> > is not the sha1 of the v1.7.9 tag object, but rather some commit, so it\n> > would not match.\n> \n> I think he is trying to avoid saying \"v1.7.9 points at itself\", and wants\n> to know not just the value of $(rev-parse v1.7.9) but the refname.\n\nHmm. I read his example again, and now I'm even more confused.\n\nIf I give an object name to --points-at, should or should not a\nlightweight tag pointing to that object be found?\n\nIf not, then I don't see how \"git tag --points-at v1.7.9\" would find\nv1.7.9. Because we would use get_sha1 to parse \"v1.7.9\", returning the\nsha1 of the tag object. And then when trying to match, we would look at\neach tag object, find its \"object\" line, and compare that. In the case\nof considering whether to show the v1.7.9 tag, we would be comparing the\nsha1 of the commit that it points to to the actual tag sha1 itself, and\nnot match.\n\nBut in that case, nor would we match \"my-v1.7.9\" above, as it is a\nlightweight tag that also points to v1.7.9's tag object.\n\nIf we _do_ want to match lightweight tags, then in the matching phase we\nlook for both the sha1 contained in the tag ref, as well as the sha1 of\nthe thing the tag points to (_if_ it is a tag object). In that case, we\nwould find both v1.7.9 and my-v1.7.9.\n\nSo I am not sure which is preferable. But I don't see how you could or\nwould want to distinguish the two tags above. They are functionally\nidentical, in that they are both refs pointing to the exact same tag\nobject. If the example had started with \"git tag -s my-v1.7.9 v1.7.9\"\nthen it would make more sense to me.\n\n-Peff\n"},{"id":"184168","messageId":"20120207220806.GD6264@tgrennan-laptop","threadId":"29557","inReplyTo":"20120207213012.GA5846@sigill.intra.peff.net","subject":"Re: [PATCHv2] tag: add --points-at list option","fromName":"Tom Grennan","fromEmail":"tmgrennan@gmail.com","sentAt":"2012-02-07T22:08:06Z","receivedAt":"2012-02-07T22:08:06Z","isPatch":false,"sender":{"key":"tmgrennan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/422771?v=4"},"body":"On Tue, Feb 07, 2012 at 04:30:12PM -0500, Jeff King wrote:\n>On Tue, Feb 07, 2012 at 12:20:44PM -0800, Junio C Hamano wrote:\n>\n>> Jeff King <peff@peff.net> writes:\n>> \n>> >> I think the following would show the pointed at tag too.\n>> >>   $ git tag my-v1.7.9 v1.7.9\n>> >>   $ ./git-tag -l --points-at v1.7.9\n>> >>   my-v1.7.9\n>> >>   v1.7.9\n>> >> \n>> >> vs.\n>> >> \n>> >>   $ ./git-tag -l --points-at v1.7.9\n>> >>   my-v1.7.9\n>> >> \n>> >> I found that I had to filter matching refnames.\n>> >\n>> > Ah, so you are trying _not_ to show lightweight tags (I thought you\n>> > meant you also wanted to show them)? But I still don't see why the code\n>> > I posted before wouldn't work in that case. The \"object\" field of v1.7.9\n>> > is not the sha1 of the v1.7.9 tag object, but rather some commit, so it\n>> > would not match.\n>> \n>> I think he is trying to avoid saying \"v1.7.9 points at itself\", and wants\n>> to know not just the value of $(rev-parse v1.7.9) but the refname.\n>\n>Hmm. I read his example again, and now I'm even more confused.\n>\n>If I give an object name to --points-at, should or should not a\n>lightweight tag pointing to that object be found?\n>\n>If not, then I don't see how \"git tag --points-at v1.7.9\" would find\n>v1.7.9. Because we would use get_sha1 to parse \"v1.7.9\", returning the\n>sha1 of the tag object. And then when trying to match, we would look at\n>each tag object, find its \"object\" line, and compare that. In the case\n>of considering whether to show the v1.7.9 tag, we would be comparing the\n>sha1 of the commit that it points to to the actual tag sha1 itself, and\n>not match.\n>\n>But in that case, nor would we match \"my-v1.7.9\" above, as it is a\n>lightweight tag that also points to v1.7.9's tag object.\n>\n>If we _do_ want to match lightweight tags, then in the matching phase we\n>look for both the sha1 contained in the tag ref, as well as the sha1 of\n>the thing the tag points to (_if_ it is a tag object). In that case, we\n>would find both v1.7.9 and my-v1.7.9.\n>\n>So I am not sure which is preferable. But I don't see how you could or\n>would want to distinguish the two tags above. They are functionally\n>identical, in that they are both refs pointing to the exact same tag\n>object. If the example had started with \"git tag -s my-v1.7.9 v1.7.9\"\n>then it would make more sense to me.\n\nv1 and v2 wouldn't list lightweight tags of the points-at objects.\nBoth versions behave like this:\n  $ git tag my-lw-v1.7.9 v1.7.9\n  $ git tag my-a-v1.7.9 v1.7.9\n  $ git tag my-s-v1.7.9 v1.7.9\n  $ git tag -l --points-at v1.7.9\n  my-a-v1.7.9\n  my-s-v1.7.9\n\nWhile addressing Junio's comments I realized that by first matching the\nsha's and not refnames like the following will show LW tags too.\nSo, v3 will act like this:\n\n  $ git tag my-lw-v1.7.9 v1.7.9\n  $ git tag my-a-v1.7.9 v1.7.9\n  $ git tag my-s-v1.7.9 v1.7.9\n  $ git tag -l --points-at v1.7.9\n  my-lw-v1.7.9\n  my-a-v1.7.9\n  my-s-v1.7.9\n\nNote, w/o strcmp(pa->refname, refname), this shows the points-at too:\n\n  $ git tag my-lw-v1.7.9 v1.7.9\n  $ git tag my-a-v1.7.9 v1.7.9\n  $ git tag my-s-v1.7.9 v1.7.9\n  $ git tag -l --points-at v1.7.9\n  my-lw-v1.7.9\n  my-a-v1.7.9\n  my-s-v1.7.9\n  v1.7.9\n\nWhich I don't think we'd want.\n\nstatic struct points_at *match_points_at(struct points_at *points_at,\n\t\t\t\t\t const char *refname,\n\t\t\t\t\t const unsigned char *sha1)\n{\n\tstruct object *obj;\n\tstruct points_at *pa;\n\tconst unsigned char *tagged_sha1;\n\n\t/* First look for lightweight tags - those with matching sha's\n\t * but different names */\n\tfor (pa = points_at; pa; pa = pa->next)\n\t\tif (!hashcmp(pa->sha1, sha1) && strcmp(pa->refname, refname))\n\t\t\treturn pa;\n\tobj = parse_object(sha1);\n\tif (!obj || obj->type != OBJ_TAG)\n\t\treturn 0;\n\ttagged_sha1 = ((struct tag *)obj)->tagged->sha1;\n\twhile (points_at && hashcmp(points_at->sha1, tagged_sha1))\n\t\tpoints_at = points_at->next;\n\treturn points_at;\n}\n\n-- \nTomG\n"},{"id":"184169","messageId":"20120208002554.GA6035@sigill.intra.peff.net","threadId":"29557","inReplyTo":"20120207220806.GD6264@tgrennan-laptop","subject":"Re: [PATCHv2] tag: add --points-at list option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-08T00:25:54Z","receivedAt":"2012-02-08T00:25:54Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 07, 2012 at 02:08:06PM -0800, Tom Grennan wrote:\n\n> v1 and v2 wouldn't list lightweight tags of the points-at objects.\n> Both versions behave like this:\n>   $ git tag my-lw-v1.7.9 v1.7.9\n>   $ git tag my-a-v1.7.9 v1.7.9\n>   $ git tag my-s-v1.7.9 v1.7.9\n>   $ git tag -l --points-at v1.7.9\n>   my-a-v1.7.9\n>   my-s-v1.7.9\n\nI assume the 2nd and 3rd line should be:\n\n  $ git tag -a my-a-v1.7.9 v1.7.9\n  $ git tag -s my-s-v1.7.9 v1.7.9\n\n> static struct points_at *match_points_at(struct points_at *points_at,\n> \t\t\t\t\t const char *refname,\n> \t\t\t\t\t const unsigned char *sha1)\n> {\n> \tstruct object *obj;\n> \tstruct points_at *pa;\n> \tconst unsigned char *tagged_sha1;\n> \n> \t/* First look for lightweight tags - those with matching sha's\n> \t * but different names */\n> \tfor (pa = points_at; pa; pa = pa->next)\n> \t\tif (!hashcmp(pa->sha1, sha1) && strcmp(pa->refname, refname))\n> \t\t\treturn pa;\n\nOK, I see what you are trying to accomplish here. But I really don't\nlike it. Two complaints:\n\n  1. Why is the name of the tag relevant? That is, if you are interested\n     in lightweight tags, and you have two tag refs, \"refs/tags/a\" and\n     \"refs/tags/b\", both pointing to the same tag object, then in what\n     situation is it useful to show \"a\" but not \"b\"?\n\n     It seems to me you would either want lightweight tags or not. And I\n     thought not, because the point of this was to reveal signatures or\n     annotations about a tag. Your my-lw-v1.7.9 says neither. Why do we\n     want to show it?\n\n     Also, it's not symmetric. What if I say \"git tag\n     --points-at=my-lw-v1.7.9\"? Then I would get your signed and\n     annotated tags (even though they're _not_ saying anything about\n     ny-lw-v1.7.9), and I would get v1.7.9 (even though it's not saying\n     anything about it either; in fact, it's the opposite!).\n\n  2. I thought --points-at was about providing an object name. But it's\n     not. It's about providing a particular string. So with this code,\n     \"git tag --points-at=v1.7.9\" and \"git tag --points-at=$(git\n     rev-parse v1.7.9)\" are two different things. Which seems odd and\n     un-git-like to me.\n\n     Your documentation says \"Only list annotated or signed tags of the\n     given object\", which implies to me that --points-at is an arbitrary\n     object specifier, not a specific tagname.\n\nIt seems like your rationale is just avoiding a mention of v1.7.9\nbecause, hey, it was obviously on the command line and the user isn't\ninterested in it. But I don't think that's true. The user asked for\nevery tag pointing to v1.7.9's object, and v1.7.9 is such a tag. It is\nno more or less true for v1.7.9 than it is for my-lw-v1.7.9.\n\n-Peff\n"},{"id":"184171","messageId":"20120208014515.GE6264@tgrennan-laptop","threadId":"29557","inReplyTo":"20120208002554.GA6035@sigill.intra.peff.net","subject":"Re: [PATCHv2] tag: add --points-at list option","fromName":"Tom Grennan","fromEmail":"tmgrennan@gmail.com","sentAt":"2012-02-08T01:45:15Z","receivedAt":"2012-02-08T01:45:15Z","isPatch":false,"sender":{"key":"tmgrennan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/422771?v=4"},"body":"On Tue, Feb 07, 2012 at 07:25:54PM -0500, Jeff King wrote:\n>On Tue, Feb 07, 2012 at 02:08:06PM -0800, Tom Grennan wrote:\n>\n>> v1 and v2 wouldn't list lightweight tags of the points-at objects.\n>> Both versions behave like this:\n>>   $ git tag my-lw-v1.7.9 v1.7.9\n>>   $ git tag my-a-v1.7.9 v1.7.9\n>>   $ git tag my-s-v1.7.9 v1.7.9\n>>   $ git tag -l --points-at v1.7.9\n>>   my-a-v1.7.9\n>>   my-s-v1.7.9\n>\n>I assume the 2nd and 3rd line should be:\n>\n>  $ git tag -a my-a-v1.7.9 v1.7.9\n>  $ git tag -s my-s-v1.7.9 v1.7.9\n\nYes\n\n>> static struct points_at *match_points_at(struct points_at *points_at,\n>> \t\t\t\t\t const char *refname,\n>> \t\t\t\t\t const unsigned char *sha1)\n>> {\n>> \tstruct object *obj;\n>> \tstruct points_at *pa;\n>> \tconst unsigned char *tagged_sha1;\n>> \n>> \t/* First look for lightweight tags - those with matching sha's\n>> \t * but different names */\n>> \tfor (pa = points_at; pa; pa = pa->next)\n>> \t\tif (!hashcmp(pa->sha1, sha1) && strcmp(pa->refname, refname))\n>> \t\t\treturn pa;\n>\n>OK, I see what you are trying to accomplish here. But I really don't\n>like it. Two complaints:\n>\n>  1. Why is the name of the tag relevant? That is, if you are interested\n>     in lightweight tags, and you have two tag refs, \"refs/tags/a\" and\n>     \"refs/tags/b\", both pointing to the same tag object, then in what\n>     situation is it useful to show \"a\" but not \"b\"?\n\nYes, I suppose this is more \"tags or aliases of <object>\" rather than\n\"tags that point at <object>\".\n\n>     It seems to me you would either want lightweight tags or not. And I\n>     thought not, because the point of this was to reveal signatures or\n>     annotations about a tag. Your my-lw-v1.7.9 says neither. Why do we\n>     want to show it?\n\nInitially I didn't care about listing these lightweight tags (aliases)\nbut now I see that this could be useful to find turds in refs/tags.\n  $ git tag my-v.1.7.9 v1.7.9\n  ...\n  $ git tag -l --points-at v1.7.9\n  my-v.1.7.9\nOops\n\n>     Also, it's not symmetric. What if I say \"git tag\n>     --points-at=my-lw-v1.7.9\"? Then I would get your signed and\n>     annotated tags (even though they're _not_ saying anything about\n>     ny-lw-v1.7.9), and I would get v1.7.9 (even though it's not saying\n>     anything about it either; in fact, it's the opposite!).\n\nHuh?  As you noted, the lightweight tag is just an alternate reference,\nso why wouldn't want to see the annotated and signed tags of that common\nobject?\n\n  $ ./git-tag -l --points-at tomg-lw-v1.7.9 \n  tomg-annotate-v1.7.9\n  tomg-signed-v1.7.9\n  v1.7.9\n  $ ./git-tag -l --points-at v1.7.9 \n  tomg-annotate-v1.7.9\n  tomg-lw-v1.7.9\n  tomg-signed-v1.7.9\n\n>  2. I thought --points-at was about providing an object name. But it's\n>     not. It's about providing a particular string. So with this code,\n>     \"git tag --points-at=v1.7.9\" and \"git tag --points-at=$(git\n>     rev-parse v1.7.9)\" are two different things. Which seems odd and\n>     un-git-like to me.\n\nYep,\n  $ ./git-tag -l --points-at $(git rev-parse v1.7.9)\n  tomg-annotate-v1.7.9\n  tomg-lw-v1.7.9\n  tomg-signed-v1.7.9\n  v1.7.9\n\n>     Your documentation says \"Only list annotated or signed tags of the\n>     given object\", which implies to me that --points-at is an arbitrary\n>     object specifier, not a specific tagname.\n\nYes, I changed that in the patch that I've prepared but will revert this\nif you'd rather not list these lightweight tags.\n\n>It seems like your rationale is just avoiding a mention of v1.7.9\n>because, hey, it was obviously on the command line and the user isn't\n>interested in it.\n\nYes, exactly.\n\n>But I don't think that's true. The user asked for every tag pointing to\n>v1.7.9's object, and v1.7.9 is such a tag. It is no more or less true\n>for v1.7.9 than it is for my-lw-v1.7.9.\n\nMy reaction when I tested this was, \"don't tell me what I already know.\"\nBut consistency with $(git rev-parse ...) seems more important.\nAnd as you noted, a sha1_array would save code and to me, less code is\nalways better.\n\nThanks,\nTomG\n"},{"id":"184175","messageId":"1328682076-23380-1-git-send-email-tmgrennan@gmail.com","threadId":"29557","inReplyTo":"20120208002554.GA6035@sigill.intra.peff.net","subject":"[PATCHv3] tag: add --points-at list option","fromName":"Tom Grennan","fromEmail":"tmgrennan@gmail.com","sentAt":"2012-02-08T06:21:15Z","receivedAt":"2012-02-08T06:21:15Z","isPatch":false,"sender":{"key":"tmgrennan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/422771?v=4"},"body":"Please see version 3 of the \"points-at\" feature.  In addition to addressing\nthe comments on v2, this now lists lightweight tags to the given object.\n\nTom Grennan (1):\n  tag: add --points-at list option\n\n Documentation/git-tag.txt |    5 +++-\n builtin/tag.c             |   50 ++++++++++++++++++++++++++++++++++++++++++--\n 2 files changed, 51 insertions(+), 4 deletions(-)\n\n-- \n1.7.8\n"},{"id":"184176","messageId":"1328682076-23380-2-git-send-email-tmgrennan@gmail.com","threadId":"29557","inReplyTo":"1328682076-23380-1-git-send-email-tmgrennan@gmail.com","subject":"[PATCHv3] tag: add --points-at list option","fromName":"Tom Grennan","fromEmail":"tmgrennan@gmail.com","sentAt":"2012-02-08T06:21:16Z","receivedAt":"2012-02-08T06:21:16Z","isPatch":false,"sender":{"key":"tmgrennan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/422771?v=4"},"body":"This filters the list for tags of the given object.\nExample,\n\n   john$ git tag v1.0-john v1.0\n   john$ git tag -l --points-at v1.0\n   v1.0-john\n\nSigned-off-by: Tom Grennan <tmgrennan@gmail.com>\n---\n Documentation/git-tag.txt |    5 +++-\n builtin/tag.c             |   50 ++++++++++++++++++++++++++++++++++++++++++--\n 2 files changed, 51 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex 5ead91e..124ed36 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -12,7 +12,7 @@ SYNOPSIS\n 'git tag' [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]\n \t<tagname> [<commit> | <object>]\n 'git tag' -d <tagname>...\n-'git tag' [-n[<num>]] -l [--contains <commit>]\n+'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]\n \t[--column[=<options>] | --no-column] [<pattern>...]\n 'git tag' -v <tagname>...\n \n@@ -95,6 +95,9 @@ This option is only applicable when listing tags without annotation lines.\n --contains <commit>::\n \tOnly list tags which contain the specified commit.\n \n+--points-at <object>::\n+\tOnly list tags of the given object.\n+\n -m <msg>::\n --message=<msg>::\n \tUse the given tag message (instead of prompting).\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 5fbd62c..c5da622 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -16,11 +16,13 @@\n #include \"revision.h\"\n #include \"gpg-interface.h\"\n #include \"column.h\"\n+#include \"sha1-array.h\"\n \n static const char * const git_tag_usage[] = {\n \t\"git tag [-a|-s|-u <key-id>] [-f] [-m <msg>|-F <file>] <tagname> [<head>]\",\n \t\"git tag -d <tagname>...\",\n-\t\"git tag -l [-n[<num>]] [<pattern>...]\",\n+\t\"git tag -l [-n[<num>]] [--contains <commit>] [--points-at <object>] \\\\\"\n+\t\t\"\\n\\t\\t[<pattern>...]\",\n \t\"git tag -v <tagname>...\",\n \tNULL\n };\n@@ -31,6 +33,7 @@ struct tag_filter {\n \tstruct commit_list *with_commit;\n };\n \n+static struct sha1_array points_at;\n static unsigned int colopts;\n \n static int match_pattern(const char **patterns, const char *ref)\n@@ -44,6 +47,22 @@ static int match_pattern(const char **patterns, const char *ref)\n \treturn 0;\n }\n \n+static const unsigned char *match_points_at(const unsigned char *sha1)\n+{\n+\tint i;\n+\tconst unsigned char *tagged_sha1 = (unsigned char*)\"\";\n+\tstruct object *obj = parse_object(sha1);\n+\n+\tif (obj && obj->type == OBJ_TAG)\n+\t\ttagged_sha1 = ((struct tag *)obj)->tagged->sha1;\n+\tfor (i = 0; i < points_at.nr; i++)\n+\t\tif (!hashcmp(points_at.sha1[i], sha1))\n+\t\t\treturn sha1;\n+\t\telse if (!hashcmp(points_at.sha1[i], tagged_sha1))\n+\t\t\treturn tagged_sha1;\n+\treturn NULL;\n+}\n+\n static int in_commit_list(const struct commit_list *want, struct commit *c)\n {\n \tfor (; want; want = want->next)\n@@ -141,6 +160,9 @@ static int show_reference(const char *refname, const unsigned char *sha1,\n \t\t\t\treturn 0;\n \t\t}\n \n+\t\tif (points_at.nr && !match_points_at(sha1))\n+\t\t\treturn 0;\n+\n \t\tif (!filter->lines) {\n \t\t\tprintf(\"%s\\n\", refname);\n \t\t\treturn 0;\n@@ -389,6 +411,23 @@ static int strbuf_check_tag_ref(struct strbuf *sb, const char *name)\n \treturn check_refname_format(sb->buf, 0);\n }\n \n+int parse_opt_points_at(const struct option *opt __attribute__ ((unused)),\n+\t\t\tconst char *arg, int unset)\n+{\n+\tunsigned char sha1[20];\n+\n+\tif (unset) {\n+\t\tsha1_array_clear(&points_at);\n+\t\treturn 0;\n+\t}\n+\tif (!arg)\n+\t\treturn error(_(\"switch 'points-at' requires an object\"));\n+\tif (get_sha1(arg, sha1))\n+\t\treturn error(_(\"malformed object name '%s'\"), arg);\n+\tsha1_array_append(&points_at, sha1);\n+\treturn 0;\n+}\n+\n int cmd_tag(int argc, const char **argv, const char *prefix)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -432,6 +471,10 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\t\tPARSE_OPT_LASTARG_DEFAULT,\n \t\t\tparse_opt_with_commit, (intptr_t)\"HEAD\",\n \t\t},\n+\t\t{\n+\t\t\tOPTION_CALLBACK, 0, \"points-at\", NULL, \"object\",\n+\t\t\t\"print only tags of the object\", 0, parse_opt_points_at\n+\t\t},\n \t\tOPT_END()\n \t};\n \n@@ -478,8 +521,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t}\n \tif (lines != -1)\n \t\tdie(_(\"-n option is only allowed with -l.\"));\n-\tif (with_commit)\n-\t\tdie(_(\"--contains option is only allowed with -l.\"));\n+\tif (with_commit || points_at.nr)\n+\t\tdie(_(\"--contains and --points-at options \"\n+\t\t      \"are only allowed with -l.\"));\n \tif (delete)\n \t\treturn for_each_tag_name(argv, delete_tag);\n \tif (verify)\n-- \n1.7.8\n"},{"id":"184187","messageId":"20120208153100.GA8773@sigill.intra.peff.net","threadId":"29557","inReplyTo":"20120208014515.GE6264@tgrennan-laptop","subject":"Re: [PATCHv2] tag: add --points-at list option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-08T15:31:00Z","receivedAt":"2012-02-08T15:31:00Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 07, 2012 at 05:45:15PM -0800, Tom Grennan wrote:\n\n> >     Also, it's not symmetric. What if I say \"git tag\n> >     --points-at=my-lw-v1.7.9\"? Then I would get your signed and\n> >     annotated tags (even though they're _not_ saying anything about\n> >     ny-lw-v1.7.9), and I would get v1.7.9 (even though it's not saying\n> >     anything about it either; in fact, it's the opposite!).\n> \n> Huh?  As you noted, the lightweight tag is just an alternate reference,\n> so why wouldn't want to see the annotated and signed tags of that common\n> object?\n> \n>   $ ./git-tag -l --points-at tomg-lw-v1.7.9 \n>   tomg-annotate-v1.7.9\n>   tomg-signed-v1.7.9\n>   v1.7.9\n>   $ ./git-tag -l --points-at v1.7.9 \n>   tomg-annotate-v1.7.9\n>   tomg-lw-v1.7.9\n>   tomg-signed-v1.7.9\n\nSorry, I should have been more clear here (the word symmetric isn't\nright; it _is_ symmetric). My understanding of the point of your\noriginal feature was to mention things that talk about a tag (because\nyou wanted to know what signatures were made around it).\n\nWith tag objects this is easy, because they contain a pointer. But when\nit comes to lightweight tags, you cannot tell in which direction the\n\"talking about\" occurred[1]. That is, a lightweight tag of another tag\nis just creating a new ref, which looks the same as the old ref. So\nsomething like --points-at cannot say \"X talks about Y\", because it\nmight as well have been \"Y talks about X\".\n\nSo I think you are better off to mention both X and Y (or to mention\nneither).\n\n> >     Your documentation says \"Only list annotated or signed tags of the\n> >     given object\", which implies to me that --points-at is an arbitrary\n> >     object specifier, not a specific tagname.\n> \n> Yes, I changed that in the patch that I've prepared but will revert this\n> if you'd rather not list these lightweight tags.\n\nI'm OK with not mentioning lightweight tags. I just feel it should be\nall-or-nothing. It was specifically the \"given object\" that I took issue\nwith, since in your examples v1.7.9 was treated differently from its\nsha1.\n\n> My reaction when I tested this was, \"don't tell me what I already know.\"\n> But consistency with $(git rev-parse ...) seems more important.\n> And as you noted, a sha1_array would save code and to me, less code is\n> always better.\n\nThanks. Either I've convinced you, or I've made you so sick of the\ndiscussion that you're agreeing. The system works. :)\n\n-Peff\n\n[1] Actually, a tag object embeds the name of the ref under which it was\noriginally created (so the refs/tags/v1.7.9 tag has a \"tag v1.7.9\"\nheader in it). So in some cases, you _can_ determine the \"original\" ref\nof a lightweight tag versus other refs made about it later. I'm still\nnot sure --points-at is a good place to try to make that distinction,\nthough.\n"},{"id":"184188","messageId":"20120208154442.GB8773@sigill.intra.peff.net","threadId":"29557","inReplyTo":"1328682076-23380-2-git-send-email-tmgrennan@gmail.com","subject":"Re: [PATCHv3] tag: add --points-at list option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-08T15:44:42Z","receivedAt":"2012-02-08T15:44:42Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 07, 2012 at 10:21:16PM -0800, Tom Grennan wrote:\n\n> +static const unsigned char *match_points_at(const unsigned char *sha1)\n> +{\n> +\tint i;\n> +\tconst unsigned char *tagged_sha1 = (unsigned char*)\"\";\n> +\tstruct object *obj = parse_object(sha1);\n> +\n> +\tif (obj && obj->type == OBJ_TAG)\n> +\t\ttagged_sha1 = ((struct tag *)obj)->tagged->sha1;\n\nThis is not safe. A sha1 is not NUL-terminated, but is rather _always_\n20 bytes. So when the object is not a tag, you do the hashcmp against\nyour single-byte string literal above, and we end up comparing whatever\ngarbage is in the data segment after the string literal.\n\nWhat you want instead is the all-zeros sha1, like:\n\n  const unsigned char null_sha1[20] = { 0 };\n\nThough we provide a null_sha1 global already. So doing:\n\n  const unsigned char *tagged_sha1 = null_sha1;\n\nwould be sufficient.\n\nThat being said, I don't know why you want to do both lookups in the\nsame loop of the points_at. If it's a lightweight tag and the tag\nmatches, you can get away with not parsing the object at all (although\nto be fair, that is the minority case, so it is unlikely to matter).\n\nAlso, should we be producing an error if !obj? It would indicate a tag\nthat points to a bogus object.\n\n> +\tfor (i = 0; i < points_at.nr; i++)\n> +\t\tif (!hashcmp(points_at.sha1[i], sha1))\n> +\t\t\treturn sha1;\n> +\t\telse if (!hashcmp(points_at.sha1[i], tagged_sha1))\n> +\t\t\treturn tagged_sha1;\n> +\treturn NULL;\n\nWhy write your own linear search? sha1_array_lookup will do a binary\nsearch for you.\n\nOther than that, the patch looks OK to me.\n\n-Peff\n"},{"id":"184193","messageId":"20120208184332.GF6264@tgrennan-laptop","threadId":"29557","inReplyTo":"20120208154442.GB8773@sigill.intra.peff.net","subject":"Re: [PATCHv3] tag: add --points-at list option","fromName":"Tom Grennan","fromEmail":"tmgrennan@gmail.com","sentAt":"2012-02-08T18:43:32Z","receivedAt":"2012-02-08T18:43:32Z","isPatch":false,"sender":{"key":"tmgrennan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/422771?v=4"},"body":"On Wed, Feb 08, 2012 at 10:44:42AM -0500, Jeff King wrote:\n>On Tue, Feb 07, 2012 at 10:21:16PM -0800, Tom Grennan wrote:\n>\n>> +static const unsigned char *match_points_at(const unsigned char *sha1)\n>> +{\n>> +\tint i;\n>> +\tconst unsigned char *tagged_sha1 = (unsigned char*)\"\";\n>> +\tstruct object *obj = parse_object(sha1);\n>> +\n>> +\tif (obj && obj->type == OBJ_TAG)\n>> +\t\ttagged_sha1 = ((struct tag *)obj)->tagged->sha1;\n>\n>This is not safe. A sha1 is not NUL-terminated, but is rather _always_\n>20 bytes. So when the object is not a tag, you do the hashcmp against\n>your single-byte string literal above, and we end up comparing whatever\n>garbage is in the data segment after the string literal.\n\nYikes! That was dumb.\n\n>What you want instead is the all-zeros sha1, like:\n>\n>  const unsigned char null_sha1[20] = { 0 };\n>\n>Though we provide a null_sha1 global already. So doing:\n>\n>  const unsigned char *tagged_sha1 = null_sha1;\n>\n>would be sufficient.\n\nOr just initialize at test tagged_sha1 with NULL.\n\nstatic const unsigned char *match_points_at(const unsigned char *sha1)\n{\n\tint i;\n\tconst unsigned char *tagged_sha1 = NULL;\n\tstruct object *obj = parse_object(sha1);\n\n\tif (obj && obj->type == OBJ_TAG)\n\t\ttagged_sha1 = ((struct tag *)obj)->tagged->sha1;\n\tfor (i = 0; i < points_at.nr; i++)\n\t\tif (!hashcmp(points_at.sha1[i], sha1))\n\t\t\treturn sha1;\n\t\telse if (tagged_sha1 &&\n\t\t\t !hashcmp(points_at.sha1[i], tagged_sha1))\n\t\t\treturn tagged_sha1;\n\treturn NULL;\n}\n\n>That being said, I don't know why you want to do both lookups in the\n>same loop of the points_at. If it's a lightweight tag and the tag\n>matches, you can get away with not parsing the object at all (although\n>to be fair, that is the minority case, so it is unlikely to matter).\n\nYes, I think your saying that the lightweight search could go before the\ntag object search like this.\n\nstatic const unsigned char *match_points_at(const unsigned char *sha1)\n{\n\tconst unsigned char *tagged_sha1 = NULL;\n\tstruct object *obj = parse_object(sha1);\n\n\tif (sha1_array_lookup(&points_at, sha1) >= 0)\n\t\treturn sha1;\n\tif (obj && obj->type == OBJ_TAG)\n\t\ttagged_sha1 = ((struct tag *)obj)->tagged->sha1;\n\tif (tagged_sha1 && sha1_array_lookup(&points_at, tagged_sha1) >= 0)\n\t\treturn tagged_sha1;\n\treturn NULL;\n}\n\n>Also, should we be producing an error if !obj? It would indicate a tag\n>that points to a bogus object.\n\nI think the test of (obj) is redundant as this should be caught\nby get_sha1() in parse_opt_points_at()\n\nint parse_opt_points_at(const struct option *opt __attribute__ ((unused)),\n\t\t\tconst char *arg, int unset)\n{\n\tunsigned char sha1[20];\n\n\tif (unset) {\n\t\tsha1_array_clear(&points_at);\n\t\treturn 0;\n\t}\n\tif (!arg)\n\t\treturn error(_(\"switch 'points-at' requires an object\"));\n\tif (get_sha1(arg, sha1))\n\t\treturn error(_(\"malformed object name '%s'\"), arg);\n\tsha1_array_append(&points_at, sha1);\n\treturn 0;\n}\n\n>> +\tfor (i = 0; i < points_at.nr; i++)\n>> +\t\tif (!hashcmp(points_at.sha1[i], sha1))\n>> +\t\t\treturn sha1;\n>> +\t\telse if (!hashcmp(points_at.sha1[i], tagged_sha1))\n>> +\t\t\treturn tagged_sha1;\n>> +\treturn NULL;\n>\n>Why write your own linear search? sha1_array_lookup will do a binary\n>search for you.\n\nWell, it's only a linear search of the points_at command arguments.\nBut by that reasoning, might as well do two sha1_array_lookups like\nabove and save some code b/c \"less code is always better\"(TM).\n\n>Other than that, the patch looks OK to me.\n\nThanks, I'll send what I hope to be the final version later today.\n\n-- \nTomG\n"},{"id":"184195","messageId":"20120208185750.GA22220@sigill.intra.peff.net","threadId":"29557","inReplyTo":"20120208184332.GF6264@tgrennan-laptop","subject":"Re: [PATCHv3] tag: add --points-at list option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-08T18:57:50Z","receivedAt":"2012-02-08T18:57:50Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 08, 2012 at 10:43:32AM -0800, Tom Grennan wrote:\n\n> >Though we provide a null_sha1 global already. So doing:\n> >\n> >  const unsigned char *tagged_sha1 = null_sha1;\n> >\n> >would be sufficient.\n> \n> Or just initialize at test tagged_sha1 with NULL.\n\nOh yeah, that is even better.\n\n> >That being said, I don't know why you want to do both lookups in the\n> >same loop of the points_at. If it's a lightweight tag and the tag\n> >matches, you can get away with not parsing the object at all (although\n> >to be fair, that is the minority case, so it is unlikely to matter).\n> \n> Yes, I think your saying that the lightweight search could go before the\n> tag object search like this.\n\nExactly, though:\n\n> static const unsigned char *match_points_at(const unsigned char *sha1)\n> {\n> \tconst unsigned char *tagged_sha1 = NULL;\n> \tstruct object *obj = parse_object(sha1);\n> \n> \tif (sha1_array_lookup(&points_at, sha1) >= 0)\n> \t\treturn sha1;\n> \tif (obj && obj->type == OBJ_TAG)\n> \t\ttagged_sha1 = ((struct tag *)obj)->tagged->sha1;\n\nYou can delay the relatively expensive parse_object until you find the\nresults of the first lookup (though like I said earlier, it is unlikely to\nmatter, as it only helps in the positive-match case. Out of N tags, you\nwill likely end up parsing N-1 of them anyway).\n\n> >Also, should we be producing an error if !obj? It would indicate a tag\n> >that points to a bogus object.\n> \n> I think the test of (obj) is redundant as this should be caught\n> by get_sha1() in parse_opt_points_at()\n\nNo, it's not redundant. get_sha1 is purely about looking up the name and\nfinding a sha1. parse_object is about looking up the object represented\nby that sha1 in the object db. get_sha1 can sometimes involve parsing\nobjects (e.g., looking for \"foo^1\" will need to parse the commit object\nat \"foo\"),  but does not have to.\n\nBesides which, you are not calling parse_object on the sha1 from\n--points-at, but rather the sha1 for each tag ref given to us by\nfor_each_tag_ref.\n\n> >Why write your own linear search? sha1_array_lookup will do a binary\n> >search for you.\n> \n> Well, it's only a linear search of the points_at command arguments.\n> But by that reasoning, might as well do two sha1_array_lookups like\n> above and save some code b/c \"less code is always better\"(TM).\n\nRight. I expect the N to be small in this case, so I doubt it matters.\nBut two sha1_array_lookups is still asymptotically smaller, because the\nexpensive operation is hashcmp(). So two binary searches is O(2*lg n),\nwhereas a linear walk with 2 hashcmps per item is O(2*n).\n\n> >Other than that, the patch looks OK to me.\n> \n> Thanks, I'll send what I hope to be the final version later today.\n\nThanks for working on this and being so responsive to review.\n\n-Peff\n"},{"id":"184196","messageId":"20120208185823.GG6264@tgrennan-laptop","threadId":"29557","inReplyTo":"20120208184332.GF6264@tgrennan-laptop","subject":"Re: [PATCHv3] tag: add --points-at list option","fromName":"Tom Grennan","fromEmail":"tmgrennan@gmail.com","sentAt":"2012-02-08T18:58:23Z","receivedAt":"2012-02-08T18:58:23Z","isPatch":false,"sender":{"key":"tmgrennan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/422771?v=4"},"body":"On Wed, Feb 08, 2012 at 10:43:32AM -0800, Tom Grennan wrote:\n>On Wed, Feb 08, 2012 at 10:44:42AM -0500, Jeff King wrote:\n>>On Tue, Feb 07, 2012 at 10:21:16PM -0800, Tom Grennan wrote:\n>>\n>>Also, should we be producing an error if !obj? It would indicate a tag\n>>that points to a bogus object.\n>\n>I think the test of (obj) is redundant as this should be caught\n>by get_sha1() in parse_opt_points_at()\n\nI'm wrong. That tests the sha of the point-at argument, not the\nsha/objects of the refs/tags entry.  I'll add...\n\n\tif (!obj)\n\t\tdie(_(\"invalid tag, 'refs/tags/%s'\"), refname);\n\n-- \nTomG\n"},{"id":"184199","messageId":"1328731972-13137-1-git-send-email-tmgrennan@gmail.com","threadId":"29557","inReplyTo":"20120208185750.GA22220@sigill.intra.peff.net","subject":"[PATCHv4] tag: add --points-at list option","fromName":"Tom Grennan","fromEmail":"tmgrennan@gmail.com","sentAt":"2012-02-08T20:12:51Z","receivedAt":"2012-02-08T20:12:51Z","isPatch":false,"sender":{"key":"tmgrennan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/422771?v=4"},"body":"Please see the following patch that I hoped is the last version of the\n\"points-at\" feature.  Thank you for your patience.\n\nTom Grennan (1):\n  tag: add --points-at list option\n\n Documentation/git-tag.txt |    5 +++-\n builtin/tag.c             |   52 ++++++++++++++++++++++++++++++++++++++++++--\n 2 files changed, 53 insertions(+), 4 deletions(-)\n\n-- \n1.7.8\n"},{"id":"184200","messageId":"1328731972-13137-2-git-send-email-tmgrennan@gmail.com","threadId":"29557","inReplyTo":"1328731972-13137-1-git-send-email-tmgrennan@gmail.com","subject":"[PATCHv4] tag: add --points-at list option","fromName":"Tom Grennan","fromEmail":"tmgrennan@gmail.com","sentAt":"2012-02-08T20:12:52Z","receivedAt":"2012-02-08T20:12:52Z","isPatch":false,"sender":{"key":"tmgrennan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/422771?v=4"},"body":"This filters the list for tags of the given object.\nExample,\n\n   john$ git tag v1.0-john v1.0\n   john$ git tag -l --points-at v1.0\n   v1.0-john\n\nSigned-off-by: Tom Grennan <tmgrennan@gmail.com>\n---\n Documentation/git-tag.txt |    5 +++-\n builtin/tag.c             |   52 ++++++++++++++++++++++++++++++++++++++++++--\n 2 files changed, 53 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex 5ead91e..124ed36 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -12,7 +12,7 @@ SYNOPSIS\n 'git tag' [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]\n \t<tagname> [<commit> | <object>]\n 'git tag' -d <tagname>...\n-'git tag' [-n[<num>]] -l [--contains <commit>]\n+'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]\n \t[--column[=<options>] | --no-column] [<pattern>...]\n 'git tag' -v <tagname>...\n \n@@ -95,6 +95,9 @@ This option is only applicable when listing tags without annotation lines.\n --contains <commit>::\n \tOnly list tags which contain the specified commit.\n \n+--points-at <object>::\n+\tOnly list tags of the given object.\n+\n -m <msg>::\n --message=<msg>::\n \tUse the given tag message (instead of prompting).\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 5fbd62c..f3051c7 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -16,11 +16,13 @@\n #include \"revision.h\"\n #include \"gpg-interface.h\"\n #include \"column.h\"\n+#include \"sha1-array.h\"\n \n static const char * const git_tag_usage[] = {\n \t\"git tag [-a|-s|-u <key-id>] [-f] [-m <msg>|-F <file>] <tagname> [<head>]\",\n \t\"git tag -d <tagname>...\",\n-\t\"git tag -l [-n[<num>]] [<pattern>...]\",\n+\t\"git tag -l [-n[<num>]] [--contains <commit>] [--points-at <object>] \\\\\"\n+\t\t\"\\n\\t\\t[<pattern>...]\",\n \t\"git tag -v <tagname>...\",\n \tNULL\n };\n@@ -31,6 +33,7 @@ struct tag_filter {\n \tstruct commit_list *with_commit;\n };\n \n+static struct sha1_array points_at;\n static unsigned int colopts;\n \n static int match_pattern(const char **patterns, const char *ref)\n@@ -44,6 +47,24 @@ static int match_pattern(const char **patterns, const char *ref)\n \treturn 0;\n }\n \n+static const unsigned char *match_points_at(const char *refname,\n+\t\t\t\t\t    const unsigned char *sha1)\n+{\n+\tconst unsigned char *tagged_sha1 = NULL;\n+\tstruct object *obj;\n+\n+\tif (sha1_array_lookup(&points_at, sha1) >= 0)\n+\t\treturn sha1;\n+\tobj = parse_object(sha1);\n+\tif (!obj)\n+\t\tdie(_(\"malformed object at '%s'\"), refname);\n+\tif (obj->type == OBJ_TAG)\n+\t\ttagged_sha1 = ((struct tag *)obj)->tagged->sha1;\n+\tif (tagged_sha1 && sha1_array_lookup(&points_at, tagged_sha1) >= 0)\n+\t\treturn tagged_sha1;\n+\treturn NULL;\n+}\n+\n static int in_commit_list(const struct commit_list *want, struct commit *c)\n {\n \tfor (; want; want = want->next)\n@@ -141,6 +162,9 @@ static int show_reference(const char *refname, const unsigned char *sha1,\n \t\t\t\treturn 0;\n \t\t}\n \n+\t\tif (points_at.nr && !match_points_at(refname, sha1))\n+\t\t\treturn 0;\n+\n \t\tif (!filter->lines) {\n \t\t\tprintf(\"%s\\n\", refname);\n \t\t\treturn 0;\n@@ -389,6 +413,23 @@ static int strbuf_check_tag_ref(struct strbuf *sb, const char *name)\n \treturn check_refname_format(sb->buf, 0);\n }\n \n+int parse_opt_points_at(const struct option *opt __attribute__ ((unused)),\n+\t\t\tconst char *arg, int unset)\n+{\n+\tunsigned char sha1[20];\n+\n+\tif (unset) {\n+\t\tsha1_array_clear(&points_at);\n+\t\treturn 0;\n+\t}\n+\tif (!arg)\n+\t\treturn error(_(\"switch 'points-at' requires an object\"));\n+\tif (get_sha1(arg, sha1))\n+\t\treturn error(_(\"malformed object name '%s'\"), arg);\n+\tsha1_array_append(&points_at, sha1);\n+\treturn 0;\n+}\n+\n int cmd_tag(int argc, const char **argv, const char *prefix)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -432,6 +473,10 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\t\tPARSE_OPT_LASTARG_DEFAULT,\n \t\t\tparse_opt_with_commit, (intptr_t)\"HEAD\",\n \t\t},\n+\t\t{\n+\t\t\tOPTION_CALLBACK, 0, \"points-at\", NULL, \"object\",\n+\t\t\t\"print only tags of the object\", 0, parse_opt_points_at\n+\t\t},\n \t\tOPT_END()\n \t};\n \n@@ -478,8 +523,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t}\n \tif (lines != -1)\n \t\tdie(_(\"-n option is only allowed with -l.\"));\n-\tif (with_commit)\n-\t\tdie(_(\"--contains option is only allowed with -l.\"));\n+\tif (with_commit || points_at.nr)\n+\t\tdie(_(\"--contains and --points-at options \"\n+\t\t      \"are only allowed with -l.\"));\n \tif (delete)\n \t\treturn for_each_tag_name(argv, delete_tag);\n \tif (verify)\n-- \n1.7.8\n"},{"id":"184201","messageId":"20120208205857.GA22479@sigill.intra.peff.net","threadId":"29557","inReplyTo":"1328731972-13137-2-git-send-email-tmgrennan@gmail.com","subject":"Re: [PATCHv4] tag: add --points-at list option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-08T20:58:57Z","receivedAt":"2012-02-08T20:58:57Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 08, 2012 at 12:12:52PM -0800, Tom Grennan wrote:\n\n> This filters the list for tags of the given object.\n> Example,\n> \n>    john$ git tag v1.0-john v1.0\n>    john$ git tag -l --points-at v1.0\n>    v1.0-john\n\nAnd probably \"v1.0\", as well, in this iteration. :)\n\nThe patch content itself looks good to me, except:\n\n> --- a/Documentation/git-tag.txt\n> +++ b/Documentation/git-tag.txt\n> @@ -12,7 +12,7 @@ SYNOPSIS\n>  'git tag' [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]\n>  \t<tagname> [<commit> | <object>]\n>  'git tag' -d <tagname>...\n> -'git tag' [-n[<num>]] -l [--contains <commit>]\n> +'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]\n>  \t[--column[=<options>] | --no-column] [<pattern>...]\n\nWhat's this \"column\" stuff doing here? The nd/columns topic is still in\n\"next\", isn't it? Did you base this on \"next\" or \"pu\"?\n\nUsually topics should be based on master, so they can graduate\nindependently of each other. In this case, it might make sense to build\non top of jk/maint-tag-show-fixes (d0548a3), but I don't think that is\neven necessary here (my fixes ended up not being too closely related, I\nthink).\n\nOther than that, I think the patch is fine. There are no tests, so\nperhaps these should be squashed in:\n\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex e93ac73..f61e398 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1269,4 +1269,43 @@ test_expect_success 'mixing incompatibles modes and options is forbidden' '\n \ttest_must_fail git tag -v -s\n '\n \n+# check points-at\n+\n+test_expect_success '--points-at cannot be used in non-list mode' '\n+\ttest_must_fail git tag --points-at=v4.0 foo\n+'\n+\n+test_expect_success '--points-at finds lightweight tags' '\n+\techo v4.0 >expect &&\n+\tgit tag --points-at v4.0 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--points-at finds annotated tags of commits' '\n+\tgit tag -m \"v4.0, annotated\" annotated-v4.0 v4.0 &&\n+\techo annotated-v4.0 >expect &&\n+\tgit tag -l --points-at v4.0 \"annotated*\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--points-at finds annotated tags of tags' '\n+\tgit tag -m \"describing the v4.0 tag object\" \\\n+\t\tannotated-again-v4.0 annotated-v4.0 &&\n+\tcat >expect <<-\\EOF &&\n+\tannotated-again-v4.0\n+\tannotated-v4.0\n+\tEOF\n+\tgit tag --points-at=annotated-v4.0 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'multiple --points-at are OR-ed together' '\n+\tcat >expect <<-\\EOF &&\n+\tv2.0\n+\tv3.0\n+\tEOF\n+\tgit tag --points-at=v2.0 --points-at=v3.0 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.7.9.rc2.14.g3da2b\n"},{"id":"184202","messageId":"20120208210156.GA9588@sigill.intra.peff.net","threadId":"29557","inReplyTo":"7vty337rug.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/3] tag: die when listing missing or corrupt objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-08T21:01:56Z","receivedAt":"2012-02-08T21:01:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 06, 2012 at 10:13:27AM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > OK, that's easy enough to do. Should we show lightweight tags to commits\n> > for backwards compatibility (and just drop the parse_signature junk in\n> > that case)? The showing of blobs or trees is the really bad thing, I\n> > think.\n> \n> For now, dropping 3/3 and queuing this instead...\n> \n> ---\n> Subject: tag: do not show non-tag contents with \"-n\"\n\nSince jk/maint-tag-show-fixes is still in pu, perhaps we can squash in\nthis test from my 3/3:\n\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex e93ac73..0db0f6a 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -586,6 +586,19 @@ test_expect_success \\\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'annotations for blobs are empty' '\n+\tblob=$(git hash-object -w --stdin <<-\\EOF\n+\tBlob paragraph 1.\n+\n+\tBlob paragraph 2.\n+\tEOF\n+\t) &&\n+\tgit tag tag-blob $blob &&\n+\techo \"tag-blob        \" >expect &&\n+\tgit tag -n1 -l tag-blob >actual &&\n+\ttest_cmp expect actual\n+'\n+\n # trying to verify annotated non-signed tags:\n \n test_expect_success GPG \\\n\nIf we want to be more thorough, I can write up a more complete test\nbattery making sure tags and commits are both shown, but blobs and trees\nare not.\n\n-Peff\n"},{"id":"184207","messageId":"20120208221531.GI6264@tgrennan-laptop","threadId":"29557","inReplyTo":"20120208205857.GA22479@sigill.intra.peff.net","subject":"Re: [PATCHv4] tag: add --points-at list option","fromName":"Tom Grennan","fromEmail":"tmgrennan@gmail.com","sentAt":"2012-02-08T22:15:31Z","receivedAt":"2012-02-08T22:15:31Z","isPatch":false,"sender":{"key":"tmgrennan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/422771?v=4"},"body":"On Wed, Feb 08, 2012 at 03:58:57PM -0500, Jeff King wrote:\n>On Wed, Feb 08, 2012 at 12:12:52PM -0800, Tom Grennan wrote:\n>\n>> This filters the list for tags of the given object.\n>> Example,\n>> \n>>    john$ git tag v1.0-john v1.0\n>>    john$ git tag -l --points-at v1.0\n>>    v1.0-john\n>\n>And probably \"v1.0\", as well, in this iteration. :)\n\nYep.\n\n>The patch content itself looks good to me, except:\n>\n>> --- a/Documentation/git-tag.txt\n>> +++ b/Documentation/git-tag.txt\n>> @@ -12,7 +12,7 @@ SYNOPSIS\n>>  'git tag' [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]\n>>  \t<tagname> [<commit> | <object>]\n>>  'git tag' -d <tagname>...\n>> -'git tag' [-n[<num>]] -l [--contains <commit>]\n>> +'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]\n>>  \t[--column[=<options>] | --no-column] [<pattern>...]\n>\n>What's this \"column\" stuff doing here? The nd/columns topic is still in\n>\"next\", isn't it? Did you base this on \"next\" or \"pu\"?\n>\n>Usually topics should be based on master, so they can graduate\n>independently of each other. In this case, it might make sense to build\n>on top of jk/maint-tag-show-fixes (d0548a3), but I don't think that is\n>even necessary here (my fixes ended up not being too closely related, I\n>think).\n\nYes, it's no longer related to jk/maint-tag-show-fixes.\nI've prepared a rebase patch to master and will add these tests.\n\nThanks,\nTomG\n\n>Other than that, I think the patch is fine. There are no tests, so\n>perhaps these should be squashed in:\n>\n>diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\n>index e93ac73..f61e398 100755\n>--- a/t/t7004-tag.sh\n>+++ b/t/t7004-tag.sh\n>@@ -1269,4 +1269,43 @@ test_expect_success 'mixing incompatibles modes and options is forbidden' '\n> \ttest_must_fail git tag -v -s\n> '\n> \n>+# check points-at\n>+\n>+test_expect_success '--points-at cannot be used in non-list mode' '\n>+\ttest_must_fail git tag --points-at=v4.0 foo\n>+'\n>+\n>+test_expect_success '--points-at finds lightweight tags' '\n>+\techo v4.0 >expect &&\n>+\tgit tag --points-at v4.0 >actual &&\n>+\ttest_cmp expect actual\n>+'\n>+\n>+test_expect_success '--points-at finds annotated tags of commits' '\n>+\tgit tag -m \"v4.0, annotated\" annotated-v4.0 v4.0 &&\n>+\techo annotated-v4.0 >expect &&\n>+\tgit tag -l --points-at v4.0 \"annotated*\" >actual &&\n>+\ttest_cmp expect actual\n>+'\n>+\n>+test_expect_success '--points-at finds annotated tags of tags' '\n>+\tgit tag -m \"describing the v4.0 tag object\" \\\n>+\t\tannotated-again-v4.0 annotated-v4.0 &&\n>+\tcat >expect <<-\\EOF &&\n>+\tannotated-again-v4.0\n>+\tannotated-v4.0\n>+\tEOF\n>+\tgit tag --points-at=annotated-v4.0 >actual &&\n>+\ttest_cmp expect actual\n>+'\n>+\n>+test_expect_success 'multiple --points-at are OR-ed together' '\n>+\tcat >expect <<-\\EOF &&\n>+\tv2.0\n>+\tv3.0\n>+\tEOF\n>+\tgit tag --points-at=v2.0 --points-at=v3.0 >actual &&\n>+\ttest_cmp expect actual\n>+'\n>+\n> test_done\n>-- \n>1.7.9.rc2.14.g3da2b\n>\n"},{"id":"184210","messageId":"1328742223-24419-1-git-send-email-tmgrennan@gmail.com","threadId":"29557","inReplyTo":"20120208205857.GA22479@sigill.intra.peff.net","subject":"[PATCH-master] tag: add --points-at list option","fromName":"Tom Grennan","fromEmail":"tmgrennan@gmail.com","sentAt":"2012-02-08T23:03:42Z","receivedAt":"2012-02-08T23:03:42Z","isPatch":true,"sender":{"key":"tmgrennan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/422771?v=4"},"body":"The following applies the \"points-at\" feature to master and includes unit tests\nfrom Jeff King <peff@peff.net>\n\nTom Grennan (1):\n  tag: add --points-at list option\n\n Documentation/git-tag.txt |    6 ++++-\n builtin/tag.c             |   50 ++++++++++++++++++++++++++++++++++++++++++++-\n t/t7004-tag.sh            |   39 +++++++++++++++++++++++++++++++++++\n 3 files changed, 93 insertions(+), 2 deletions(-)\n\n-- \n1.7.8\n"},{"id":"184211","messageId":"1328742223-24419-2-git-send-email-tmgrennan@gmail.com","threadId":"29557","inReplyTo":"1328742223-24419-1-git-send-email-tmgrennan@gmail.com","subject":"[PATCH-master] tag: add --points-at list option","fromName":"Tom Grennan","fromEmail":"tmgrennan@gmail.com","sentAt":"2012-02-08T23:03:43Z","receivedAt":"2012-02-08T23:03:43Z","isPatch":true,"sender":{"key":"tmgrennan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/422771?v=4"},"body":"This filters the list for tags of the given object.\nExample,\n\n   john$ git tag v1.0-john v1.0\n   john$ git tag -l --points-at v1.0\n   v1.0-john\n   v1.0\n\nSigned-off-by: Tom Grennan <tmgrennan@gmail.com>\n---\n Documentation/git-tag.txt |    6 ++++-\n builtin/tag.c             |   50 ++++++++++++++++++++++++++++++++++++++++++++-\n t/t7004-tag.sh            |   39 +++++++++++++++++++++++++++++++++++\n 3 files changed, 93 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex 53ff5f6..8d32b9a 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -12,7 +12,8 @@ SYNOPSIS\n 'git tag' [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]\n \t<tagname> [<commit> | <object>]\n 'git tag' -d <tagname>...\n-'git tag' [-n[<num>]] -l [--contains <commit>] [<pattern>...]\n+'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]\n+\t[<pattern>...]\n 'git tag' -v <tagname>...\n \n DESCRIPTION\n@@ -86,6 +87,9 @@ OPTIONS\n --contains <commit>::\n \tOnly list tags which contain the specified commit.\n \n+--points-at <object>::\n+\tOnly list tags of the given object.\n+\n -m <msg>::\n --message=<msg>::\n \tUse the given tag message (instead of prompting).\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 31f02e8..27c3557 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -15,11 +15,13 @@\n #include \"diff.h\"\n #include \"revision.h\"\n #include \"gpg-interface.h\"\n+#include \"sha1-array.h\"\n \n static const char * const git_tag_usage[] = {\n \t\"git tag [-a|-s|-u <key-id>] [-f] [-m <msg>|-F <file>] <tagname> [<head>]\",\n \t\"git tag -d <tagname>...\",\n-\t\"git tag -l [-n[<num>]] [<pattern>...]\",\n+\t\"git tag -l [-n[<num>]] [--contains <commit>] [--points-at <object>] \"\n+\t\t\"\\n\\t\\t[<pattern>...]\",\n \t\"git tag -v <tagname>...\",\n \tNULL\n };\n@@ -30,6 +32,8 @@ struct tag_filter {\n \tstruct commit_list *with_commit;\n };\n \n+static struct sha1_array points_at;\n+\n static int match_pattern(const char **patterns, const char *ref)\n {\n \t/* no pattern means match everything */\n@@ -41,6 +45,24 @@ static int match_pattern(const char **patterns, const char *ref)\n \treturn 0;\n }\n \n+static const unsigned char *match_points_at(const char *refname,\n+\t\t\t\t\t    const unsigned char *sha1)\n+{\n+\tconst unsigned char *tagged_sha1 = NULL;\n+\tstruct object *obj;\n+\n+\tif (sha1_array_lookup(&points_at, sha1) >= 0)\n+\t\treturn sha1;\n+\tobj = parse_object(sha1);\n+\tif (!obj)\n+\t\tdie(_(\"malformed object at '%s'\"), refname);\n+\tif (obj->type == OBJ_TAG)\n+\t\ttagged_sha1 = ((struct tag *)obj)->tagged->sha1;\n+\tif (tagged_sha1 && sha1_array_lookup(&points_at, tagged_sha1) >= 0)\n+\t\treturn tagged_sha1;\n+\treturn NULL;\n+}\n+\n static int in_commit_list(const struct commit_list *want, struct commit *c)\n {\n \tfor (; want; want = want->next)\n@@ -105,6 +127,9 @@ static int show_reference(const char *refname, const unsigned char *sha1,\n \t\t\t\treturn 0;\n \t\t}\n \n+\t\tif (points_at.nr && !match_points_at(refname, sha1))\n+\t\t\treturn 0;\n+\n \t\tif (!filter->lines) {\n \t\t\tprintf(\"%s\\n\", refname);\n \t\t\treturn 0;\n@@ -375,6 +400,23 @@ static int strbuf_check_tag_ref(struct strbuf *sb, const char *name)\n \treturn check_refname_format(sb->buf, 0);\n }\n \n+int parse_opt_points_at(const struct option *opt __attribute__ ((unused)),\n+\t\t\tconst char *arg, int unset)\n+{\n+\tunsigned char sha1[20];\n+\n+\tif (unset) {\n+\t\tsha1_array_clear(&points_at);\n+\t\treturn 0;\n+\t}\n+\tif (!arg)\n+\t\treturn error(_(\"switch 'points-at' requires an object\"));\n+\tif (get_sha1(arg, sha1))\n+\t\treturn error(_(\"malformed object name '%s'\"), arg);\n+\tsha1_array_append(&points_at, sha1);\n+\treturn 0;\n+}\n+\n int cmd_tag(int argc, const char **argv, const char *prefix)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -417,6 +459,10 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\t\tPARSE_OPT_LASTARG_DEFAULT,\n \t\t\tparse_opt_with_commit, (intptr_t)\"HEAD\",\n \t\t},\n+\t\t{\n+\t\t\tOPTION_CALLBACK, 0, \"points-at\", NULL, \"object\",\n+\t\t\t\"print only tags of the object\", 0, parse_opt_points_at\n+\t\t},\n \t\tOPT_END()\n \t};\n \n@@ -448,6 +494,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"-n option is only allowed with -l.\"));\n \tif (with_commit)\n \t\tdie(_(\"--contains option is only allowed with -l.\"));\n+\tif (points_at.nr)\n+\t\tdie(_(\"--points-at option is only allowed with -l.\"));\n \tif (delete)\n \t\treturn for_each_tag_name(argv, delete_tag);\n \tif (verify)\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex e93ac73..f61e398 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1269,4 +1269,43 @@ test_expect_success 'mixing incompatibles modes and options is forbidden' '\n \ttest_must_fail git tag -v -s\n '\n \n+# check points-at\n+\n+test_expect_success '--points-at cannot be used in non-list mode' '\n+\ttest_must_fail git tag --points-at=v4.0 foo\n+'\n+\n+test_expect_success '--points-at finds lightweight tags' '\n+\techo v4.0 >expect &&\n+\tgit tag --points-at v4.0 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--points-at finds annotated tags of commits' '\n+\tgit tag -m \"v4.0, annotated\" annotated-v4.0 v4.0 &&\n+\techo annotated-v4.0 >expect &&\n+\tgit tag -l --points-at v4.0 \"annotated*\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--points-at finds annotated tags of tags' '\n+\tgit tag -m \"describing the v4.0 tag object\" \\\n+\t\tannotated-again-v4.0 annotated-v4.0 &&\n+\tcat >expect <<-\\EOF &&\n+\tannotated-again-v4.0\n+\tannotated-v4.0\n+\tEOF\n+\tgit tag --points-at=annotated-v4.0 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'multiple --points-at are OR-ed together' '\n+\tcat >expect <<-\\EOF &&\n+\tv2.0\n+\tv3.0\n+\tEOF\n+\tgit tag --points-at=v2.0 --points-at=v3.0 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.7.8\n"},{"id":"184212","messageId":"20120209014430.GA21661@sigill.intra.peff.net","threadId":"29557","inReplyTo":"1328742223-24419-2-git-send-email-tmgrennan@gmail.com","subject":"Re: [PATCH-master] tag: add --points-at list option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-09T01:44:30Z","receivedAt":"2012-02-09T01:44:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 08, 2012 at 03:03:43PM -0800, Tom Grennan wrote:\n\n> This filters the list for tags of the given object.\n> Example,\n> \n>    john$ git tag v1.0-john v1.0\n>    john$ git tag -l --points-at v1.0\n>    v1.0-john\n>    v1.0\n> \n> Signed-off-by: Tom Grennan <tmgrennan@gmail.com>\n\nThis version looks fine to me. Thanks.\n\n-Peff\n"},{"id":"184217","messageId":"7vfweky6hl.fsf@alter.siamese.dyndns.org","threadId":"29557","inReplyTo":"20120209014430.GA21661@sigill.intra.peff.net","subject":"Re: [PATCH-master] tag: add --points-at list option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-09T04:29:26Z","receivedAt":"2012-02-09T04:29:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> Signed-off-by: Tom Grennan <tmgrennan@gmail.com>\n>\n> This version looks fine to me. Thanks.\n\nThanks, both.  Will queue.\n"},{"id":"184218","messageId":"7vbop8y6a8.fsf@alter.siamese.dyndns.org","threadId":"29557","inReplyTo":"20120208210156.GA9588@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] tag: die when listing missing or corrupt objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-09T04:33:51Z","receivedAt":"2012-02-09T04:33:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Since jk/maint-tag-show-fixes is still in pu, perhaps we can squash in\n> this test from my 3/3:\n\nThanks.\n"}]}