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

From: Jeff King <peff@peff.net>
Date: Nov 29, 2016, 07:06 UTC
Message-ID: <20161129070637.eult6o3m34r2mima@sigill.intra.peff.net>
In-Reply-To: <20161128182508.10570-1-jack@nottheoilrig.com>

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

Show 13 quoted lines

> 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