Re: [PATCH v3 2/8] pack-objects: add --name-hash-version option

[Derrick Stolee <stolee@gmail.com>]

On 1/22/25 5:17 PM, Taylor Blau wrote:
> On Fri, Dec 20, 2024 at 05:19:48PM +0000, Derrick Stolee via GitGitGadget wrote:
>> From: Derrick Stolee <stolee@gmail.com>

>> +static inline uint32_t pack_name_hash_fn(const char *name)
>> +{
>> +	switch (name_hash_version)
>> +	{
> 
> This is definitely a nitpick, but the opening curly brace should appear
> on the preceding line. I don't think our CodingGuidelines explicitly say
> this. But in my head the convention is that only function bodies have
> their enclosing braces on their own line, as opposed to:

You're right, I messed up on the switch statement.

>      static inline uint32_t pack_name_hash_fn(const char *name) {

but based on my expectations and your earlier comment I think that this
suggestion is a copy/paste error and you meant to highlight the switch
statement.

>> +	validate_name_hash_version();
>> +	if (write_bitmap_index && name_hash_version != 1) {
>> +		warning(_("currently, --write-bitmap-index requires --name-hash-version=1"));
>> +		name_hash_version = 1;
>> +	}
>> +
> 
> Hmm. validate_name_hash_version() is its own function, which seems good,
> but then we do further validation on it here. I wonder if we should
> either move the latter step into validate_name_hash_version(), which
> would require us to pass a pointer to name_hash_version, or assign it
> the return value of validate_name_hash_version() (assuming it were
> changed to return the appropriate type instead of void).
> 
> I think that we are probably pretty far into bike-shedding territory,
> but figured I'd share as it jumped out to me while reviewing.

Sounds good. I'll try to minimize the uses of name_hash_version outside
of the cluster of methods that implement its details. This may become
more complicated later when the timing of these checks is more interesting.

Thanks,
-Stolee