[PATCH 0/2] describe: support the syntax "--abbrev=+"

[PATCH 0/2] describe: support the syntax "--abbrev=+"

[Jonh Wendell]

Sometimes it's interesting to have a simple output that answers the question:
Are there commits after the latest tag?

One possible solution is to just print a "+" (plus) signal after the tag. Example:

> git describe --abbrev=1 5261ec5d5
v2.1.0-rc1-2-g5261ec

> git describe --abbrev=+ 5261ec5d5
v2.1.0-rc1+

First patch was sent in Aug 23, re-sending with Signed-off-by and
CC'ing Junio.

Jonh Wendell (2):
  describe: support the syntax "--abbrev=+"
  describe: Add documentation for "--abbrev=+"

 Documentation/git-describe.txt |  6 ++++++
 builtin/describe.c             | 26 +++++++++++++++++++++-----
 2 files changed, 27 insertions(+), 5 deletions(-)

-- 
1.9.3

[PATCH 1/2] describe: support the syntax "--abbrev=+"

[Jonh Wendell]

It will print just a "+" sign appended to the found tag, if there
are commits between the tag and the supplied commit.

It's useful when you just need a simple output to know if the
supplied commit is an exact match or not.

Signed-off-by: Jonh Wendell <jonh.wendell@gmail.com>
---
 builtin/describe.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/builtin/describe.c b/builtin/describe.c
index ee6a3b9..3a5c052 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -30,6 +30,7 @@ static int have_util;
 static const char *pattern;
 static int always;
 static const char *dirty;
+static int simple_abbrev = 0;
 
 /* diff-index command arguments to check if working tree is dirty. */
 static const char *diff_index_args[] = {
@@ -378,8 +379,12 @@ static void describe(const char *arg, int last_one)
 	}
 
 	display_name(all_matches[0].name);
-	if (abbrev)
-		show_suffix(all_matches[0].depth, cmit->object.sha1);
+	if (abbrev) {
+		if (simple_abbrev)
+			printf("+");
+		else
+			show_suffix(all_matches[0].depth, cmit->object.sha1);
+	}
 	if (dirty)
 		printf("%s", dirty);
 	printf("\n");
@@ -388,6 +393,16 @@ static void describe(const char *arg, int last_one)
 		clear_commit_marks(cmit, -1);
 }
 
+static int parse_opt_abbrev_for_describe_cb(const struct option *opt, const char *arg, int unset)
+{
+	if (arg && !strncmp(arg, "+", 1)) {
+		simple_abbrev = 1;
+		return 0;
+	}
+
+	return parse_opt_abbrev_cb(opt, arg, unset);
+}
+
 int cmd_describe(int argc, const char **argv, const char *prefix)
 {
 	int contains = 0;
@@ -398,7 +413,6 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "tags",       &tags, N_("use any tag, even unannotated")),
 		OPT_BOOL(0, "long",       &longformat, N_("always use long format")),
 		OPT_BOOL(0, "first-parent", &first_parent, N_("only follow first parent")),
-		OPT__ABBREV(&abbrev),
 		OPT_SET_INT(0, "exact-match", &max_candidates,
 			    N_("only output exact matches"), 0),
 		OPT_INTEGER(0, "candidates", &max_candidates,
@@ -410,6 +424,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 		{OPTION_STRING, 0, "dirty",  &dirty, N_("mark"),
 			N_("append <mark> on dirty working tree (default: \"-dirty\")"),
 			PARSE_OPT_OPTARG, NULL, (intptr_t) "-dirty"},
+		{OPTION_CALLBACK, 0, "abbrev", &abbrev, N_("n"), N_("use <n> digits to display SHA-1s"),
+			PARSE_OPT_OPTARG, &parse_opt_abbrev_for_describe_cb, 0},
 		OPT_END(),
 	};
 
@@ -425,8 +441,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 
 	save_commit_buffer = 0;
 
-	if (longformat && abbrev == 0)
-		die(_("--long is incompatible with --abbrev=0"));
+	if (longformat && (abbrev == 0 || simple_abbrev))
+		die(_("--long is incompatible with --abbrev=+ or --abbrev=0"));
 
 	if (contains) {
 		struct argv_array args;
-- 
1.9.3

Re: [PATCH 1/2] describe: support the syntax "--abbrev=+"

[Jeff King]

On Fri, Sep 12, 2014 at 11:26:43AM -0300, Jonh Wendell wrote:

> It will print just a "+" sign appended to the found tag, if there
> are commits between the tag and the supplied commit.
> 
> It's useful when you just need a simple output to know if the
> supplied commit is an exact match or not.

Seems like a reasonable extension of the "--abbrev=0" behavior.

>  builtin/describe.c | 26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)

