From: Derrick Stolee <stolee@gmail.com>
Date: Tue, 06 May 2025 19:46:43 GMT
Subject: Re: [PATCH v2 08/13] pack-objects: enable --path-walk via config
Message-ID: <ea342443-2c71-476d-ab45-86cbe711fec7@gmail.com>
In-Reply-To: <aBVYa9SzXk7eguIz@nand.local>

On 5/2/25 7:42 PM, Taylor Blau wrote:
> On Mon, Mar 24, 2025 at 03:22:44PM +0000, Derrick Stolee via GitGitGadget wrote:

>> +pack.usePathWalk::
>> +	When true, git will default to using the '--path-walk' option in
>> +	'git pack-objects' when the '--revs' option is present. This
>> +	algorithm groups objects by path to maximize the ability to
>> +	compute delta chains across historical versions of the same
>> +	object. This may disable other options, such as using bitmaps to
>> +	enumerate objects.
>> +
> 
> Same note here as in the previous patch, I think it's fine to refer
> readers to the git-pack-objects[1] documentation instead of repeating
> yourself here. (And it removes the risk of these three descriptions
> falling out of sync with one another).

Sure.

>>   pack.preferBitmapTips::
>>   	When selecting which commits will receive bitmaps, prefer a
>>   	commit at the tip of any reference that is a suffix of any value
>> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
>> index a6b8a78d42a..0ea85754c52 100644
>> --- a/builtin/pack-objects.c
>> +++ b/builtin/pack-objects.c
>> @@ -4652,6 +4652,9 @@ int cmd_pack_objects(int argc,
>>   		if (use_bitmap_index > 0 ||
>>   		    !use_internal_rev_list)
>>   			path_walk = 0;
>> +		else if (the_repository->gitdir &&
>> +			 the_repository->settings.pack_use_path_walk)
>> +			path_walk = 1;
>>   		else
>>   			path_walk = git_env_bool("GIT_TEST_PACK_PATH_WALK", 0);
>>   	}
> 
> The limited diff context makes it hard for me to tell for sure, but this
> takes place after git_config(), right? If so, I think we can avoid using
> the repository settings machinery here and just use the config API
> directly.

As I'm paging this thread back into my memory, I internally thought
"obviously the test demonstrates that it works..."

> (FWIW, I typically think of repository settings as a way to expose
> config information to some part of the codebase that doesn't otherwise
> have easy access to, e.g., a static field that was set by a git_config()
> callback).
> 
> Separate from the above: should this have a sanity test to makes sure
> that we read the config setting correctly?
...but of course I neglected to create such a test. Will fix.

Thanks,
-Stolee