Re: [PATCH v4 0/9] packfile: avoid using the 'the_repository' global variable

From: Jeff King <peff@peff.net>
Date: Nov 1, 2024, 17:29 UTC
Message-ID: <20241101172939.GA2337295@coredump.intra.peff.net>
In-Reply-To: <CAOLa=ZTp=QqD0P2zhGunrOfmAebwvLJ20bQq9Tog3eLaO9pwoA@mail.gmail.com>

On Fri, Nov 01, 2024 at 11:07:48AM -0500, karthik nayak wrote:

Show 17 quoted lines

> 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.

  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. ;)

-Peff