You can probably just squash the related documentation in with this
patch. Also, maybe some tests in t6120? It doesn't look like we test
--abbrev=0, either; if you are feeling especially charitable, it might
be good to add some tests for it, too.

> @@ -378,8 +379,12 @@ static void describe(const char *arg, int last_one)
>  	}
>  
>  	display_name(all_matches[0].name);
> -	if (abbrev)
> -		show_suffix(all_matches[0].depth, cmit->object.sha1);
> +	if (abbrev) {
> +		if (simple_abbrev)
> +			printf("+");
> +		else
> +			show_suffix(all_matches[0].depth, cmit->object.sha1);
> +	}

This covers the case when we do have a commit to show. The exact-match
case is handled elsewhere, and I wondered what would happen if you
passed "--long", but:

> +	if (longformat && (abbrev == 0 || simple_abbrev))
> +		die(_("--long is incompatible with --abbrev=+ or --abbrev=0"));

You cover that here. Good.

> +static int parse_opt_abbrev_for_describe_cb(const struct option *opt, const char *arg, int unset)
> +{
> +	if (arg && !strncmp(arg, "+", 1)) {

Why strncmp here? If I pass "--abbrev=+10", shouldn't that be an error?

> +		simple_abbrev = 1;
> +		return 0;
> +	}
> +
> +	return parse_opt_abbrev_cb(opt, arg, unset);
> +}

What happens if you pass the option multiple times? I'd expect later
ones to override earlier ones. For "--abbrev=0 --abbrev=10" this just
works, because they both store the value in the abbrev variable. But you
store simple_abbrev as a separate variable.

What do these do?

  1. --abbrev=10 --abbrev=+

  2. --abbrev=+ --abbrev=10

  3. --abbrev=0 --abbrev=+

The first one will respect simple_abbrev, since it avoids calling
show_suffix at all. Good. The second one will do the same. We probably
need to reset simple_abbrev to 0 whenever we see another --abbrev. The
third one will not respect simple_abbrev, because we never enter the "if
(abbrev)" conditional. We probably need to reset "abbrev" to something
non-zero when we set simple_abbrev.

I.e.:

