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