From: Taylor Blau <me@ttaylorr.com>
Date: Fri, 17 Oct 2025 20:58:43 GMT
Subject: Re: [PATCH 1/8] builtin/gc: remove global `repack` variable
Message-ID: <aPKuA37Y0Yzl4bpn@nand.local>
In-Reply-To: <20251016-pks-maintenance-geometric-strategy-v1-1-18943d474203@pks.im>

On Thu, Oct 16, 2025 at 09:26:32AM +0200, Patrick Steinhardt wrote:
> @@ -1269,6 +1272,19 @@ static int maintenance_task_gc_background(struct maintenance_run_opts *opts,
>  	return run_command(&child);
>  }
>
> +static int gc_condition(struct gc_config *cfg)
> +{
> +	/*
> +	 * Note that it's fine to drop the repack arguments here, as we execute
> +	 * git-gc(1) as a separate child process anyway. So it knows to compute
> +	 * these arguments again.
> +	 */
> +	struct strvec repack_args = STRVEC_INIT;
> +	int ret = need_to_gc(cfg, &repack_args);
> +	strvec_clear(&repack_args);
> +	return ret;
> +}
> +

Thanks for calling this one out in the patch message. I think had I not
read that I would have been confused why we were putting contents into
the strvec here just to throw it away, but the explanation you wrote
above makes it clear :-).

The rest of the patch looks great to me.

Thanks,
Taylor