diff --git a/builtin/describe.c b/builtin/describe.c
index 3a5c052..532161e 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -397,9 +397,11 @@ static int parse_opt_abbrev_for_describe_cb(const struct option *opt, const char
 {
 	if (arg && !strncmp(arg, "+", 1)) {
 		simple_abbrev = 1;
+		abbrev = 1; /* doesn't matter as long as it is non-zero */
 		return 0;
 	}
 
+	simple_abbrev = 0;
 	return parse_opt_abbrev_cb(opt, arg, unset);
 }
 

Another alternative would be to stuff the simple_abbrev flag into
an unused value of "abbrev" (say, -2), but that is probably a little
less obvious than just resetting them together as above.

-Peff

Re: [PATCH 1/2] describe: support the syntax "--abbrev=+"

[Eric Sunshine]

On Sun, Sep 14, 2014 at 4:18 AM, Jeff King <peff@peff.net> wrote:
> On Fri, Sep 12, 2014 at 11:26:43AM -0300, Jonh Wendell wrote:
>
>> It will print just a "+" sign appended to the found tag, if there
>> are commits between the tag and the supplied commit.
>>
>> It's useful when you just need a simple output to know if the
>> supplied commit is an exact match or not.
>
> Seems like a reasonable extension of the "--abbrev=0" behavior.

My reaction is opposite: Such overloading of --abbrev= feels abusive,
non-obvious, and is inconsistent with --abbrev accepted by other
commands.

It's also potentially ambiguous. If the tag name ends with a '+' and
there are no commits atop it, then the client can be fooled into
thinking there are. Being able to configure the suffix, rather than
hardcoding '+', might help, but seems ugly.

The justification in the cover letter is that it provides a way to
check if there are commits atop the latest tag. Within a script, it
might be sufficient merely to compare the output of 'git describe' and
'git describe --abbrev=0'. If they differ, then there are commits atop
the latest tag.

Thus, this feature seems somewhat misguided, but perhaps I'm missing
something obvious.

>>  builtin/describe.c | 26 +++++++++++++++++++++-----
>>  1 file changed, 21 insertions(+), 5 deletions(-)
>
> You can probably just squash the related documentation in with this
> patch. Also, maybe some tests in t6120? It doesn't look like we test
> --abbrev=0, either; if you are feeling especially charitable, it might
> be good to add some tests for it, too.
>
>> @@ -378,8 +379,12 @@ static void describe(const char *arg, int last_one)
>>       }
>>
>>       display_name(all_matches[0].name);
>> -     if (abbrev)
>> -             show_suffix(all_matches[0].depth, cmit->object.sha1);
>> +     if (abbrev) {
>> +             if (simple_abbrev)
>> +                     printf("+");
>> +             else
>> +                     show_suffix(all_matches[0].depth, cmit->object.sha1);
>> +     }
>
> This covers the case when we do have a commit to show. The exact-match
> case is handled elsewhere, and I wondered what would happen if you
> passed "--long", but:
>
>> +     if (longformat && (abbrev == 0 || simple_abbrev))
>> +             die(_("--long is incompatible with --abbrev=+ or --abbrev=0"));
>
> You cover that here. Good.
>
>> +static int parse_opt_abbrev_for_describe_cb(const struct option *opt, const char *arg, int unset)
>> +{
>> +     if (arg && !strncmp(arg, "+", 1)) {
>
> Why strncmp here? If I pass "--abbrev=+10", shouldn't that be an error?
>
>> +             simple_abbrev = 1;
>> +             return 0;
>> +     }
>> +
>> +     return parse_opt_abbrev_cb(opt, arg, unset);
>> +}
>
> What happens if you pass the option multiple times? I'd expect later
> ones to override earlier ones. For "--abbrev=0 --abbrev=10" this just
> works, because they both store the value in the abbrev variable. But you
> store simple_abbrev as a separate variable.
>
> What do these do?
>
>   1. --abbrev=10 --abbrev=+
>
>   2. --abbrev=+ --abbrev=10
>
>   3. --abbrev=0 --abbrev=+
>
> The first one will respect simple_abbrev, since it avoids calling
> show_suffix at all. Good. The second one will do the same. We probably
> need to reset simple_abbrev to 0 whenever we see another --abbrev. The
> third one will not respect simple_abbrev, because we never enter the "if
> (abbrev)" conditional. We probably need to reset "abbrev" to something
> non-zero when we set simple_abbrev.
>
> I.e.:
>
> diff --git a/builtin/describe.c b/builtin/describe.c
> index 3a5c052..532161e 100644
> --- a/builtin/describe.c
> +++ b/builtin/describe.c
> @@ -397,9 +397,11 @@ static int parse_opt_abbrev_for_describe_cb(const struct option *opt, const char
>  {
>         if (arg && !strncmp(arg, "+", 1)) {
>                 simple_abbrev = 1;
> +               abbrev = 1; /* doesn't matter as long as it is non-zero */
>                 return 0;
>         }
>
> +       simple_abbrev = 0;
>         return parse_opt_abbrev_cb(opt, arg, unset);
>  }
>
>
> Another alternative would be to stuff the simple_abbrev flag into
> an unused value of "abbrev" (say, -2), but that is probably a little
> less obvious than just resetting them together as above.
>
> -Peff

[PATCH 2/2] describe: Add documentation for "--abbrev=+"

[Jonh Wendell]

Signed-off-by: Jonh Wendell <jonh.wendell@gmail.com>
---
 Documentation/git-describe.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
index d20ca40..e291770 100644
--- a/Documentation/git-describe.txt
+++ b/Documentation/git-describe.txt
@@ -54,6 +54,12 @@ OPTIONS
 	abbreviated object name, use <n> digits, or as many digits
 	as needed to form a unique object name.  An <n> of 0
 	will suppress long format, only showing the closest tag.
+	+
+	+
+	A special case of <n> equals to "\+" (without quotes) will print
+	just a "+" sign instead of the whole suffix. This is useful if you
+	only need to know if the supplied <commit-ish> points to an exact
+	match or if there are commits between the tag found and the <commit-ish>.
 
 --candidates=<n>::
 	Instead of considering only the 10 most recent tags as
-- 
1.9.3

Re: [PATCH 2/2] describe: Add documentation for "--abbrev=+"

[Jeff King]

On Fri, Sep 12, 2014 at 11:26:44AM -0300, Jonh Wendell wrote:

> --- a/Documentation/git-describe.txt
> +++ b/Documentation/git-describe.txt
> @@ -54,6 +54,12 @@ OPTIONS
>  	abbreviated object name, use <n> digits, or as many digits
>  	as needed to form a unique object name.  An <n> of 0
>  	will suppress long format, only showing the closest tag.
> +	+
> +	+

Why two continuation lines here? One seems to be enough on my version of
asciidoc (though it would not be the first time we have encountered
weird behaviors in our documentation toolchain).

> +	A special case of <n> equals to "\+" (without quotes) will print
> +	just a "+" sign instead of the whole suffix. This is useful if you
> +	only need to know if the supplied <commit-ish> points to an exact
> +	match or if there are commits between the tag found and the <commit-ish>.

I found this first phrase a little hard to parse. What about just:

  An <n> of `+` will print...

That uses the same wording as above, and I think using backticks is
probably the right thing. It will appear in a monospaced font, which
avoids the need to explain the quotes (and you also do not need to
backslash-escape inside it).

-Peff

[PATCH 0/2] describe: support the syntax "--abbrev=+"

[Jonh Wendell]

Sometimes it's interesting to have a simple output that answers the question:
Are there commits after the latest tag?

One possible solution is to just print a "+" (plus) signal after the tag. Example:

> git describe --abbrev=1 5261ec5d5
v2.1.0-rc1-2-g5261ec

> git describe --abbrev=+ 5261ec5d5
v2.1.0-rc1+


Jonh Wendell (2):
  describe: support the syntax "--abbrev=+"
  describe: Add documentation for "--abbrev=+"

 Documentation/git-describe.txt |  6 ++++++
 builtin/describe.c             | 26 +++++++++++++++++++++-----
 2 files changed, 27 insertions(+), 5 deletions(-)

-- 
1.9.3

[PATCH 1/2] describe: support the syntax "--abbrev=+"

[Jonh Wendell]

It will print just a "+" sign appended to the found tag, if there
are commits between the tag and the supplied commit.

It's useful when you just need a simple output to know if the
supplied commit is an exact match or not.
---
 builtin/describe.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/builtin/describe.c b/builtin/describe.c
index ee6a3b9..3a5c052 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -30,6 +30,7 @@ static int have_util;
 static const char *pattern;
 static int always;
 static const char *dirty;
+static int simple_abbrev = 0;
 
 /* diff-index command arguments to check if working tree is dirty. */
 static const char *diff_index_args[] = {
@@ -378,8 +379,12 @@ static void describe(const char *arg, int last_one)
 	}
 
 	display_name(all_matches[0].name);
-	if (abbrev)
-		show_suffix(all_matches[0].depth, cmit->object.sha1);
+	if (abbrev) {
+		if (simple_abbrev)
+			printf("+");
+		else
+			show_suffix(all_matches[0].depth, cmit->object.sha1);
+	}
 	if (dirty)
 		printf("%s", dirty);
 	printf("\n");
@@ -388,6 +393,16 @@ static void describe(const char *arg, int last_one)
 		clear_commit_marks(cmit, -1);
 }
 
