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

On 7/12/26 23:36, Junio C Hamano wrote:

Tian Yuchen [off-list ref] writes:

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()'.

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.

quoted

  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.

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

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

Thanks, will change all these parts.

Regard, yuchen