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

[Junio C Hamano <gitster@pobox.com>]

Taylor Blau <me@ttaylorr.com> writes:

> 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.

>  	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/