{"thread":{"id":"37545","subject":"[PATCH 0/2] describe: support the syntax \"--abbrev=+\"","startedAt":"2014-09-12T14:26:42Z","lastAt":"2014-09-14T08:56:38Z","messageCount":6,"participants":["Jonh Wendell","Jeff King","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"249305","messageId":"1410532004-22769-1-git-send-email-jonh.wendell@gmail.com","threadId":"37545","inReplyTo":null,"subject":"[PATCH 0/2] describe: support the syntax \"--abbrev=+\"","fromName":"Jonh Wendell","fromEmail":"jonh.wendell@gmail.com","sentAt":"2014-09-12T14:26:42Z","receivedAt":"2014-09-12T14:26:42Z","isPatch":true,"sender":{"key":"jonh.wendell@gmail.com","avatar":"https://gravatar.com/avatar/9f343d0de3c38bc408ce71f2b42077877daeae03a12037272e02b7da60068aea?d=mp&s=160"},"body":"Sometimes it's interesting to have a simple output that answers the question:\nAre there commits after the latest tag?\n\nOne possible solution is to just print a \"+\" (plus) signal after the tag. Example:\n\n> git describe --abbrev=1 5261ec5d5\nv2.1.0-rc1-2-g5261ec\n\n> git describe --abbrev=+ 5261ec5d5\nv2.1.0-rc1+\n\nFirst patch was sent in Aug 23, re-sending with Signed-off-by and\nCC'ing Junio.\n\nJonh Wendell (2):\n  describe: support the syntax \"--abbrev=+\"\n  describe: Add documentation for \"--abbrev=+\"\n\n Documentation/git-describe.txt |  6 ++++++\n builtin/describe.c             | 26 +++++++++++++++++++++-----\n 2 files changed, 27 insertions(+), 5 deletions(-)\n\n-- \n1.9.3\n"},{"id":"249306","messageId":"1410532004-22769-2-git-send-email-jonh.wendell@gmail.com","threadId":"37545","inReplyTo":"1410532004-22769-1-git-send-email-jonh.wendell@gmail.com","subject":"[PATCH 1/2] describe: support the syntax \"--abbrev=+\"","fromName":"Jonh Wendell","fromEmail":"jonh.wendell@gmail.com","sentAt":"2014-09-12T14:26:43Z","receivedAt":"2014-09-12T14:26:43Z","isPatch":true,"sender":{"key":"jonh.wendell@gmail.com","avatar":"https://gravatar.com/avatar/9f343d0de3c38bc408ce71f2b42077877daeae03a12037272e02b7da60068aea?d=mp&s=160"},"body":"It will print just a \"+\" sign appended to the found tag, if there\nare commits between the tag and the supplied commit.\n\nIt's useful when you just need a simple output to know if the\nsupplied commit is an exact match or not.\n\nSigned-off-by: Jonh Wendell <jonh.wendell@gmail.com>\n---\n builtin/describe.c | 26 +++++++++++++++++++++-----\n 1 file changed, 21 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex ee6a3b9..3a5c052 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -30,6 +30,7 @@ static int have_util;\n static const char *pattern;\n static int always;\n static const char *dirty;\n+static int simple_abbrev = 0;\n \n /* diff-index command arguments to check if working tree is dirty. */\n static const char *diff_index_args[] = {\n@@ -378,8 +379,12 @@ static void describe(const char *arg, int last_one)\n \t}\n \n \tdisplay_name(all_matches[0].name);\n-\tif (abbrev)\n-\t\tshow_suffix(all_matches[0].depth, cmit->object.sha1);\n+\tif (abbrev) {\n+\t\tif (simple_abbrev)\n+\t\t\tprintf(\"+\");\n+\t\telse\n+\t\t\tshow_suffix(all_matches[0].depth, cmit->object.sha1);\n+\t}\n \tif (dirty)\n \t\tprintf(\"%s\", dirty);\n \tprintf(\"\\n\");\n@@ -388,6 +393,16 @@ static void describe(const char *arg, int last_one)\n \t\tclear_commit_marks(cmit, -1);\n }\n \n+static int parse_opt_abbrev_for_describe_cb(const struct option *opt, const char *arg, int unset)\n+{\n+\tif (arg && !strncmp(arg, \"+\", 1)) {\n+\t\tsimple_abbrev = 1;\n+\t\treturn 0;\n+\t}\n+\n+\treturn parse_opt_abbrev_cb(opt, arg, unset);\n+}\n+\n int cmd_describe(int argc, const char **argv, const char *prefix)\n {\n \tint contains = 0;\n@@ -398,7 +413,6 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"tags\",       &tags, N_(\"use any tag, even unannotated\")),\n \t\tOPT_BOOL(0, \"long\",       &longformat, N_(\"always use long format\")),\n \t\tOPT_BOOL(0, \"first-parent\", &first_parent, N_(\"only follow first parent\")),\n-\t\tOPT__ABBREV(&abbrev),\n \t\tOPT_SET_INT(0, \"exact-match\", &max_candidates,\n \t\t\t    N_(\"only output exact matches\"), 0),\n \t\tOPT_INTEGER(0, \"candidates\", &max_candidates,\n@@ -410,6 +424,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \t\t{OPTION_STRING, 0, \"dirty\",  &dirty, N_(\"mark\"),\n \t\t\tN_(\"append <mark> on dirty working tree (default: \\\"-dirty\\\")\"),\n \t\t\tPARSE_OPT_OPTARG, NULL, (intptr_t) \"-dirty\"},\n+\t\t{OPTION_CALLBACK, 0, \"abbrev\", &abbrev, N_(\"n\"), N_(\"use <n> digits to display SHA-1s\"),\n+\t\t\tPARSE_OPT_OPTARG, &parse_opt_abbrev_for_describe_cb, 0},\n \t\tOPT_END(),\n \t};\n \n@@ -425,8 +441,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \n \tsave_commit_buffer = 0;\n \n-\tif (longformat && abbrev == 0)\n-\t\tdie(_(\"--long is incompatible with --abbrev=0\"));\n+\tif (longformat && (abbrev == 0 || simple_abbrev))\n+\t\tdie(_(\"--long is incompatible with --abbrev=+ or --abbrev=0\"));\n \n \tif (contains) {\n \t\tstruct argv_array args;\n-- \n1.9.3\n"},{"id":"249307","messageId":"1410532004-22769-3-git-send-email-jonh.wendell@gmail.com","threadId":"37545","inReplyTo":"1410532004-22769-1-git-send-email-jonh.wendell@gmail.com","subject":"[PATCH 2/2] describe: Add documentation for \"--abbrev=+\"","fromName":"Jonh Wendell","fromEmail":"jonh.wendell@gmail.com","sentAt":"2014-09-12T14:26:44Z","receivedAt":"2014-09-12T14:26:44Z","isPatch":true,"sender":{"key":"jonh.wendell@gmail.com","avatar":"https://gravatar.com/avatar/9f343d0de3c38bc408ce71f2b42077877daeae03a12037272e02b7da60068aea?d=mp&s=160"},"body":"Signed-off-by: Jonh Wendell <jonh.wendell@gmail.com>\n---\n Documentation/git-describe.txt | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt\nindex d20ca40..e291770 100644\n--- a/Documentation/git-describe.txt\n+++ b/Documentation/git-describe.txt\n@@ -54,6 +54,12 @@ OPTIONS\n \tabbreviated object name, use <n> digits, or as many digits\n \tas needed to form a unique object name.  An <n> of 0\n \twill suppress long format, only showing the closest tag.\n+\t+\n+\t+\n+\tA special case of <n> equals to \"\\+\" (without quotes) will print\n+\tjust a \"+\" sign instead of the whole suffix. This is useful if you\n+\tonly need to know if the supplied <commit-ish> points to an exact\n+\tmatch or if there are commits between the tag found and the <commit-ish>.\n \n --candidates=<n>::\n \tInstead of considering only the 10 most recent tags as\n-- \n1.9.3\n"},{"id":"249386","messageId":"20140914081847.GA20526@peff.net","threadId":"37545","inReplyTo":"1410532004-22769-2-git-send-email-jonh.wendell@gmail.com","subject":"Re: [PATCH 1/2] describe: support the syntax \"--abbrev=+\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-09-14T08:18:48Z","receivedAt":"2014-09-14T08:18:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 12, 2014 at 11:26:43AM -0300, Jonh Wendell wrote:\n\n> It will print just a \"+\" sign appended to the found tag, if there\n> are commits between the tag and the supplied commit.\n> \n> It's useful when you just need a simple output to know if the\n> supplied commit is an exact match or not.\n\nSeems like a reasonable extension of the \"--abbrev=0\" behavior.\n\n>  builtin/describe.c | 26 +++++++++++++++++++++-----\n>  1 file changed, 21 insertions(+), 5 deletions(-)\n\nYou can probably just squash the related documentation in with this\npatch. Also, maybe some tests in t6120? It doesn't look like we test\n--abbrev=0, either; if you are feeling especially charitable, it might\nbe good to add some tests for it, too.\n\n> @@ -378,8 +379,12 @@ static void describe(const char *arg, int last_one)\n>  \t}\n>  \n>  \tdisplay_name(all_matches[0].name);\n> -\tif (abbrev)\n> -\t\tshow_suffix(all_matches[0].depth, cmit->object.sha1);\n> +\tif (abbrev) {\n> +\t\tif (simple_abbrev)\n> +\t\t\tprintf(\"+\");\n> +\t\telse\n> +\t\t\tshow_suffix(all_matches[0].depth, cmit->object.sha1);\n> +\t}\n\nThis covers the case when we do have a commit to show. The exact-match\ncase is handled elsewhere, and I wondered what would happen if you\npassed \"--long\", but:\n\n> +\tif (longformat && (abbrev == 0 || simple_abbrev))\n> +\t\tdie(_(\"--long is incompatible with --abbrev=+ or --abbrev=0\"));\n\nYou cover that here. Good.\n\n> +static int parse_opt_abbrev_for_describe_cb(const struct option *opt, const char *arg, int unset)\n> +{\n> +\tif (arg && !strncmp(arg, \"+\", 1)) {\n\nWhy strncmp here? If I pass \"--abbrev=+10\", shouldn't that be an error?\n\n> +\t\tsimple_abbrev = 1;\n> +\t\treturn 0;\n> +\t}\n> +\n> +\treturn parse_opt_abbrev_cb(opt, arg, unset);\n> +}\n\nWhat happens if you pass the option multiple times? I'd expect later\nones to override earlier ones. For \"--abbrev=0 --abbrev=10\" this just\nworks, because they both store the value in the abbrev variable. But you\nstore simple_abbrev as a separate variable.\n\nWhat do these do?\n\n  1. --abbrev=10 --abbrev=+\n\n  2. --abbrev=+ --abbrev=10\n\n  3. --abbrev=0 --abbrev=+\n\nThe first one will respect simple_abbrev, since it avoids calling\nshow_suffix at all. Good. The second one will do the same. We probably\nneed to reset simple_abbrev to 0 whenever we see another --abbrev. The\nthird one will not respect simple_abbrev, because we never enter the \"if\n(abbrev)\" conditional. We probably need to reset \"abbrev\" to something\nnon-zero when we set simple_abbrev.\n\nI.e.:\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex 3a5c052..532161e 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -397,9 +397,11 @@ static int parse_opt_abbrev_for_describe_cb(const struct option *opt, const char\n {\n \tif (arg && !strncmp(arg, \"+\", 1)) {\n \t\tsimple_abbrev = 1;\n+\t\tabbrev = 1; /* doesn't matter as long as it is non-zero */\n \t\treturn 0;\n \t}\n \n+\tsimple_abbrev = 0;\n \treturn parse_opt_abbrev_cb(opt, arg, unset);\n }\n \n\nAnother alternative would be to stuff the simple_abbrev flag into\nan unused value of \"abbrev\" (say, -2), but that is probably a little\nless obvious than just resetting them together as above.\n\n-Peff\n"},{"id":"249388","messageId":"20140914082302.GB20526@peff.net","threadId":"37545","inReplyTo":"1410532004-22769-3-git-send-email-jonh.wendell@gmail.com","subject":"Re: [PATCH 2/2] describe: Add documentation for \"--abbrev=+\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-09-14T08:23:02Z","receivedAt":"2014-09-14T08:23:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 12, 2014 at 11:26:44AM -0300, Jonh Wendell wrote:\n\n> --- a/Documentation/git-describe.txt\n> +++ b/Documentation/git-describe.txt\n> @@ -54,6 +54,12 @@ OPTIONS\n>  \tabbreviated object name, use <n> digits, or as many digits\n>  \tas needed to form a unique object name.  An <n> of 0\n>  \twill suppress long format, only showing the closest tag.\n> +\t+\n> +\t+\n\nWhy two continuation lines here? One seems to be enough on my version of\nasciidoc (though it would not be the first time we have encountered\nweird behaviors in our documentation toolchain).\n\n> +\tA special case of <n> equals to \"\\+\" (without quotes) will print\n> +\tjust a \"+\" sign instead of the whole suffix. This is useful if you\n> +\tonly need to know if the supplied <commit-ish> points to an exact\n> +\tmatch or if there are commits between the tag found and the <commit-ish>.\n\nI found this first phrase a little hard to parse. What about just:\n\n  An <n> of `+` will print...\n\nThat uses the same wording as above, and I think using backticks is\nprobably the right thing. It will appear in a monospaced font, which\navoids the need to explain the quotes (and you also do not need to\nbackslash-escape inside it).\n\n-Peff\n"},{"id":"249393","messageId":"CAPig+cQJS+fFTLPXBa1a0-TH-FSAJaHX4w49CsxnjRKgsAsiSw@mail.gmail.com","threadId":"37545","inReplyTo":"20140914081847.GA20526@peff.net","subject":"Re: [PATCH 1/2] describe: support the syntax \"--abbrev=+\"","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-09-14T08:56:38Z","receivedAt":"2014-09-14T08:56:38Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Sep 14, 2014 at 4:18 AM, Jeff King <peff@peff.net> wrote:\n> On Fri, Sep 12, 2014 at 11:26:43AM -0300, Jonh Wendell wrote:\n>\n>> It will print just a \"+\" sign appended to the found tag, if there\n>> are commits between the tag and the supplied commit.\n>>\n>> It's useful when you just need a simple output to know if the\n>> supplied commit is an exact match or not.\n>\n> Seems like a reasonable extension of the \"--abbrev=0\" behavior.\n\nMy reaction is opposite: Such overloading of --abbrev= feels abusive,\nnon-obvious, and is inconsistent with --abbrev accepted by other\ncommands.\n\nIt's also potentially ambiguous. If the tag name ends with a '+' and\nthere are no commits atop it, then the client can be fooled into\nthinking there are. Being able to configure the suffix, rather than\nhardcoding '+', might help, but seems ugly.\n\nThe justification in the cover letter is that it provides a way to\ncheck if there are commits atop the latest tag. Within a script, it\nmight be sufficient merely to compare the output of 'git describe' and\n'git describe --abbrev=0'. If they differ, then there are commits atop\nthe latest tag.\n\nThus, this feature seems somewhat misguided, but perhaps I'm missing\nsomething obvious.\n\n>>  builtin/describe.c | 26 +++++++++++++++++++++-----\n>>  1 file changed, 21 insertions(+), 5 deletions(-)\n>\n> You can probably just squash the related documentation in with this\n> patch. Also, maybe some tests in t6120? It doesn't look like we test\n> --abbrev=0, either; if you are feeling especially charitable, it might\n> be good to add some tests for it, too.\n>\n>> @@ -378,8 +379,12 @@ static void describe(const char *arg, int last_one)\n>>       }\n>>\n>>       display_name(all_matches[0].name);\n>> -     if (abbrev)\n>> -             show_suffix(all_matches[0].depth, cmit->object.sha1);\n>> +     if (abbrev) {\n>> +             if (simple_abbrev)\n>> +                     printf(\"+\");\n>> +             else\n>> +                     show_suffix(all_matches[0].depth, cmit->object.sha1);\n>> +     }\n>\n> This covers the case when we do have a commit to show. The exact-match\n> case is handled elsewhere, and I wondered what would happen if you\n> passed \"--long\", but:\n>\n>> +     if (longformat && (abbrev == 0 || simple_abbrev))\n>> +             die(_(\"--long is incompatible with --abbrev=+ or --abbrev=0\"));\n>\n> You cover that here. Good.\n>\n>> +static int parse_opt_abbrev_for_describe_cb(const struct option *opt, const char *arg, int unset)\n>> +{\n>> +     if (arg && !strncmp(arg, \"+\", 1)) {\n>\n> Why strncmp here? If I pass \"--abbrev=+10\", shouldn't that be an error?\n>\n>> +             simple_abbrev = 1;\n>> +             return 0;\n>> +     }\n>> +\n>> +     return parse_opt_abbrev_cb(opt, arg, unset);\n>> +}\n>\n> What happens if you pass the option multiple times? I'd expect later\n> ones to override earlier ones. For \"--abbrev=0 --abbrev=10\" this just\n> works, because they both store the value in the abbrev variable. But you\n> store simple_abbrev as a separate variable.\n>\n> What do these do?\n>\n>   1. --abbrev=10 --abbrev=+\n>\n>   2. --abbrev=+ --abbrev=10\n>\n>   3. --abbrev=0 --abbrev=+\n>\n> The first one will respect simple_abbrev, since it avoids calling\n> show_suffix at all. Good. The second one will do the same. We probably\n> need to reset simple_abbrev to 0 whenever we see another --abbrev. The\n> third one will not respect simple_abbrev, because we never enter the \"if\n> (abbrev)\" conditional. We probably need to reset \"abbrev\" to something\n> non-zero when we set simple_abbrev.\n>\n> I.e.:\n>\n> diff --git a/builtin/describe.c b/builtin/describe.c\n> index 3a5c052..532161e 100644\n> --- a/builtin/describe.c\n> +++ b/builtin/describe.c\n> @@ -397,9 +397,11 @@ static int parse_opt_abbrev_for_describe_cb(const struct option *opt, const char\n>  {\n>         if (arg && !strncmp(arg, \"+\", 1)) {\n>                 simple_abbrev = 1;\n> +               abbrev = 1; /* doesn't matter as long as it is non-zero */\n>                 return 0;\n>         }\n>\n> +       simple_abbrev = 0;\n>         return parse_opt_abbrev_cb(opt, arg, unset);\n>  }\n>\n>\n> Another alternative would be to stuff the simple_abbrev flag into\n> an unused value of \"abbrev\" (say, -2), but that is probably a little\n> less obvious than just resetting them together as above.\n>\n> -Peff\n"}]}