Re: [PATCH v10 4/9] environment: move pager_program into repo_config_values

[Junio C Hamano <gitster@pobox.com>]

Tian Yuchen <cat@malon.dev> writes:

> The 'pager_program' variable is currently defined as a file-scoped
> static string in pager.c. Move it into 'struct repo_config_values'.
>
> The configuration parsing logic remains strictly within pager.c to
> respect subsystem boundaries. The read/write operations are simply
> redirected to the repository-specific structure using
> 'repo_config_values()'.

By redirecting to repo_config_values(r), we now enforce that the
passed repository must be 'the_repository' (due to the assertion in
repo_config_values()).  All current callers of git_pager() and
check_pager_config() indeed pass 'the_repository', so this new
enforcement does not harm them.  However, it paves the way to later
lift the assertion and allow us to configure different pagers for
different repositories, which is a welcome improvement.

>  static int core_pager_config(const char *var, const char *value,
>  			     const struct config_context *ctx UNUSED,
> -			     void *data UNUSED)
> +			     void *data)
>  {
> -	if (!strcmp(var, "core.pager"))
> -		return git_config_string(&pager_program, var, value);
> +	struct repository *r = data;
> +
> +	if (!strcmp(var, "core.pager")) {
> +		FREE_AND_NULL(repo_config_values(r)->pager_program);
> +		return git_config_string(&repo_config_values(r)->pager_program, var, value);
> +	}

It may be just me, but I would have preferred to see this written
more like


	if (!strcmp(var, "core.pager")) {
		struct repo_config_values *values = repo_config_values(r);

		FREE_AND_NULL(values->pager_program);
		return git_config_string(&values->pager_program, var, value);
	}

which will make it easier to see that we are freeing the same thing
immediately before we overwrite it.  It also shortens the lines.
For a temporary variable with a very short scope like this one that
is introduced solely for readability, it is OK to use even shorter
name like 'v' if you want to ('r' certainly has a much longer
lifespan that it, and I would probably have preferred to see it
called 'repo').

    Side note: we might want to give a hint in the coding guidelines
    document that a variable with larger lifespan should get longer
    names, or something.

>  	return 0;
>  }
>  
> @@ -91,10 +97,10 @@ const char *git_pager(struct repository *r, int stdout_is_tty)
>  
>  	pager = getenv("GIT_PAGER");
>  	if (!pager) {
> -		if (!pager_program)
> +		if (!repo_config_values(r)->pager_program)
>  			read_early_config(r,
> -					  core_pager_config, NULL);
> -		pager = pager_program;
> +					  core_pager_config, r);
> +		pager = repo_config_values(r)->pager_program;
>  	}

Same here.

>  	if (!pager)
>  		pager = getenv("PAGER");
> @@ -302,7 +308,9 @@ int check_pager_config(struct repository *r, const char *cmd)
>  
>  	read_early_config(r, pager_command_config, &data);
>  
> -	if (data.value)
> -		pager_program = data.value;
> +	if (data.value) {
> +		free(repo_config_values(r)->pager_program);
> +		repo_config_values(r)->pager_program = data.value;
> +	}

Same here.