From: Junio C Hamano <gitster@pobox.com>
Date: Sun, 21 Mar 2010 20:23:38 GMT
Subject: Re: [PATCH v2 3/3] find_unique_abbrev: early out without a memcpy
Message-ID: <7v39ztmnjp.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1269111329-5896-4-git-send-email-kusmabite@gmail.com>

Erik Faye-Lund <kusmabite@googlemail.com> writes:

> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
> ---
>  sha1_name.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/sha1_name.c b/sha1_name.c
> index bf92417..2b1be58 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -196,10 +196,10 @@ const char *find_unique_abbrev(const unsigned char *sha1, int len)
>  	int status, exists;
>  	static char hex[41];
>  
> +	if (len == 40 || !len)
> +		return sha1_to_hex(sha1);
>  	exists = has_sha1_file(sha1);
>  	memcpy(hex, sha1_to_hex(sha1), 40);
> -	if (len == 40 || !len)
> -		return hex;

This is somewhat iffy.  hex[] being static means there can only be one
outstanding return value from f-u-a being used, iow

	printf("%s %s", f-u-a(a, 0), f-u-a(b, 0))

is a no-no.  But at the same time, it means that you can use one more
recycled buffer than sha1_to_hex() gives us, so this may be safe:

	char *ua = f-u-a(a, 0);
        printf("%s %s %s %s %s", ua,
            sha1_to_hex(b), sha1_to_hex(c),  sha1_to_hex(d), sha1_to_hex(e));

but with the above it probably is not anymore, no?

As an optimization patch, I would buy that delaying the "exists" check
until the "no abbreviation" check returned early would make sense, though.