{"thread":{"id":"40138","subject":"[PATCH] describe: make '--always' fallback work after '--exact-match' failed","startedAt":"2015-08-20T12:13:43Z","lastAt":"2015-08-24T18:47:58Z","messageCount":9,"participants":["SZEDER Gábor","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"268360","messageId":"1440072823-18024-1-git-send-email-szeder@ira.uka.de","threadId":"40138","inReplyTo":null,"subject":"[PATCH] describe: make '--always' fallback work after '--exact-match' failed","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2015-08-20T12:13:43Z","receivedAt":"2015-08-20T12:13:43Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"'git describe [...] --always' should always show the unique abbreviated\nobject name as a fallback when the given commit cannot be described with\nthe given set of options, see da2478dbb0 (describe --always: fall back\nto showing an abbreviated object name, 2008-03-02).\n\nHowever, this is not the case when the combination '--exact-match\n--always' is given and the commit cannot be described, because in such\ncases 'git describe' errors out, as if '--always' were not given at all.\n\nRespect '--always' and print the unique abbreviated object name instead\nof erroring out when the commit cannot be described with '--exact-match\n--always'.\n\nSigned-off-by: SZEDER Gábor \n---\n builtin/describe.c | 2 +-\n t/t6120-describe.sh | 2 ++\n 2 files changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex a36c829e57..ce36032b1f 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -266,7 +266,7 @@ static void describe(const char *arg, int last_one)\n \t\treturn;\n \t}\n \n-\tif (!max_candidates)\n+\tif (!always && !max_candidates)\n \t\tdie(_(\"no tag exactly matches '%s'\"), sha1_to_hex(cmit->object.sha1));\n \tif (debug)\n \t\tfprintf(stderr, _(\"searching to describe %s\\n\"), arg);\ndiff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\nindex c0e5b2a627..57d50156bb 100755\n--- a/t/t6120-describe.sh\n+++ b/t/t6120-describe.sh\n@@ -113,6 +113,8 @@ check_describe A-3-* --long HEAD^^2\n check_describe c-7-* --tags\n check_describe e-3-* --first-parent --tags\n \n+check_describe $(git rev-parse --short HEAD) --exact-match --always HEAD\n+\n : >err.expect\n check_describe A --all A^0\n test_expect_success 'no warning was displayed for A' '\n-- \n2.5.0.343.g6f38143\n"},{"id":"268382","messageId":"xmqqegix7qjk.fsf@gitster.dls.corp.google.com","threadId":"40138","inReplyTo":"1440072823-18024-1-git-send-email-szeder@ira.uka.de","subject":"Re: [PATCH] describe: make '--always' fallback work after '--exact-match' failed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-20T18:39:27Z","receivedAt":"2015-08-20T18:39:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> 'git describe [...] --always' should always show the unique abbreviated\n> object name as a fallback when the given commit cannot be described with\n> the given set of options, see da2478dbb0 (describe --always: fall back\n> to showing an abbreviated object name, 2008-03-02).\n>\n> However, this is not the case when the combination '--exact-match\n> --always' is given and the commit cannot be described, because in such\n> cases 'git describe' errors out, as if '--always' were not given at all.\n>\n> Respect '--always' and print the unique abbreviated object name instead\n> of erroring out when the commit cannot be described with '--exact-match\n> --always'.\n>\n> Signed-off-by: SZEDER Gábor \n\nWell, that can be argued both ways. Your patch introduces a\nregression, as \"--exact-match\" is an instruction to error out when\nno tag exactly matches, and you deliberately break that.\n\nMy knee-jerk reaction is that the most sensible way forward is to\nmake --exact-match and --always mutually incompatible.\n"},{"id":"268407","messageId":"20150821134039.Horde.wW7OPRQO28gZidreZRgP7g2@webmail.informatik.kit.edu","threadId":"40138","inReplyTo":"xmqqegix7qjk.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] describe: make '--always' fallback work after '--exact-match' failed","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2015-08-21T11:40:39Z","receivedAt":"2015-08-21T11:40:39Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\nQuoting Junio C Hamano :\n\n> SZEDER Gábor writes:\n>\n>> 'git describe [...] --always' should always show the unique abbreviated\n>> object name as a fallback when the given commit cannot be described with\n>> the given set of options, see da2478dbb0 (describe --always: fall back\n>> to showing an abbreviated object name, 2008-03-02).\n>>\n>> However, this is not the case when the combination '--exact-match\n>> --always' is given and the commit cannot be described, because in such\n>> cases 'git describe' errors out, as if '--always' were not given at all.\n>>\n>> Respect '--always' and print the unique abbreviated object name instead\n>> of erroring out when the commit cannot be described with '--exact-match\n>> --always'.\n>>\n>> Signed-off-by: SZEDER Gábor \n>\n> Well, that can be argued both ways. Your patch introduces a\n> regression, as \"--exact-match\" is an instruction to error out when\n> no tag exactly matches, and you deliberately break that.\n\nThis patch doesn't break '--exact-match', in fact doesn't modify it at all\nwhen it's on its own or combined with other options, but it makes\n'--exact-match --always' finally work.\n\n'git describe' errors out by default if it can't describe the given\ncommit. So if a user wants an exact match or an error otherwise, then he\nshould not give '--always' at all, because that's the instruction to not\nerror out but give the abbreviated object name instead.\n\nWhy should '--exact-match' be any different from the other options that\ntell 'git describe' what to use for the description? Why should\n'--always' not work with '--exact-match', when it works in the other\ncases?\n\nConsider '--contains': it should find a tag that comes after the given\ncommit or error out if such a tag doesn't exist. Now, in current git.git:\n\n $ git describe --contains master\n fatal: cannot describe 'ff86faf2fa02bc21933c9e1dcc75c8d81b3e104a'\n $ git describe --contains --always master\n ff86faf2fa\n\nOr the default behavior without any options: it should find a tag\nreachable from the given commit or error out, but what if we pass in a\ncommit before the first tag? It recommends '--always':\n\n $ git describe e83c516\n fatal: No tags can describe 'e83c5163316f89bfbde7d9ab23ca2e25604af290'.\n Try --always, or create some tags.\n $ git describe --always e83c516\n e83c516331\n\n\n> My knee-jerk reaction is that the most sensible way forward is to\n> make --exact-match and --always mutually incompatible.\n\nThat would be wrong, it's perfectly valid to ask for an exactly matching\ntag or, if there is no such tag, the abbreviated object name as a\nfallback.\n"},{"id":"268423","messageId":"1440168632-15412-1-git-send-email-szeder@ira.uka.de","threadId":"40138","inReplyTo":"1440072823-18024-1-git-send-email-szeder@ira.uka.de","subject":"[PATCH] describe --contains: default to HEAD when no commit-ish is given","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2015-08-21T14:50:32Z","receivedAt":"2015-08-21T14:50:32Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"'git describe --contains' doesn't default to HEAD when no commit is\ngiven, and it doesn't produce any output, not even an error:\n\n ~/src/git ((v2.5.0))$ ./git describe --contains\n ~/src/git ((v2.5.0))$ ./git describe --contains HEAD\n v2.5.0^0\n\nUnlike other 'git describe' options, the '--contains' code path is\nimplemented by calling 'name-rev' with a bunch of options plus all the\ncommit-ishes that were passed to 'git describe'. If no commit-ish was\npresent, then 'name-rev' got invoked with none, which then leads to the\nbehavior illustrated above.\n\nPorcelain commands usually default to HEAD when no commit-ish is given,\nand 'git describe' already does so in all other cases, so it should do\nso with '--contains' as well.\n\nPass HEAD to 'name-rev' when no commit-ish is given on the command line\nto make '--contains' behave consistently with other 'git describe'\noptions.\n\n'git describe's short help already indicates that the commit-ish is\noptional, but the synopsis in the man page doesn't, so update it\naccordingly as well.\n\nSigned-off-by: SZEDER Gábor \n---\n Documentation/git-describe.txt | 4 ++--\n builtin/describe.c | 11 +++++++----\n t/t6120-describe.sh | 8 ++++++++\n 3 files changed, 17 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt\nindex e045fc73f8..c8f28c8c86 100644\n--- a/Documentation/git-describe.txt\n+++ b/Documentation/git-describe.txt\n@@ -9,7 +9,7 @@ git-describe - Describe a commit using the most recent tag reachable from it\n SYNOPSIS\n --------\n [verse]\n-'git describe' [--all] [--tags] [--contains] [--abbrev=] ...\n+'git describe' [--all] [--tags] [--contains] [--abbrev=] [...]\n 'git describe' [--all] [--tags] [--contains] [--abbrev=] --dirty[=]\n \n DESCRIPTION\n@@ -27,7 +27,7 @@ see the -a and -s options to linkgit:git-tag[1].\n OPTIONS\n -------\n ...::\n-\tCommit-ish object names to describe.\n+\tCommit-ish object names to describe. Defaults to HEAD if omitted.\n \n --dirty[=]::\n \tDescribe the working tree.\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex ce36032b1f..0e31ac5cb9 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -443,10 +443,13 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \t\t\tif (pattern)\n \t\t\t\targv_array_pushf(&args, \"--refs=refs/tags/%s\", pattern);\n \t\t}\n-\t\twhile (*argv) {\n-\t\t\targv_array_push(&args, *argv);\n-\t\t\targv++;\n-\t\t}\n+\t\tif (argc)\n+\t\t\twhile (*argv) {\n+\t\t\t\targv_array_push(&args, *argv);\n+\t\t\t\targv++;\n+\t\t\t}\n+\t\telse\n+\t\t\targv_array_push(&args, \"HEAD\");\n \t\treturn cmd_name_rev(args.argc, args.argv, prefix);\n \t}\n \ndiff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\nindex 57d50156bb..bf52a0a1cc 100755\n--- a/t/t6120-describe.sh\n+++ b/t/t6120-describe.sh\n@@ -115,6 +115,14 @@ check_describe e-3-* --first-parent --tags\n \n check_describe $(git rev-parse --short HEAD) --exact-match --always HEAD\n \n+test_expect_success 'describe --contains defaults to HEAD without commit-ish' '\n+\techo \"A^0\" >expect &&\n+\tgit checkout A &&\n+\ttest_when_finished \"git checkout -\" &&\n+\tgit describe --contains >actual &&\n+\ttest_cmp expect actual\n+'\n+\n : >err.expect\n check_describe A --all A^0\n test_expect_success 'no warning was displayed for A' '\n-- \n2.5.0.416.g84b07b4\n"},{"id":"268428","messageId":"xmqqwpwo7i17.fsf@gitster.dls.corp.google.com","threadId":"40138","inReplyTo":"20150821134039.Horde.wW7OPRQO28gZidreZRgP7g2@webmail.informatik.kit.edu","subject":"Re: [PATCH] describe: make '--always' fallback work after '--exact-match' failed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-21T15:55:32Z","receivedAt":"2015-08-21T15:55:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> Quoting Junio C Hamano :\n>\n>> Well, that can be argued both ways.\n> ...\n>\n> 'git describe' errors out by default if it can't describe the given\n> commit.\n\nYes. \"describe\" always fails when it cannot produce an acceptable\ndescription.\n\nAnd \"--always\" and \"--exact-match\" are two incompatible ways for\nusers to change what is considered an acceptable description.\n\n - With \"--exact-match\" and \"--always\", the definition of\n \"acceptable\" changes: by default, only a name based on some\n anchor point is acceptable.\n\n - \"--always\" loosens the definition and also allows an\n abbreviated commit object name as acceptable.\n\n - \"--exact-match\" tightens the definition. In addition to \"only\n a name based on some anchor point is acceptable\", it requires\n that \"based on\" to be \"0 distance from the anchor point\". The\n help text says succinctly: \"*ONLY* output exact matches\".\n\nIf you allow a request with \"--exact-match\" to show something that\nis not an exact match, that no longer is \"--exact-match\". Allowing\nto mix \"--always\" to that option breaks it, as what the command does\nno longer is to \"only outputs exact matches\".\n\n - One option \"--exact-match\" says that it is wrong to show anything\n but exact matches.\n\n - The other option \"--always\" says it is willing to show a name\n that is not an exact match. \n\nThese are competing goals. You give preference to the latter, but\nthat is not the only valid point of view.\n\nAnd that is why I said it can be argued both ways. I think these\ntwo are fundamentally incompatible.\n\n\n[Footnote]\n\n*1* Other parts of \"'describe' only considers a name based on some\n anchor point as acceptable.\" are also modified by various\n options:\n\n - With \"--contains\", the definition of \"based on\" changes\n direction: a commit may be described as somewhere ahead of an\n anchor point by default, but can be described as somewhere behind\n of an anchor point.\n\n - With \"--all\" and \"--tags\", the definition of \"anchor point\"\n changes: by default, only annotated tags are possible anchor\n points.\n"},{"id":"268431","messageId":"xmqqoai07gen.fsf@gitster.dls.corp.google.com","threadId":"40138","inReplyTo":"1440168632-15412-1-git-send-email-szeder@ira.uka.de","subject":"Re: [PATCH] describe --contains: default to HEAD when no commit-ish is given","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-21T16:30:40Z","receivedAt":"2015-08-21T16:30:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> 'git describe --contains' doesn't default to HEAD when no commit is\n> given, and it doesn't produce any output, not even an error:\n>\n> ~/src/git ((v2.5.0))$ ./git describe --contains\n> ~/src/git ((v2.5.0))$ ./git describe --contains HEAD\n> v2.5.0^0\n\nGood spotting. I think defaulting to HEAD is a good move.\n\n> diff --git a/builtin/describe.c b/builtin/describe.c\n> index ce36032b1f..0e31ac5cb9 100644\n> --- a/builtin/describe.c\n> +++ b/builtin/describe.c\n> @@ -443,10 +443,13 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n> \t\t\tif (pattern)\n> \t\t\t\targv_array_pushf(&args, \"--refs=refs/tags/%s\", pattern);\n> \t\t}\n> -\t\twhile (*argv) {\n> -\t\t\targv_array_push(&args, *argv);\n> -\t\t\targv++;\n> -\t\t}\n> +\t\tif (argc)\n\n\"What would this code do to 'describe --all --contains'?\" was my\nknee-jerk reaction, but the options are all parsed by this code and\nnothing is delegated to name-rev, so 'if (!argc)' here is truly the\nlack of any revisions to be described, which is good.\n\n> +\t\t\twhile (*argv) {\n> +\t\t\t\targv_array_push(&args, *argv);\n> +\t\t\t\targv++;\n> +\t\t\t}\n> +\t\telse\n> +\t\t\targv_array_push(&args, \"HEAD\");\n\nBy the way, I usually prefer a fatter 'else' clause when everything\nelse is equal, i.e.\n\n\tif (!argc)\n \targv_array_push(&args, \"HEAD\"); /* default to HEAD */\n\telse {\n\t\twhile (*argv) {\n \t...\n\t\t}\n\t}\n\nbecause it is easy to miss tiny else-clause while reading code, but\nit is harder to miss tiny then-clause. In this case, however, the\nwhile loop can be replaced with argv_array_pushv() these days, so\nperhaps\n\n\tif (!argc)\n \targv_array_push(&args, \"HEAD\"); /* default to HEAD ... */\n\telse\n\t\targv_array_pushv(&args, argv); /* or relay what we got */\n\nor something?\n\n> \t\treturn cmd_name_rev(args.argc, args.argv, prefix);\n> \t}\n\n> \n> diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\n> index 57d50156bb..bf52a0a1cc 100755\n> --- a/t/t6120-describe.sh\n> +++ b/t/t6120-describe.sh\n> @@ -115,6 +115,14 @@ check_describe e-3-* --first-parent --tags\n> \n> check_describe $(git rev-parse --short HEAD) --exact-match --always HEAD\n> \n> +test_expect_success 'describe --contains defaults to HEAD without commit-ish' '\n> +\techo \"A^0\" >expect &&\n> +\tgit checkout A &&\n> +\ttest_when_finished \"git checkout -\" &&\n> +\tgit describe --contains >actual &&\n> +\ttest_cmp expect actual\n> +'\n> : >err.expect\n> check_describe A --all A^0\n> test_expect_success 'no warning was displayed for A' '\n"},{"id":"268577","messageId":"20150824181417.Horde.2DAlveA2ZaXbdApUl8wLOQ8@webmail.informatik.kit.edu","threadId":"40138","inReplyTo":"xmqqoai07gen.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] describe --contains: default to HEAD when no commit-ish is given","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2015-08-24T16:14:17Z","receivedAt":"2015-08-24T16:14:17Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\nQuoting Junio C Hamano :\n\n>> @@ -443,10 +443,13 @@ int cmd_describe(int argc, const char **argv, \n>> const char *prefix)\n>> \t\t\tif (pattern)\n>> \t\t\t\targv_array_pushf(&args, \"--refs=refs/tags/%s\", pattern);\n>> \t\t}\n>> -\t\twhile (*argv) {\n>> -\t\t\targv_array_push(&args, *argv);\n>> -\t\t\targv++;\n>> -\t\t}\n>> +\t\tif (argc)\n>\n> \"What would this code do to 'describe --all --contains'?\" was my\n> knee-jerk reaction, but the options are all parsed by this code and\n> nothing is delegated to name-rev, so 'if (!argc)' here is truly the\n> lack of any revisions to be described, which is good.\n\nExactly. parse-opts removes all --options from argv as it processes\nthem, barfs at --unknown-options, so all what remains must be treated\nas a commit-ish. And if nothing is left, well, then there was none\ngiven.\n\n>> +\t\t\twhile (*argv) {\n>> +\t\t\t\targv_array_push(&args, *argv);\n>> +\t\t\t\targv++;\n>> +\t\t\t}\n>> +\t\telse\n>> +\t\t\targv_array_push(&args, \"HEAD\");\n>\n> By the way, I usually prefer a fatter 'else' clause when everything\n> else is equal, i.e.\n>\n> \tif (!argc)\n> \targv_array_push(&args, \"HEAD\"); /* default to HEAD */\n> \telse {\n> \t\twhile (*argv) {\n> \t...\n> \t\t}\n> \t}\n>\n> because it is easy to miss tiny else-clause while reading code, but\n> it is harder to miss tiny then-clause. In this case, however, the\n> while loop can be replaced with argv_array_pushv() these days, so\n> perhaps\n>\n> \tif (!argc)\n> \targv_array_push(&args, \"HEAD\"); /* default to HEAD ... */\n> \telse\n> \t\targv_array_pushv(&args, argv); /* or relay what we got */\n>\n> or something?\n\nIndeed, I didn't notice argv_array_pushv() being added, log tells me\nit happened quite recently. I suppose with both branches becoming a\none-liner the order of them can remain what it was in the patch,\nthis sparing the negation from 'if (!argc)'.\n\nv2 comes in a minute.\n\n\nGábor\n"},{"id":"268578","messageId":"1440432918-27751-1-git-send-email-szeder@ira.uka.de","threadId":"40138","inReplyTo":"20150824181417.Horde.2DAlveA2ZaXbdApUl8wLOQ8@webmail.informatik.kit.edu","subject":"[PATCH v2] describe --contains: default to HEAD when no commit-ish is given","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2015-08-24T16:15:18Z","receivedAt":"2015-08-24T16:15:18Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"'git describe --contains' doesn't default to HEAD when no commit is\ngiven, and it doesn't produce any output, not even an error:\n\n ~/src/git ((v2.5.0))$ ./git describe --contains\n ~/src/git ((v2.5.0))$ ./git describe --contains HEAD\n v2.5.0^0\n\nUnlike other 'git describe' options, the '--contains' code path is\nimplemented by calling 'name-rev' with a bunch of options plus all the\ncommit-ishes that were passed to 'git describe'. If no commit-ish was\npresent, then 'name-rev' got invoked with none, which then leads to the\nbehavior illustrated above.\n\nPorcelain commands usually default to HEAD when no commit-ish is given,\nand 'git describe' already does so in all other cases, so it should do\nso with '--contains' as well.\n\nPass HEAD to 'name-rev' when no commit-ish is given on the command line\nto make '--contains' behave consistently with other 'git describe'\noptions. While at it, use argv_array_pushv() instead of the loop to\npass commit-ishes to 'git name-rev'.\n\n'git describe's short help already indicates that the commit-ish is\noptional, but the synopsis in the man page doesn't, so update it\naccordingly as well.\n\nSigned-off-by: SZEDER Gábor \n---\n Documentation/git-describe.txt | 4 ++--\n builtin/describe.c | 8 ++++----\n t/t6120-describe.sh | 8 ++++++++\n 3 files changed, 14 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt\nindex e045fc73f8..c8f28c8c86 100644\n--- a/Documentation/git-describe.txt\n+++ b/Documentation/git-describe.txt\n@@ -9,7 +9,7 @@ git-describe - Describe a commit using the most recent tag reachable from it\n SYNOPSIS\n --------\n [verse]\n-'git describe' [--all] [--tags] [--contains] [--abbrev=] ...\n+'git describe' [--all] [--tags] [--contains] [--abbrev=] [...]\n 'git describe' [--all] [--tags] [--contains] [--abbrev=] --dirty[=]\n \n DESCRIPTION\n@@ -27,7 +27,7 @@ see the -a and -s options to linkgit:git-tag[1].\n OPTIONS\n -------\n ...::\n-\tCommit-ish object names to describe.\n+\tCommit-ish object names to describe. Defaults to HEAD if omitted.\n \n --dirty[=]::\n \tDescribe the working tree.\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex ce36032b1f..9dadfb58c8 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -443,10 +443,10 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \t\t\tif (pattern)\n \t\t\t\targv_array_pushf(&args, \"--refs=refs/tags/%s\", pattern);\n \t\t}\n-\t\twhile (*argv) {\n-\t\t\targv_array_push(&args, *argv);\n-\t\t\targv++;\n-\t\t}\n+\t\tif (argc)\n+\t\t\targv_array_pushv(&args, argv);\n+\t\telse\n+\t\t\targv_array_push(&args, \"HEAD\");\n \t\treturn cmd_name_rev(args.argc, args.argv, prefix);\n \t}\n \ndiff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\nindex 57d50156bb..bf52a0a1cc 100755\n--- a/t/t6120-describe.sh\n+++ b/t/t6120-describe.sh\n@@ -115,6 +115,14 @@ check_describe e-3-* --first-parent --tags\n \n check_describe $(git rev-parse --short HEAD) --exact-match --always HEAD\n \n+test_expect_success 'describe --contains defaults to HEAD without commit-ish' '\n+\techo \"A^0\" >expect &&\n+\tgit checkout A &&\n+\ttest_when_finished \"git checkout -\" &&\n+\tgit describe --contains >actual &&\n+\ttest_cmp expect actual\n+'\n+\n : >err.expect\n check_describe A --all A^0\n test_expect_success 'no warning was displayed for A' '\n-- \n2.5.0.418.gdd37a9b\n"},{"id":"268597","messageId":"xmqqd1yc4j6p.fsf@gitster.dls.corp.google.com","threadId":"40138","inReplyTo":"20150824181417.Horde.2DAlveA2ZaXbdApUl8wLOQ8@webmail.informatik.kit.edu","subject":"Re: [PATCH] describe --contains: default to HEAD when no commit-ish is given","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-24T18:47:58Z","receivedAt":"2015-08-24T18:47:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n>> By the way, I usually prefer a fatter 'else' clause when everything\n>> else is equal, i.e.\n>>\n>> \tif (!argc)\n>> \targv_array_push(&args, \"HEAD\"); /* default to HEAD */\n>> \telse {\n>> \t\twhile (*argv) {\n>> \t...\n>> \t\t}\n>> \t}\n>>\n>> because it is easy to miss tiny else-clause while reading code, but\n>> it is harder to miss tiny then-clause. In this case, however, the\n>> while loop can be replaced with argv_array_pushv() these days, so\n>> perhaps\n>>\n>> \tif (!argc)\n>> \targv_array_push(&args, \"HEAD\"); /* default to HEAD ... */\n>> \telse\n>> \t\targv_array_pushv(&args, argv); /* or relay what we got */\n>>\n>> or something?\n>\n> Indeed, I didn't notice argv_array_pushv() being added, log tells me\n> it happened quite recently. I suppose with both branches becoming a\n> one-liner the order of them can remain what it was in the patch,\n> this sparing the negation from 'if (!argc)'.\n\nAnother reason to favor the way the code is illustrated for\neducational purposes above is it is easier to see exceptional case\nfirst, i.e. \"if we have nothing then we do this special thing, but\notherwise we do the normal thing\".\n\nBut that is a much weaker preference than the preference to \"fatter\nelse\"; I could go either way.\n\n> v2 comes in a minute.\n\nThanks.\n"}]}