On 7/12/26 22:12, Pablo Sabater wrote:

On Sun Jul 12, 2026 at 1:17 PM CEST, Tian Yuchen wrote:

quoted

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 [off-list ref]
Mentored-by: Ayush Chandekar [off-list ref]
Mentored-by: Olamide Caleb Bello [off-list ref]
Signed-off-by: Tian Yuchen <redacted>
---
  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.

quoted

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

quoted

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

quoted

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

quoted

+	}
  	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