From: Justin Tobler <jltobler@gmail.com>
Date: Thu, 16 Oct 2025 20:07:55 GMT
Subject: Re: [PATCH 1/8] builtin/gc: remove global `repack` variable
Message-ID: <uaw3lnxa2yi7mgwmkm2l3lpr7hkfwludzjai53an5nhjh2woif@2kgtzxpnp2ma>
In-Reply-To: <20251016-pks-maintenance-geometric-strategy-v1-1-18943d474203@pks.im>

On 25/10/16 09:26AM, Patrick Steinhardt wrote:
> The global `repack` variable is used to store all command line arguments
> that we eventually want to pass to git-repack(1). It is being appended
> to from multiple different functions, which makes it hard to follow the
> logic. Besides being hard to follow, it also makes it unnecessarily hard
> to reuse this infrastructure in new code.
> 
> Refactor the code so that we store this variable on the stack and pass
> a pointer to it around as needed. This is done so that we can reuse
> `add_repack_all_options()` in a subsequent commit.
> 
> The refactoring itself is straight-forward. One function that deserves
> attention though is `need_to_gc()`: this function determines whether or
> not we need to execute garbage collection for `git gc --auto`, but also
> for `git maintenance run --auto`. But besides figuring out whether we
> have to perform GC, the function also sets up the `repack` arguments.
> 
> For `git gc --auto` it's trivial to adapt, as we already have the
> on-stack variable at our fingertips. But for the maintenance condition
> it's less obvious what to do.
> 
> As it turns out, we can just use another temporary variable there that
> we then immediately discard. If we need to perform GC we execute a child
> git-gc(1) process to repack objects for us, and that process will have
> to recompute the arguments anyway.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
[snip]
> @@ -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.
> +	 */

Also we don't expect any arguments to be configured ahead of time so we
are good.

> +	struct strvec repack_args = STRVEC_INIT;
> +	int ret = need_to_gc(cfg, &repack_args);
> +	strvec_clear(&repack_args);
> +	return ret;
> +}
> +
>  static int prune_packed(struct maintenance_run_opts *opts)
>  {
>  	struct child_process child = CHILD_PROCESS_INIT;
> @@ -1596,7 +1612,7 @@ static const struct maintenance_task tasks[] = {
>  		.name = "gc",
>  		.foreground = maintenance_task_gc_foreground,
>  		.background = maintenance_task_gc_background,
> -		.auto_condition = need_to_gc,
> +		.auto_condition = gc_condition,

Now that the `need_to_gc()` function signature has changed, we use a
wrapper function that provides the repack args. In this case, only the
args that get set during `need_to_gc()` are required, and the args are
not needed afterwards, so it is safe to discard.

This patch looks good.

-Justin