+static int parse_opt_abbrev_for_describe_cb(const struct option *opt, const char *arg, int unset)
+{
+	if (arg && !strncmp(arg, "+", 1)) {
+		simple_abbrev = 1;
+		return 0;
+	}
+
+	return parse_opt_abbrev_cb(opt, arg, unset);
+}
+
 int cmd_describe(int argc, const char **argv, const char *prefix)
 {
 	int contains = 0;
@@ -398,7 +413,6 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "tags",       &tags, N_("use any tag, even unannotated")),
 		OPT_BOOL(0, "long",       &longformat, N_("always use long format")),
 		OPT_BOOL(0, "first-parent", &first_parent, N_("only follow first parent")),
-		OPT__ABBREV(&abbrev),
 		OPT_SET_INT(0, "exact-match", &max_candidates,
 			    N_("only output exact matches"), 0),
 		OPT_INTEGER(0, "candidates", &max_candidates,
@@ -410,6 +424,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 		{OPTION_STRING, 0, "dirty",  &dirty, N_("mark"),
 			N_("append <mark> on dirty working tree (default: \"-dirty\")"),
 			PARSE_OPT_OPTARG, NULL, (intptr_t) "-dirty"},
+		{OPTION_CALLBACK, 0, "abbrev", &abbrev, N_("n"), N_("use <n> digits to display SHA-1s"),
+			PARSE_OPT_OPTARG, &parse_opt_abbrev_for_describe_cb, 0},
 		OPT_END(),
 	};
 
@@ -425,8 +441,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 
 	save_commit_buffer = 0;
 
-	if (longformat && abbrev == 0)
-		die(_("--long is incompatible with --abbrev=0"));
+	if (longformat && (abbrev == 0 || simple_abbrev))
+		die(_("--long is incompatible with --abbrev=+ or --abbrev=0"));
 
 	if (contains) {
 		struct argv_array args;
-- 
1.9.3