On Sun, Sep 14, 2014 at 4:18 AM, Jeff King <peff@xxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html