Re: [PATCH v2 3/6] pack-objects: add GIT_TEST_FULL_NAME_HASH

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 19, 2024, 22:22 UTC
Message-ID: <xmqq34lv5lnn.fsf@gitster.g>
In-Reply-To: <48b3876a10247d2ba65a6e5f1ff8ed3a662813f8.1726692381.git.gitgitgadget@gmail.com>

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 5 quoted lines

> From: Derrick Stolee <stolee@gmail.com>
>
> Add a new environment variable to opt-in to the --full-name-hash option
> in 'git pack-objects'. This allows for extra testing of the feature
> without repeating all of the test scenarios.

This also allows the programmer on the C implementation side to be a bit lazy, as the --full-name-hash option does not have to be plumbed through from the end-user facing commands (like "bundle") down to the underlying "pack-objects" command ;-).

As an end-user facing tweak mechanism, an environment variable is the most clunky, followed by a configuration variable (which can be used via "git -c" and exhibits the same clunkiness as an environment variable), and a command line parameter is the most versatile in allowing users to customize the behaviour per-invocation of the commands. So in the longer term, we probably want to plumb through the option, like you did for "repack -> pack-objects" call chain, for all end-user visible commands that call into pack-objects.

But for testing purposes, the solution presented here is of course good enough.

Show 7 quoted lines

> Second, there are two tests in t5616-partial-clone.sh that I believe are
> actually broken scenarios. While the client is set up to clone the
> 'promisor-server' repo via a treeless partial clone filter (tree:0),
> that filter does not translate to the 'server' repo. Thus, fetching from
> these repos causes the server to think that the client has all reachable
> trees and blobs from the commits advertised as 'haves'. This leads the
> server to providing a thin pack assuming those objects as delta bases.

In short, the tests are based on broken assumption and checking bogus outcome? Somebody familiar with the partial clone area should probably take a look into it and fix the tests if that is the case.

Show 5 quoted lines

> -	if (write_bitmap_index && use_full_name_hash)
> +	if (write_bitmap_index && use_full_name_hash > 0)
>  		die(_("currently, the --full-name-hash option is incompatible with --write-bitmap-index"));
> +	if (use_full_name_hash < 0)
> +		use_full_name_hash = git_env_bool("GIT_TEST_FULL_NAME_HASH", 0);

OK.