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

[Tian Yuchen <cat@malon.dev>]

On 7/12/26 23:36, Junio C Hamano wrote:
> 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.
> 

Exactly.

>>   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);
> 	}
> 

I see. I think it's better to name the struct 'cfg' so that it is 
consistent with what we did before.

> 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.

Thanks, will change all these parts.

Regard, yuchen