Re: [PATCH 2/7] builtin/gc: refactor to read config into structure

From: Derrick Stolee <stolee@gmail.com>
Date: Aug 15, 2024, 13:46 UTC
Message-ID: <6304df4c-705a-42f7-9e43-1d89404731c6@gmail.com>
In-Reply-To: <ff6aa9d7ba0bd67d3682ecb44c024a6d59a7decd.1723533091.git.ps@pks.im>

On 8/13/24 3:17 AM, Patrick Steinhardt wrote:

Show 9 quoted lines

> The git-gc(1) command knows to read a bunch of config keys to tweak its
> own behaviour. The values are parsed into global variables, which makes
> it hard to correctly manage the lifecycle of values that may require a
> memory allocation.
> 
> Refactor the code to use a `struct gc_config` that gets populated and
> passed around. For one, this makes previously-implicit dependencies on
> these config values clear. Second, it will allow us to properly manage
> the lifecycle in the next commit.

I think this is a valuable goal.

> -static const char *gc_log_expire = "1.day.ago";
> -static const char *prune_expire = "2.weeks.ago";
> -static const char *prune_worktrees_expire = "3.months.ago";

I was going to mention this in the previous patch where you change how these variables are cast into git_config_get_expiry(). They aren't changing to non-const here, either.

> +struct gc_config {

...

> +	const char *gc_log_expire;
> +	const char *prune_expire;
> +	const char *prune_worktrees_expire;

The fact that they are initialized to const strings makes it difficult to know if they've been updated. I wonder if we need to change them to have a "if NULL, then use a const default" somewhere. (And maybe you do this later in the series).

> +static void gc_config(struct gc_config *cfg)

I appreciate that you are taking the step to make the structure a process parameter and not just another global.

Thanks, -Stolee