[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+


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

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

[Jonh Wendell]

---
 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=+"

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 924 bytes --]

On Sat, Aug 23, 2014 at 02:13:22PM -0300, Jonh Wendell wrote:
> ---
>  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.
> +	+
> +	+

Did you intend to have two lines with just plus signs here?  I'm not
aware of anywhere else in the Documentation where we do that.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Jonh Wendell]

2014-08-24 3:35 GMT-03:00 brian m. carlson <sandals@crustytoothpaste.net>:
> On Sat, Aug 23, 2014 at 02:13:22PM -0300, Jonh Wendell wrote:
>> ---
>>  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.
>> +     +
>> +     +
>
> Did you intend to have two lines with just plus signs here?  I'm not
> aware of anywhere else in the Documentation where we do that.
> --

In my tests just one line was not enough to produce a proper line
break in the html output.
With two lines like above the output in man and html are ok (just 1 line break).

-- 
Jonh Wendell
http://www.bani.com.br

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

[Jonh Wendell]

hi there!
just a ping here, these are my first patches to git.
any comment, feedback?

2014-08-23 14:13 GMT-03:00 Jonh Wendell <jonh.wendell@gmail.com>:
> 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
>



-- 
Jonh Wendell
http://www.bani.com.br

[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