Re: [PATCH] Look at both unpacked and packed objects for short name expansion

From: Junio C Hamano <junkio@cox.net>
Date: Sep 16, 2005, 00:28 UTC
Message-ID: <7vfys5votr.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <Pine.LNX.4.63.0509160120470.3859@wgmdd8.biozentrum.uni-wuerzburg.de>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> We used to check for a unique expansion in the unpacked objects, and if
> one was found, we forgot to check in the packed objects, too.

Show 13 quoted lines

> diff --git a/sha1_name.c b/sha1_name.c
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -115,7 +115,8 @@ static int get_short_sha1(const char *na
>  	if (i < 4)
>  		return -1;
>  	if (find_short_object_filename(i, canonical, sha1))
> -		return 0;
> +                /* do not allow non-unique short names */
> +                return find_short_packed_object(i, res, canonical)!=0?-1:0;
>  	if (find_short_packed_object(i, res, sha1))
>  		return 0;
>  	return -1;

Sorry, I do not understand what you are doing here.

We find an unpacked object file in the local object store, and then if we find a unique match (otherwise we try if short-packed-object finds unique match) we try to see if we have the same one in one of the packs we have and refuse? Wouldn't this make things inconvenient for people who run 'git repack' without running 'git prune-packed' afterwards?

And reading find_short_object_filename() I noticed it does not do anything with alternates, and wonder if is this intentional. Who did this part anyway?

Also find_short_packed_object() does not appear to require unique prefix when it does its thing.

I would prefer a proper fix to:

 - find_short_object_filename() to look at alt_odb as well;

 - find_short_packed_object() to make sure it is getting a
   unique match.

 - get_short_sha1() to make sure that the input is overall
   unique.  I think with the current code structure, with or
   without your patch, would do a wrong thing if you pass a
   prefix that matches two unpacked objects and two packed
   ones.

I suspect your patch is an attempt to fix the last one, but I am not sure..