Re: [PATCH 1/7] pack-objects: add --full-name-hash option

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 21, 2024, 23:32 UTC
Message-ID: <xmqqmshsduyh.fsf@gitster.g>
In-Reply-To: <Zz+nk4w+y63vCupK@nand.local>

Taylor Blau <me@ttaylorr.com> writes:

Show 20 quoted lines

> On Thu, Nov 21, 2024 at 03:08:09PM -0500, Taylor Blau wrote:
>> The remaining parts of this change look good to me.
>
> Oops, one thing I forgot (which reading Peff's message in [1] reminded
> me of) is that I think we need to disable full-name hashing when we're
> reusing existing packfiles as is the case with try_partial_reuse().
>
> There we're always looking at classic name hash values, so mixing the
> two would be a mistake. I think that amounts to:
>
> --- 8< ---
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 762949e4c8..7e370bcfc9 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -4070,6 +4070,8 @@ static int get_object_list_from_bitmap(struct rev_info *revs)
>  	if (!(bitmap_git = prepare_bitmap_walk(revs, 0)))
>  		return -1;
>
> +	use_full_name_hash = 0;

Hmph, is this early enough, or has some other code path already computed the name hashes for the paths for the files to be packed?

    ... Goes and looks ...

This is called from get_object_list() which

 - is not called under --stdin-packs,
 - is not called in cruft mode,
 - is not called when reading object list from --stdin

so we are looking at the bog-standard "objects to be packed are given in the form of rev-list command line options from our command line". And in the function, we walk the history near the end, which makes show_object calls that adds object-entry with the name-hash. So the call to get_object_list_from_bitmap() happens way before the first use of the name-hash function, so this is probably safe.

And obviously get_object_list_from_bitmap() is the only place we select objects to be packed from an existing pack and a bitmap file, so even if we gain new callers in the future, it is very likely that the new callers would benefit from this change.

OK.  Nicely done.

Show 9 quoted lines

>  	if (pack_options_allow_reuse())
>  		reuse_partial_packfile_from_bitmap(bitmap_git,
>  						   &reuse_packfiles,
> --- >8 ---
>
> Thanks,
> Taylor
>
> [1]: https://lore.kernel.org/git/20241104172533.GA2985568@coredump.intra.peff.net/