Re: [PATCH v2 2/2] repack: accept larger window-memory and max-pack-size

From: Jeff King <peff@peff.net>
Date: Jan 23, 2014, 01:06 UTC
Message-ID: <20140123010642.GB17254@sigill.intra.peff.net>
In-Reply-To: <1390420685-18449-3-git-send-email-gitster@pobox.com>

On Wed, Jan 22, 2014 at 11:58:05AM -0800, Junio C Hamano wrote:

> These quantities can be larger than an int.  Use ulong to express
> them like the underlying pack-objects does, and also parse them with
> the human-friendly unit suffixes.

Hrm. I think that is a valid strategy, but...

> -	int max_pack_size = 0;
> +	unsigned long max_pack_size = 0, window_memory = 0;

Here we must use the correct C type...

> -		OPT_INTEGER(0, "window-memory", &window_memory,
> +		OPT_HUM_ULONG(0, "window-memory", &window_memory,

And here use the correct parser...

>  	if (window_memory)
> -		argv_array_pushf(&cmd_args, "--window-memory=%u", window_memory);
> +		argv_array_pushf(&cmd_args, "--window-memory=%lu", window_memory);

And here use the correct format string...

All of which must match what pack-objects does, or we risk a further break (though I do not guess it will change from ulong anytime soon). The original shell version worked because they were all strings. We do not care about the numeric value here, and are just forwarding the value along to pack-objects. Why not just use a string?

The only advantage I can think of is that this gives us slightly earlier error detection for "git repack --window-memory=bogosity".

But I think there is a subtle problem. Here (and elsewhere) we use the parsed value of "0" as a sentinel. I think that is OK for --max-pack-size, where "0" is not a reasonable value. But git-repack(1) says:

  --window-memory=0 makes memory usage unlimited, which is the default.

What does:

  git config pack.windowMemory 256m
  git repack --window-memory=0

do? It should override the config, but I think it does not with your patch (nor with the current code). Using a string would fix that (though you could also fix it by using a different sentinel, like ULONG_MAX).

>  	if (max_pack_size)
> -		argv_array_pushf(&cmd_args, "--max_pack_size=%u", max_pack_size);
> +		argv_array_pushf(&cmd_args, "--max_pack_size=%lu", max_pack_size);

These underscores are interesting:

  $ git pack-objects --max_pack_size=256m
  error: unknown option `max_pack_size=256m'

I get the feeling the test suite does not cover this feature very well. :)

-Peff