From: Derrick Stolee Date: Fri, 24 Jan 2025 17:29:03 GMT Subject: Re: [PATCH v3 2/8] pack-objects: add --name-hash-version option Message-ID: In-Reply-To: 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 >> +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