Subject: Re: [PATCH 06/10] get_short_sha1: NUL-terminate hex prefix

From: Junio C Hamano <[email-masked]>
Date: Mon, 26 Sep 2016 10:10:46 -0700

Jeff King <[email-masked]> writes:

> We store the hex prefix in a 40-byte buffer with the prefix
> itself followed by 40-minus-len "x" characters. These x's
> serve no purpose, and the lack of NUL termination makes the
> prefix string annoying to use. Let's just terminate it.

> Note that this is in contrast to the binary prefix, which
> _must_ be zero-padded, because we look at the whole thing
> during a binary search to find the first potential match in
> each pack index. 

Makes sense.

> The loose-object hex search cannot use the
> same trick because it has to do a linear walk through the
> unsorted results of readdir() (and even if it could, you'd
> want zeroes instead of x's).

OK.

>  struct disambiguate_state {
>  	int len; /* length of prefix in hex chars */
> -	char hex_pfx[GIT_SHA1_HEXSZ];
> +	char hex_pfx[GIT_SHA1_HEXSZ + 1];
>  	unsigned char bin_pfx[GIT_SHA1_RAWSZ];
>  
>  	disambiguate_hint_fn fn;
> @@ -291,7 +291,6 @@ static int init_object_disambiguation(const char *name, int len,
>  		return -1;
>  
>  	memset(ds, 0, sizeof(*ds));
> -	memset(ds->hex_pfx, 'x', GIT_SHA1_HEXSZ);

As the whole thing is cleared here...

>  
>  	for (i = 0; i < len ;i++) {
>  		unsigned char c = name[i];
> @@ -313,6 +312,7 @@ static int init_object_disambiguation(const char *name, int len,
>  	}
>  
>  	ds->len = len;
> +	ds->hex_pfx[len] = '\0';

... do we even need this one?  It would not hurt, though.

> @@ -351,7 +351,7 @@ static int get_short_sha1(const char *name, int len, unsigned char *sha1,
>  	status = finish_object_disambiguation(&ds, sha1);
>  
>  	if (!quietly && (status == SHORT_NAME_AMBIGUOUS))
> -		return error("short SHA1 %.*s is ambiguous.", ds.len, ds.hex_pfx);
> +		return error("short SHA1 %s is ambiguous.", ds.hex_pfx);

Makes sense.

Thanks.