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

[Tian Yuchen <cat@malon.dev>]

On 7/12/26 22:12, Pablo Sabater wrote:
> On Sun Jul 12, 2026 at 1:17 PM CEST, Tian Yuchen wrote:
>> 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()'.
>>
>> Similar to the recent editor_program migration, no standalone getter
>> is introduced to keep the code minimal. The dynamically allocated
>> memory is now managed by 'repo_config_values_clear()'.
>>
>> On top of that, fix a memory leak in pager.c while we are at it.
>>
>> Mentored-by: Christian Couder <christian.couder@gmail.com>
>> Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com>
>> Mentored-by: Olamide Caleb Bello <belkid98@gmail.com>
>> Signed-off-by: Tian Yuchen <cat@malon.dev>
>> ---
>>   environment.c |  2 ++
>>   environment.h |  1 +
>>   pager.c       | 26 +++++++++++++++++---------
>>   3 files changed, 20 insertions(+), 9 deletions(-)
>>
>> diff --git a/environment.c b/environment.c
>> index a65d575af4..975c9cb9eb 100644
>> --- a/environment.c
>> +++ b/environment.c
>> @@ -725,6 +725,7 @@ void repo_config_values_init(struct repo_config_values *cfg)
>>   	cfg->attributes_file = NULL;
>>   	cfg->excludes_file = NULL;
>>   	cfg->editor_program = NULL;
>> +	cfg->pager_program = NULL;
>>   	cfg->apply_sparse_checkout = 0;
>>   	cfg->branch_track = BRANCH_TRACK_REMOTE;
>>   	cfg->trust_ctime = 1;
>> @@ -742,4 +743,5 @@ void repo_config_values_clear(struct repo_config_values *cfg)
>>   	FREE_AND_NULL(cfg->attributes_file);
>>   	FREE_AND_NULL(cfg->excludes_file);
>>   	FREE_AND_NULL(cfg->editor_program);
>> +	FREE_AND_NULL(cfg->pager_program);
>>   }
>> diff --git a/environment.h b/environment.h
>> index 8178ebab76..39b6691b47 100644
>> --- a/environment.h
>> +++ b/environment.h
>> @@ -92,6 +92,7 @@ struct repo_config_values {
>>   	char *attributes_file;
>>   	char *excludes_file;
>>   	char *editor_program;
>> +	char *pager_program;
>>   	int apply_sparse_checkout;
>>   	int trust_ctime;
>>   	int check_stat;
>> diff --git a/pager.c b/pager.c
>> index 35b210e048..bc55546670 100644
>> --- a/pager.c
>> +++ b/pager.c
>> @@ -5,6 +5,8 @@
>>   #include "run-command.h"
>>   #include "sigchain.h"
>>   #include "alias.h"
>> +#include "repository.h"
>> +#include "environment.h"
>>
>>   int pager_use_color = 1;
>>
>> @@ -13,7 +15,6 @@ int pager_use_color = 1;
>>   #endif
>>
>>   static struct child_process pager_process;
>> -static char *pager_program;
>>   static int old_fd1 = -1, old_fd2 = -1;
>>
>>   /* Is the value coming back from term_columns() just a guess? */
>> @@ -75,10 +76,15 @@ static void wait_for_pager_signal(int signo)
>>
>>   static int core_pager_config(const char *var, const char *value,
>>   			     const struct config_context *ctx UNUSED,
>> -			     void *data UNUSED)
>> +			     void *data)
> 
> Could this change behaviour that a caller expects?
> 
> (looking at the hunk below) we are now using repo_config_values() which
> contains the condition 'repo != the_repository'. This means that if there
> is a caller that sends anything but the_repository, it will BUG() out.
> 
> Before this patch it would have worked, it worked because callers were
> sending the correct repository. Now we enforce it.
> 
> If we check the callers we can see that everyone sends the_repository,
> so this new assert is fine and prevents sending submodules by mistake.
> 
> Makes sense.
> 

;)


> I think it's worth mentioning that on the commit body/function.
> 

Okay, will add a line to briefly explain this. Something like what Junio 
said:

	All current callers of git_pager() and
	check_pager_config() indeed pass 'the_repository', so this new
	enforcement does not harm them.

>>   {
>> -	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);
>> +	}
> 
> Ok. Now that pager_program is not file-scoped we drop the UNUSED and
> pager_program now lives in the per-repo field.
> Then we change the address where ->pager_program (which we access through
> repo_config_values()) points to.
> 
> FREE_AND_NULL() is new, before this patch it must have been leaking,
> good job.
> 
> Similar to previous patches, let's change the pattern to only call
> repo_config_values() once and use the pointer it returns.
> 
>> +
>>   	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 as above, let's call repo_config_values() once.
> 
>>   	}
>>   	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 pattern. This also frees, but the log says "a" memory leak is fixed
> in this patch, should we change it to two?

Nice catch.

> 
>> +	}
>>   	return data.want;
>>   }
> 
> I peeked at later patches of this series and the multiple calls of
> repo_config_values() pattern keeps appearing.
> I haven't finished reviewing 5-9. If you reroll before I get to those
> patches, it may be worth fixing this pattern across the whole series.
> 
> Regards,
> Pablo

For those cases where multiple calls to repo_config_values() appear in a 
single function body, I will fix such pattern by using cfg.

Thanks, yuchen