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

From: Emily Shaffer <emilyshaffer@google.com>
Date: Jul 29, 2020, 22:23 UTC
Message-ID: <20200729222334.GE2965447@google.com>
In-Reply-To: <478c7f1d0b858755c2c4b98605405214910b6f4c.1595527000.git.gitgitgadget@gmail.com>

On Thu, Jul 23, 2020 at 05:56:33PM +0000, Derrick Stolee via GitGitGadget wrote:

Show 9 quoted lines

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

Show 9 quoted lines

> +	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 :)