Re: [PATCH] diff: handle --no-abbrev outside of repository

[Jeff King <peff@peff.net>]

On Mon, Nov 28, 2016 at 11:25:08AM -0700, Jack Bates wrote:

> diff --git a/diff.c b/diff.c
> index ec87283..0447eff 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -3106,7 +3106,8 @@ static const char *diff_abbrev_oid(const struct object_id *oid, int abbrev)
>  			abbrev = FALLBACK_DEFAULT_ABBREV;
>  		if (abbrev > GIT_SHA1_HEXSZ)
>  			die("BUG: oid abbreviation out of range: %d", abbrev);
> -		hex[abbrev] = '\0';
> +		if (abbrev)
> +			hex[abbrev] = '\0';
>  		return hex;
>  	}

This hunk made me wonder if there is a regression in v2.11, as this
fallback code is new in that version. But I don't think so.

This new code doesn't handle abbrev==0 the same as find_unique_abbrev()
does, and that's clearly a bug. But I couldn't find any way to trigger
it with the existing code.

The obvious way is with --no-abbrev, but that doesn't work without yet
without your patch.

A less obvious way is --abbrev=0, but that gets munged internally to
MINIMUM_ABBREV.

The most obscure is "git -c core.abbrev=0", but that barfs completely on
a too-small abbreviation (without even giving a good error message,
which is something we might want to fix).

So I think there is no regression, and we only have to worry about it as
part of the feature you are adding here (it might be worth calling out
the bug fix specifically in the commit message, though, or even putting
it in its own patch).

-Peff