Subject: Re: [PATCH v2 11/18] maintenance: auto-size incremental-repack batch

From: Emily Shaffer <[email-masked]>
Date: Wed, 29 Jul 2020 15:23:34 -0700

On Thu, Jul 23, 2020 at 05:56:33PM +0000, Derrick Stolee via GitGitGadget wrote:
> diff --git a/builtin/gc.c b/builtin/gc.c
> index eb4b01c104..889d97afe7 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -1021,19 +1021,65 @@ static int multi_pack_index_expire(void)
>  	return result;
>  }
>  
> +#define TWO_GIGABYTES (2147483647)

[jonathan tan] This would be easier to understand if it was expressed
with bitshift, e.g. 1 << 31

> +#define UNSET_BATCH_SIZE ((unsigned long)-1)
[jonathan tan] This looks like it's never used... and vulnerable to
cross-platform size changes because it's referring to an implicitly
sized int, and could behave differently if it was put into a larger
size, e.g. you wouldn't get 0xFFFF.. if you assigned this into a long
long.

> +	for (p = get_all_packs(r); p; p = p->next) {
> +		if (p->pack_size > max_size) {
> +			second_largest_size = max_size;
> +			max_size = p->pack_size;
> +		} else if (p->pack_size > second_largest_size)
> +			second_largest_size = p->pack_size;
> +	}
> +
> +	result_size = second_largest_size + 1;
[jonathan tan] What happens when there's only one packfile, and when
there are two packfiles? Can we write tests to illustrate the behavior?
The edge case here (result_size=1) is hard to understand by reading the
code.

> +
> +	/* But limit ourselves to a batch size of 2g */
[emily shaffer] nit: can you please capitalize G, GB, whatever :)