Re: [PATCH v4 0/9] packfile: avoid using the 'the_repository' global variable
- From
karthik nayak <karthik.188@gmail.com>
- Date
- Nov 4, 2024, 09:39 UTC
- Message-ID
- <CAOLa=ZT8dvpvJ75Oe_PV4_osi7rw3EJnH2SSzUVame-eQvOruA@mail.gmail.com>
- In-Reply-To
- <20241101172939.GA2337295@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 30 quoted lines
> On Fri, Nov 01, 2024 at 11:07:48AM -0500, karthik nayak wrote:
>
>> Thanks for letting me know, I think the fix is simply
>>
>> diff --git a/packfile.c b/packfile.c
>> index f626d38071..737cd60377 100644
>> --- a/packfile.c
>> +++ b/packfile.c
>> @@ -27,8 +27,8 @@
>> #include "pack-objects.h"
>>
>> struct packfile_config {
>> - unsigned long packed_git_window_size;
>> - unsigned long packed_git_limit;
>> + unsigned long long packed_git_window_size;
>> + unsigned long long packed_git_limit;
>> };
>>
>> #define PACKFILE_CONFIG_INIT \
>
> Wait, why did these change from "size_t" to "unsigned long" in your
> series in the first place? If the goal is moving them into a struct,
> they should otherwise retain the same types, no?
>
> Two asides, one for your series and one #leftoverbits:
>
> 1. Since these are now fields of packfile_config, do they need the
> long packed_git_ prefix anymore? Just window_size and limit would
> be nicer, I'd think.
>Moving them to repo_settings means we'd have to keep the long names, otherwise, I agree the shorter names would be better.
Show 6 quoted lines
> 2. I can imagine you might have used "unsigned long" because they are > parsed with git_config_ulong(). That is OK on Linux, where size_t > and "unsigned long" are the same size (either 32- or 64-bits). But > on Windows I think it means that you cannot configure a window > larger than 4GB on a 64-bit system. Or ironically you cannot set a > total limit larger than 4GB, even though the default is 32TB. ;)
Yup that's the reason I changed them. TIL about size_t and how it works. Thanks, I'll change the types accordingly and push a new version soon.
> > -Peff