{"thread":{"id":"33003","subject":"[PATCH 2/2] describe: Exclude --all --match=PATTERN","startedAt":"2013-02-25T05:31:52Z","lastAt":"2013-03-03T22:07:56Z","messageCount":6,"participants":["Greg Price","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"210222","messageId":"20130225053152.GI5688@biohazard-cafe.mit.edu","threadId":"33003","inReplyTo":null,"subject":"[PATCH 2/2] describe: Exclude --all --match=PATTERN","fromName":"Greg Price","fromEmail":"price@mit.edu","sentAt":"2013-02-25T05:31:52Z","receivedAt":"2013-02-25T05:31:52Z","isPatch":true,"sender":{"key":"price@mit.edu","avatar":"https://avatars.githubusercontent.com/u/28173?v=4"},"body":"Currently when --all is passed, the effect of --match is only\nto demote non-matching tags to be treated like non-tags.  This\nis puzzling behavior and not consistent with the documentation,\nespecially with the suggested usage of avoiding information leaks.\nThe combination of --all and --match is an oxymoron anyway, so\njust forbid it.\n\nSigned-off-by: Greg Price <price@mit.edu>\n---\nThis should be applied after the preceding patch; I mistakenly omitted\nthe '1/2' in its subject line.\n\n Documentation/git-describe.txt | 3 ++-\n builtin/describe.c             | 3 +++\n 2 files changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt\nindex 711040d..fd5d8f2 100644\n--- a/Documentation/git-describe.txt\n+++ b/Documentation/git-describe.txt\n@@ -83,7 +83,8 @@ OPTIONS\n --match <pattern>::\n \tOnly consider tags matching the given `glob(7)` pattern,\n \texcluding the \"refs/tags/\" prefix.  This can be used to avoid\n-\tleaking private tags from the repository.\n+\tleaking private tags from the repository.  This option is\n+\tincompatible with `--all`.\n \n --always::\n \tShow uniquely abbreviated commit object as fallback.\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex 04c185b..90a72af 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -435,6 +435,9 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \tif (longformat && abbrev == 0)\n \t\tdie(_(\"--long is incompatible with --abbrev=0\"));\n \n+\tif (pattern && all)\n+\t\tdie(_(\"--match is incompatible with --all\"));\n+\n \tif (contains) {\n \t\tconst char **args = xmalloc((7 + argc) * sizeof(char *));\n \t\tint i = 0;\n-- \n1.7.11.3\n"},{"id":"210410","messageId":"7v1uc1jyq0.fsf@alter.siamese.dyndns.org","threadId":"33003","inReplyTo":"20130225053152.GI5688@biohazard-cafe.mit.edu","subject":"Re: [PATCH 2/2] describe: Exclude --all --match=PATTERN","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-27T20:20:07Z","receivedAt":"2013-02-27T20:20:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Greg Price <price@MIT.EDU> writes:\n\n> Currently when --all is passed, the effect of --match is only\n> to demote non-matching tags to be treated like non-tags.  This\n> is puzzling behavior and not consistent with the documentation,\n> especially with the suggested usage of avoiding information leaks.\n> The combination of --all and --match is an oxymoron anyway, so\n> just forbid it.\n\nI am not sure if this is (1) \"behaviour is sometimes useful in\nnarrow cases but is not explained well\", (2) \"behaviour does not\nmake sense in any situation\", or (3) \"the combination can make sense\nif corrected, but the current behaviour is buggy\".  If it is (2) or\n(3), I think it makes sense to forbid the combination. Also, if it\nis (3), we should later come up with an improved behaviour and then\nre-enable the combination.\n\nWithout \"--all\" the command considers only the annotated tags to\nbase the descripion on, and with \"--all\", a ref that is not\nannotated tags can be used as a base, but with a lower priority (if\nan annotated tag can describe a given commit, that tag is used).\n\nSo naïvely I would expect \"--all\" and \"--match\" to base the\ndescription on refs that match the pattern without limiting the\nchoice of base to annotated tags, and refs that do not match the\ngiven pattern should not appear even as the last resort.  It appears\nto me that the current situation is (3).\n\nWill queue and cook in 'next'; thanks.\n\n>\n> Signed-off-by: Greg Price <price@mit.edu>\n> ---\n> This should be applied after the preceding patch; I mistakenly omitted\n> the '1/2' in its subject line.\n>\n>  Documentation/git-describe.txt | 3 ++-\n>  builtin/describe.c             | 3 +++\n>  2 files changed, 5 insertions(+), 1 deletion(-)\n>\n> diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt\n> index 711040d..fd5d8f2 100644\n> --- a/Documentation/git-describe.txt\n> +++ b/Documentation/git-describe.txt\n> @@ -83,7 +83,8 @@ OPTIONS\n>  --match <pattern>::\n>  \tOnly consider tags matching the given `glob(7)` pattern,\n>  \texcluding the \"refs/tags/\" prefix.  This can be used to avoid\n> -\tleaking private tags from the repository.\n> +\tleaking private tags from the repository.  This option is\n> +\tincompatible with `--all`.\n>  \n>  --always::\n>  \tShow uniquely abbreviated commit object as fallback.\n> diff --git a/builtin/describe.c b/builtin/describe.c\n> index 04c185b..90a72af 100644\n> --- a/builtin/describe.c\n> +++ b/builtin/describe.c\n> @@ -435,6 +435,9 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n>  \tif (longformat && abbrev == 0)\n>  \t\tdie(_(\"--long is incompatible with --abbrev=0\"));\n>  \n> +\tif (pattern && all)\n> +\t\tdie(_(\"--match is incompatible with --all\"));\n> +\n>  \tif (contains) {\n>  \t\tconst char **args = xmalloc((7 + argc) * sizeof(char *));\n>  \t\tint i = 0;\n"},{"id":"210442","messageId":"7vtxowglaa.fsf@alter.siamese.dyndns.org","threadId":"33003","inReplyTo":"7v1uc1jyq0.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] describe: Exclude --all --match=PATTERN","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-28T21:50:53Z","receivedAt":"2013-02-28T21:50:53Z","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> I am not sure if this is (1) \"behaviour is sometimes useful in\n> narrow cases but is not explained well\", (2) \"behaviour does not\n> make sense in any situation\", or (3) \"the combination can make sense\n> if corrected, but the current behaviour is buggy\".  If it is (2) or\n> (3), I think it makes sense to forbid the combination. Also, if it\n> is (3), we should later come up with an improved behaviour and then\n> re-enable the combination.\n>\n> Without \"--all\" the command considers only the annotated tags to\n> base the descripion on, and with \"--all\", a ref that is not\n> annotated tags can be used as a base, but with a lower priority (if\n> an annotated tag can describe a given commit, that tag is used).\n>\n> So naïvely I would expect \"--all\" and \"--match\" to base the\n> description on refs that match the pattern without limiting the\n> choice of base to annotated tags, and refs that do not match the\n> given pattern should not appear even as the last resort.  It appears\n> to me that the current situation is (3).\n>\n> Will queue and cook in 'next'; thanks.\n\nA fix to the broken semantics may look like this.  There are a few\npoints to note:\n\n * The local variable names \"is_tag\" and \"might_be_tag\" were\n   inconsistent with the rest of the program, where the global\n   variable \"tags\" is used to mean \"the user gave --tags to allow\n   lightweight ones to be used\".  By that definition of the tag, a\n   ref under refs/tags/ *is* a tag, and a ref that peels to a\n   different object is an annotated tag.  These two variable names\n   have been fixed.\n\n * The function returns early for a ref outside refs/tags/ when\n   \"--all\" is not given with or without this patch.  At the end of\n   the function, it also returned when (!all && !prio), but prio\n   becomes zero only when the ref is outside refs/tags/ (or the tag\n   does not match the pattern) in the original code.  With this\n   patch, we reject refs outside refs/tags/ early when \"--all\" is\n   not given, so the last-minute check before add_to_known_names()\n   becomes unnecessary (hence removed).\n\n * If somebody is crazy enough to have an annotated tag under\n   refs/heads/, the code would treat it as an annotated tag and\n   assign prio==2 to it, with or without this patch.  We may want to\n   tighten this further by checking with is_tag, but this patch does\n   not do anything about it; I wanted it to focus on only one bug,\n   i.e. interaction between \"--all\" and \"--match=<pattern>\".\n\n * When \"--tags\" is not given, we still give an unannotated tag to\n   add_to_known_names(), only to issue a hint when the given commit\n   is not describable with annotated tags but it could be described\n   if \"--tags\" were given.  I think this is optimizing for the wrong\n   case, and wasting resources.\n\n\n builtin/describe.c | 41 ++++++++++++++++++++---------------------\n 1 file changed, 20 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex 04c185b..b2b740d 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -137,40 +137,39 @@ static void add_to_known_names(const char *path,\n \n static int get_name(const char *path, const unsigned char *sha1, int flag, void *cb_data)\n {\n-\tint might_be_tag = !prefixcmp(path, \"refs/tags/\");\n+\tint is_tag = !prefixcmp(path, \"refs/tags/\");\n \tunsigned char peeled[20];\n-\tint is_tag, prio;\n+\tint is_annotated, prio;\n \n-\tif (!all && !might_be_tag)\n+\t/* Reject anything outside refs/tags/ unless --all */\n+\tif (!all && !is_tag)\n \t\treturn 0;\n \n+\t/* Accept only tags that match the pattern, if given */\n+\tif (pattern && (!is_tag || fnmatch(pattern, path + 10, 0)))\n+\t\treturn 0;\n+\n+\t/* Is it annotated? */\n \tif (!peel_ref(path, peeled)) {\n-\t\tis_tag = !!hashcmp(sha1, peeled);\n+\t\tis_annotated = !!hashcmp(sha1, peeled);\n \t} else {\n \t\thashcpy(peeled, sha1);\n-\t\tis_tag = 0;\n+\t\tis_annotated = 0;\n \t}\n \n-\t/* If --all, then any refs are used.\n-\t * If --tags, then any tags are used.\n-\t * Otherwise only annotated tags are used.\n+\t/*\n+\t * By default, we only use annotated tags, but with --tags\n+\t * we fall back to lightweight ones (even without --tags,\n+\t * we still remember lightweight ones, only to give hints\n+\t * in an error message).  --all allows any refs to be used.\n \t */\n-\tif (might_be_tag) {\n-\t\tif (is_tag)\n-\t\t\tprio = 2;\n-\t\telse\n-\t\t\tprio = 1;\n-\n-\t\tif (pattern && fnmatch(pattern, path + 10, 0))\n-\t\t\tprio = 0;\n-\t}\n+\tif (is_annotated)\n+\t\tprio = 2;\n+\telse if (is_tag)\n+\t\tprio = 1;\n \telse\n \t\tprio = 0;\n \n-\tif (!all) {\n-\t\tif (!prio)\n-\t\t\treturn 0;\n-\t}\n \tadd_to_known_names(all ? path + 5 : path + 10, peeled, prio, sha1);\n \treturn 0;\n }\n"},{"id":"210538","messageId":"20130303205232.GL22203@biohazard-cafe.mit.edu","threadId":"33003","inReplyTo":"7v1uc1jyq0.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] describe: Exclude --all --match=PATTERN","fromName":"Greg Price","fromEmail":"price@mit.edu","sentAt":"2013-03-03T20:52:32Z","receivedAt":"2013-03-03T20:52:32Z","isPatch":true,"sender":{"key":"price@mit.edu","avatar":"https://avatars.githubusercontent.com/u/28173?v=4"},"body":"On Wed, Feb 27, 2013 at 12:20:07PM -0800, Junio C Hamano wrote:\n> Without \"--all\" the command considers only the annotated tags to\n> base the descripion on, and with \"--all\", a ref that is not\n> annotated tags can be used as a base, but with a lower priority (if\n> an annotated tag can describe a given commit, that tag is used).\n> \n> So naïvely I would expect \"--all\" and \"--match\" to base the\n> description on refs that match the pattern without limiting the\n> choice of base to annotated tags, and refs that do not match the\n> given pattern should not appear even as the last resort.  It appears\n> to me that the current situation is (3).\n\nHmm.  It seems to me that \"--all\" says two things:\n\n (a) allow unannotated (rather than only annotated)\n\n (b) allow refs of any name (rather than only tags)\n\nWith \"--match\", particularly because the pattern always refers only to\ntags, (b) is obliterated, and your proposed semantics are (a) plus a\nsort of inverse of (b):\n\n (c) allow only refs matching the pattern\n\nwhich is what \"--match\" means alone.  But if what we are going for is\n(a) and (c), then we don't need \"--all\" for (a) -- we can get\nprecisely that with \"--tags\".  So these semantics make \"--all --match=PAT\"\nequivalent to \"--tags --match=PAT\".\n\nGiven that, I think the user is better off if we reject \"--all\n--match\" with an error message -- and perhaps the error message should\nadvise them to use \"--tags\" instead.  Otherwise we have \"--all\"\ntelling us (b) as well as (a), and \"--match\" countermanding (b) and\ngoing precisely the other direction to (c).  If the user has written\nthat by hand, then they may be confused, and if the command line was\ngenerated, perhaps called from a script, then I fear a bug in the\nscript is likely, what with the conflicting expectations expressed by\n\"--all\" and \"--match\".\n\nPatch below to suggest \"--tags\" in the error message.\n\nGreg\n\n\n>From 66f985b2510c870e62e313732de4b6709894b074 Mon Sep 17 00:00:00 2001\nFrom: Greg Price <price@mit.edu>\nDate: Sun, 3 Mar 2013 12:19:57 -0800\nSubject: [PATCH] describe: Better error message on --all --match\n\nThe reason they conflict is that --all means\n (a) allow unannotated, and\n (b) allow refs of any name (rather than only tags),\nwhile --match contradicts (b) and goes further to\n (c) allow only tags matching pattern.\n\nIf what the user wants is (a) and (c), they can use --tags to get (a)\nwithout (b).\n---\n builtin/describe.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex 2ef3f10..6581c40 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -436,7 +436,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"--long is incompatible with --abbrev=0\"));\n \n \tif (pattern && all)\n-\t\tdie(_(\"--match is incompatible with --all\"));\n+\t\tdie(_(\"--all conflicts with --match; do you mean --tags?\"));\n \n \tif (contains) {\n \t\tconst char **args = xmalloc((7 + argc) * sizeof(char *));\n-- \n1.7.11.3\n\n"},{"id":"210540","messageId":"7vfw0cb2xi.fsf@alter.siamese.dyndns.org","threadId":"33003","inReplyTo":"20130303205232.GL22203@biohazard-cafe.mit.edu","subject":"Re: [PATCH 2/2] describe: Exclude --all --match=PATTERN","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-03T21:15:21Z","receivedAt":"2013-03-03T21:15:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Greg Price <price@MIT.EDU> writes:\n\n> On Wed, Feb 27, 2013 at 12:20:07PM -0800, Junio C Hamano wrote:\n>> Without \"--all\" the command considers only the annotated tags to\n>> base the descripion on, and with \"--all\", a ref that is not\n>> annotated tags can be used as a base, but with a lower priority (if\n>> an annotated tag can describe a given commit, that tag is used).\n>> \n>> So naïvely I would expect \"--all\" and \"--match\" to base the\n>> description on refs that match the pattern without limiting the\n>> choice of base to annotated tags, and refs that do not match the\n>> given pattern should not appear even as the last resort.  It appears\n>> to me that the current situation is (3).\n>\n> Hmm.  It seems to me that \"--all\" says two things:\n>\n>  (a) allow unannotated (rather than only annotated)\n>\n>  (b) allow refs of any name (rather than only tags)\n>\n> With \"--match\", particularly because the pattern always refers only to\n> tags, (b) is obliterated, and your proposed semantics are (a) plus a\n> sort of inverse of (b):\n>\n>  (c) allow only refs matching the pattern\n\nI would think it is more like \"only (a), without changing the\ndocumented semantics of what '--all' and '--match' are by adding (b)\nor (c)\".\n\nI do not think in the longer term it is wrong per-se to change the\nsemantics of \"--match\" from the documented \"Only consider tags\nmatching the pattern\" to \"Only consider refs matching the pattern\",\nand such a change can and should be made as a separate patch\n\"describe: loosen --match to allow any ref, not just tags\" on top of\nthe patch I sent which was meant to be bugfix-only.\n"},{"id":"210542","messageId":"20130303220756.GN22203@biohazard-cafe.mit.edu","threadId":"33003","inReplyTo":"7vfw0cb2xi.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] describe: Exclude --all --match=PATTERN","fromName":"Greg Price","fromEmail":"price@mit.edu","sentAt":"2013-03-03T22:07:56Z","receivedAt":"2013-03-03T22:07:56Z","isPatch":true,"sender":{"key":"price@mit.edu","avatar":"https://avatars.githubusercontent.com/u/28173?v=4"},"body":"On Sun, Mar 03, 2013 at 01:15:21PM -0800, Junio C Hamano wrote:\n> Greg Price <price@MIT.EDU> writes:\n> > It seems to me that \"--all\" says two things:\n> >\n> >  (a) allow unannotated (rather than only annotated)\n> >\n> >  (b) allow refs of any name (rather than only tags)\n> >\n> > With \"--match\", particularly because the pattern always refers only to\n> > tags, (b) is obliterated, and your proposed semantics are (a) plus a\n> > sort of inverse of (b):\n> >\n> >  (c) allow only refs matching the pattern\n> \n> I would think it is more like \"only (a), without changing the\n> documented semantics of what '--all' and '--match' are by adding (b)\n> or (c)\".\n\nPerhaps I'm confused somehow?  I believe (a) and (b) together are the\ndocumented semantics of what '--all' is.  And I believe (c), which\ncontradicts (b) and indeed goes in the opposite direction, is the\ndocumented semantics of what '--match' is.\n\nCertainly we could choose to resolve the conflict by saying that\n'--match' overrides '--all', so that '--all' plus '--match' means\n(a)+(c).  I believe that's what you suggested.\n\nI think it would be preferable to recognize the conflict and let the\nuser sort out what they actually mean, because if they (or their\nscript) gave these options together then I think there's a substantial\nlikelihood that they are confused or their script is buggy.  If they\nmean (a)+(c), they can get it more clearly with '--tag --match'.\n\n\n\n> I do not think in the longer term it is wrong per-se to change the\n> semantics of \"--match\" from the documented \"Only consider tags\n> matching the pattern\" to \"Only consider refs matching the pattern\",\n> and such a change can and should be made as a separate patch\n> \"describe: loosen --match to allow any ref, not just tags\" on top of\n> the patch I sent which was meant to be bugfix-only.\n\nYeah, that could be useful.  What form of pattern would you suggest\nthat the new '--match' accept?  The obvious, and unambiguous, form of\npattern is glob patterns on the full ref name, as with 'for-each-ref'.\nThose are a little unwieldy for interactive use, but are perfect for a\nscript.  And probably 'describe --match' itself already makes the most\nsense in a scripted context, as for interactive use one can go into\n'gitk' or 'git log --oneline --decorate' or the like and find out the\nsame information plus more detail.\n\nParticularly when handling both tags and other refs, I can also\nimagine wanting to specify a disjunction of several patterns.  So we\nmight want to accept the option several times cumulatively.\n\nI'd worry about changing the semantics of existing 'describe --match'\ninvocations in people's scripts, etc.  Perhaps we'd give it a new name\nlike '--match-ref'.  Or we could say something like, if it starts with\n\"refs/\" it's a pattern on the whole refname, else it's a pattern on\njust the tag name, and accept that if someone has a ref called\n\"refs/tags/refs/foo\" and was finding it with 'describe --match' then\nthat will break until they edit the pattern.\n\nCheers,\nGreg\n"}]}