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

From: Jeff King <peff@peff.net>
Date: Sep 14, 2014, 08:18 UTC
Message-ID: <20140914081847.GA20526@peff.net>
In-Reply-To: <1410532004-22769-2-git-send-email-jonh.wendell@gmail.com>

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

Show 5 quoted lines

> 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.

Show 12 quoted lines

> @@ -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?

Show 6 quoted lines

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