difftool: eliminate use of global variables

Commit Message

David Aguilar Feb. 4, 2025, 10:55 p.m. UTC

Remove the 'USE_THE_REPOSITORY_VARIABLE' macro now that all state is
passed to each function from callers.

Signed-off-by: David Aguilar <[e-mail]>
---
 builtin/difftool.c | 95 +++++++++++++++++++++++++++-------------------
 1 file changed, 56 insertions(+), 39 deletions(-)

Comments

Patrick Steinhardt Feb. 5, 2025, 7:30 a.m. UTC | #1

On Tue, Feb 04, 2025 at 02:55:00PM -0800, David Aguilar wrote:
> Remove the 'USE_THE_REPOSITORY_VARIABLE' macro now that all state is
> passed to each function from callers.

I think this patch deserves to be split up into multiple patches, as
you're doing multiple things at once:

  - You remove global state located in "builtin/difftool.c", which is
    itself not required to drop `USE_THE_REPOSITORY_VARIABLE`.

  - You introduce `struct difftool_state` to encapsulate state.

  - You replace callsites for interfaces that depend on
    `the_repository`.

All of these changes are great from my point of view, but it's a bit
hard to follow. It doesn't necessarily have to be three commits, but it
would be great to split up the refactorings that introduce the struct
and the refactorings that adapt users of `the_repository`.

> diff --git a/builtin/difftool.c b/builtin/difftool.c
> index 03a8bb92a9..cd1e5882e3 100644
> --- a/builtin/difftool.c
> +++ b/builtin/difftool.c
> @@ -12,8 +12,6 @@
>   * Copyright (C) 2016 Johannes Schindelin
>   */
>  
> -#define USE_THE_REPOSITORY_VARIABLE
> -
>  #include "builtin.h"
>  
>  #include "abspath.h"
> @@ -36,18 +34,27 @@
>  #include "entry.h"
>  #include "setup.h"
>  
> -static int trust_exit_code;
> -
>  static const char *const builtin_difftool_usage[] = {
>  	N_("git difftool [<options>] [<commit> [<commit>]] [--] [<path>...]"),
>  	NULL
>  };
>  
> +struct difftool_state {
> +	int has_symlinks;
> +	int symlinks;
> +	int trust_exit_code;
> +};

Why do we have both `has_symlinks` and `symlinks`? The latter gets set
to `has_symlinks` anyway, so it's a confusing to have both.

Also, I think it would make sense to rename the struct to
`difftool_options`, as it tracks options rather than state.

> @@ -734,8 +750,9 @@ int cmd_difftool(int argc,
>  	};
>  	struct child_process child = CHILD_PROCESS_INIT;
>  
> -	git_config(difftool_config, NULL);
> -	symlinks = has_symlinks;
> +	if (repo)
> +		repo_config(repo, difftool_config, &dt_state);
> +	dt_state.symlinks = dt_state.has_symlinks;
>  
>  	argc = parse_options(argc, argv, prefix, builtin_difftool_options,
>  			     builtin_difftool_usage, PARSE_OPT_KEEP_UNKNOWN_OPT |

Okay, you're being careful in the case where we don't have a repository,
good.

> @@ -749,8 +766,8 @@ int cmd_difftool(int argc,
>  
>  	if (!no_index){
>  		setup_work_tree();
> -		setenv(GIT_DIR_ENVIRONMENT, absolute_path(repo_get_git_dir(the_repository)), 1);
> -		setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(repo_get_work_tree(the_repository)), 1);
> +		setenv(GIT_DIR_ENVIRONMENT, absolute_path(repo_get_git_dir(repo)), 1);
> +		setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(repo_get_work_tree(repo)), 1);

Okay. Here you unconditionall deref the repository, but we would have
exited already if we had no repo with `!!no_index`.

> @@ -799,6 +816,6 @@ int cmd_difftool(int argc,
>  	strvec_pushv(&child.args, argv);
>  
>  	if (dir_diff)
> -		return run_dir_diff(extcmd, symlinks, prefix, &child);
> +		return run_dir_diff(repo, &dt_state, extcmd, prefix, &child);

This one is a bit curious. We never assert that we have a repo when
`dir_diff` is set. But we make `no_index` and `dir_diff` exclusive with
one another, and thus we know that we'll have called `setup_work_tree()`
at an earlier point, which causes us to die in case there is none.

Thanks for working on this!

Patrick

Patch

diff --git a/builtin/difftool.c b/builtin/difftool.c
index 03a8bb92a9..cd1e5882e3 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -12,8 +12,6 @@ 
  * Copyright (C) 2016 Johannes Schindelin
  */
 
-#define USE_THE_REPOSITORY_VARIABLE
-
 #include "builtin.h"
 
 #include "abspath.h"
@@ -36,18 +34,27 @@ 
 #include "entry.h"
 #include "setup.h"
 
-static int trust_exit_code;
-
 static const char *const builtin_difftool_usage[] = {
 	N_("git difftool [<options>] [<commit> [<commit>]] [--] [<path>...]"),
 	NULL
 };
 
+struct difftool_state {
+	int has_symlinks;
+	int symlinks;
+	int trust_exit_code;
+};
+
 static int difftool_config(const char *var, const char *value,
 			   const struct config_context *ctx, void *cb)
 {
+	struct difftool_state *dt_state = (struct difftool_state *)cb;
 	if (!strcmp(var, "difftool.trustexitcode")) {
-		trust_exit_code = git_config_bool(var, value);
+		dt_state->trust_exit_code = git_config_bool(var, value);
+		return 0;
+	}
+	if (!strcmp(var, "core.symlinks")) {
+		dt_state->has_symlinks = git_config_bool(var, value);
 		return 0;
 	}
 
@@ -63,7 +70,8 @@  static int print_tool_help(void)
 	return run_command(&cmd);
 }
 
-static int parse_index_info(char *p, int *mode1, int *mode2,
+static int parse_index_info(struct repository *repo,
+			    char *p, int *mode1, int *mode2,
 			    struct object_id *oid1, struct object_id *oid2,
 			    char *status)
 {
@@ -75,11 +83,11 @@  static int parse_index_info(char *p, int *mode1, int *mode2,
 	*mode2 = (int)strtol(p + 1, &p, 8);
 	if (*p != ' ')
 		return error("expected ' ', got '%c'", *p);
-	if (parse_oid_hex(++p, oid1, (const char **)&p))
+	if (parse_oid_hex_algop(++p, oid1, (const char **)&p, repo->hash_algo))
 		return error("expected object ID, got '%s'", p);
 	if (*p != ' ')
 		return error("expected ' ', got '%c'", *p);
-	if (parse_oid_hex(++p, oid2, (const char **)&p))
+	if (parse_oid_hex_algop(++p, oid2, (const char **)&p, repo->hash_algo))
 		return error("expected object ID, got '%s'", p);
 	if (*p != ' ')
 		return error("expected ' ', got '%c'", *p);
@@ -106,7 +114,8 @@  static void add_path(struct strbuf *buf, size_t base_len, const char *path)
 /*
  * Determine whether we can simply reuse the file in the worktree.
  */
-static int use_wt_file(const char *workdir, const char *name,
+static int use_wt_file(struct repository *repo,
+		       const char *workdir, const char *name,
 		       struct object_id *oid)
 {
 	struct strbuf buf = STRBUF_INIT;
@@ -121,7 +130,7 @@  static int use_wt_file(const char *workdir, const char *name,
 		int fd = open(buf.buf, O_RDONLY);
 
 		if (fd >= 0 &&
-		    !index_fd(the_repository->index, &wt_oid, fd, &st, OBJ_BLOB, name, 0)) {
+		    !index_fd(repo->index, &wt_oid, fd, &st, OBJ_BLOB, name, 0)) {
 			if (is_null_oid(oid)) {
 				oidcpy(oid, &wt_oid);
 				use = 1;
@@ -212,13 +221,14 @@  static int path_entry_cmp(const void *cmp_data UNUSED,
 	return strcmp(a->path, key ? key : b->path);
 }
 
-static void changed_files(struct hashmap *result, const char *index_path,
+static void changed_files(struct repository *repo,
+			  struct hashmap *result, const char *index_path,
 			  const char *workdir)
 {
 	struct child_process update_index = CHILD_PROCESS_INIT;
 	struct child_process diff_files = CHILD_PROCESS_INIT;
 	struct strbuf buf = STRBUF_INIT;
-	const char *git_dir = absolute_path(repo_get_git_dir(the_repository));
+	const char *git_dir = absolute_path(repo_get_git_dir(repo));
 	FILE *fp;
 
 	strvec_pushl(&update_index.args,
@@ -291,13 +301,15 @@  static int ensure_leading_directories(char *path)
  * to compare the readlink(2) result as text, even on a filesystem that is
  * capable of doing a symbolic link.
  */
-static char *get_symlink(const struct object_id *oid, const char *path)
+static char *get_symlink(struct repository *repo,
+			 struct difftool_state *dt_state,
+			 const struct object_id *oid, const char *path)
 {
 	char *data;
 	if (is_null_oid(oid)) {
 		/* The symlink is unknown to Git so read from the filesystem */
 		struct strbuf link = STRBUF_INIT;
-		if (has_symlinks) {
+		if (dt_state->has_symlinks) {
 			if (strbuf_readlink(&link, path, strlen(path)))
 				die(_("could not read symlink %s"), path);
 		} else if (strbuf_read_file(&link, path, 128))
@@ -307,8 +319,7 @@  static char *get_symlink(const struct object_id *oid, const char *path)
 	} else {
 		enum object_type type;
 		unsigned long size;
-		data = repo_read_object_file(the_repository, oid, &type,
-					     &size);
+		data = repo_read_object_file(repo, oid, &type, &size);
 		if (!data)
 			die(_("could not read object %s for symlink %s"),
 				oid_to_hex(oid), path);
@@ -355,7 +366,9 @@  static void write_standin_files(struct pair_entry *entry,
 		write_file_in_directory(rdir, rdir_len, entry->path, entry->right);
 }
 
-static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
+static int run_dir_diff(struct repository *repo,
+			struct difftool_state *dt_state,
+			const char *extcmd, const char *prefix,
 			struct child_process *child)
 {
 	struct strbuf info = STRBUF_INIT, lpath = STRBUF_INIT;
@@ -375,7 +388,7 @@  static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 	struct hashmap symlinks2 = HASHMAP_INIT(pair_cmp, NULL);
 	struct hashmap_iter iter;
 	struct pair_entry *entry;
-	struct index_state wtindex = INDEX_STATE_INIT(the_repository);
+	struct index_state wtindex = INDEX_STATE_INIT(repo);
 	struct checkout lstate, rstate;
 	int err = 0;
 	struct child_process cmd = CHILD_PROCESS_INIT;
@@ -383,7 +396,7 @@  static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 	struct hashmap tmp_modified = HASHMAP_INIT(path_entry_cmp, NULL);
 	int indices_loaded = 0;
 
-	workdir = repo_get_work_tree(the_repository);
+	workdir = repo_get_work_tree(repo);
 
 	/* Setup temp directories */
 	tmp = getenv("TMPDIR");
@@ -438,8 +451,7 @@  static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 			       "not supported in\n"
 			       "directory diff mode ('-d' and '--dir-diff')."));
 
-		if (parse_index_info(info.buf, &lmode, &rmode, &loid, &roid,
-				     &status))
+		if (parse_index_info(repo, info.buf, &lmode, &rmode, &loid, &roid, &status))
 			break;
 		if (strbuf_getline_nul(&lpath, fp))
 			break;
@@ -469,13 +481,13 @@  static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 		}
 
 		if (S_ISLNK(lmode)) {
-			char *content = get_symlink(&loid, src_path);
+			char *content = get_symlink(repo, dt_state, &loid, src_path);
 			add_left_or_right(&symlinks2, src_path, content, 0);
 			free(content);
 		}
 
 		if (S_ISLNK(rmode)) {
-			char *content = get_symlink(&roid, dst_path);
+			char *content = get_symlink(repo, dt_state, &roid, dst_path);
 			add_left_or_right(&symlinks2, dst_path, content, 1);
 			free(content);
 		}
@@ -500,7 +512,7 @@  static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 			}
 			hashmap_add(&working_tree_dups, &entry->entry);
 
-			if (!use_wt_file(workdir, dst_path, &roid)) {
+			if (!use_wt_file(repo, workdir, dst_path, &roid)) {
 				if (checkout_path(rmode, &roid, dst_path,
 						  &rstate)) {
 					ret = error("could not write '%s'",
@@ -528,7 +540,7 @@  static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 					goto finish;
 				}
 				add_path(&wtdir, wtdir_len, dst_path);
-				if (symlinks) {
+				if (dt_state->symlinks) {
 					if (symlink(wtdir.buf, rdir.buf)) {
 						ret = error_errno("could not symlink '%s' to '%s'", wtdir.buf, rdir.buf);
 						goto finish;
@@ -614,7 +626,7 @@  static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 		if (lstat(rdir.buf, &st))
 			continue;
 
-		if ((symlinks && S_ISLNK(st.st_mode)) || !S_ISREG(st.st_mode))
+		if ((dt_state->symlinks && S_ISLNK(st.st_mode)) || !S_ISREG(st.st_mode))
 			continue;
 
 		if (!indices_loaded) {
@@ -626,9 +638,9 @@  static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 				ret = error("could not write %s", buf.buf);
 				goto finish;
 			}
-			changed_files(&wt_modified, buf.buf, workdir);
+			changed_files(repo, &wt_modified, buf.buf, workdir);
 			strbuf_setlen(&rdir, rdir_len);
-			changed_files(&tmp_modified, buf.buf, rdir.buf);
+			changed_files(repo, &tmp_modified, buf.buf, rdir.buf);
 			add_path(&rdir, rdir_len, name);
 			indices_loaded = 1;
 		}
@@ -702,11 +714,15 @@  static int run_file_diff(int prompt, const char *prefix,
 int cmd_difftool(int argc,
 		 const char **argv,
 		 const char *prefix,
-		 struct repository *repo UNUSED)
+		 struct repository *repo)
 {
-	int use_gui_tool = -1, dir_diff = 0, prompt = -1, symlinks = 0,
-	    tool_help = 0, no_index = 0;
+	int use_gui_tool = -1, dir_diff = 0, prompt = -1, tool_help = 0, no_index = 0;
 	static char *difftool_cmd = NULL, *extcmd = NULL;
+	struct difftool_state dt_state = {
+		.has_symlinks = 1,
+		.symlinks = 1,
+		.trust_exit_code = 0
+	};
 	struct option builtin_difftool_options[] = {
 		OPT_BOOL('g', "gui", &use_gui_tool,
 			 N_("use `diff.guitool` instead of `diff.tool`")),
@@ -717,14 +733,14 @@  int cmd_difftool(int argc,
 			0, PARSE_OPT_NONEG),
 		OPT_SET_INT_F(0, "prompt", &prompt, NULL,
 			1, PARSE_OPT_NONEG | PARSE_OPT_HIDDEN),
-		OPT_BOOL(0, "symlinks", &symlinks,
+		OPT_BOOL(0, "symlinks", &dt_state.symlinks,
 			 N_("use symlinks in dir-diff mode")),
 		OPT_STRING('t', "tool", &difftool_cmd, N_("tool"),
 			   N_("use the specified diff tool")),
 		OPT_BOOL(0, "tool-help", &tool_help,
 			 N_("print a list of diff tools that may be used with "
 			    "`--tool`")),
-		OPT_BOOL(0, "trust-exit-code", &trust_exit_code,
+		OPT_BOOL(0, "trust-exit-code", &dt_state.trust_exit_code,
 			 N_("make 'git-difftool' exit when an invoked diff "
 			    "tool returns a non-zero exit code")),
 		OPT_STRING('x', "extcmd", &extcmd, N_("command"),
@@ -734,8 +750,9 @@  int cmd_difftool(int argc,
 	};
 	struct child_process child = CHILD_PROCESS_INIT;
 
-	git_config(difftool_config, NULL);
-	symlinks = has_symlinks;
+	if (repo)
+		repo_config(repo, difftool_config, &dt_state);
+	dt_state.symlinks = dt_state.has_symlinks;
 
 	argc = parse_options(argc, argv, prefix, builtin_difftool_options,
 			     builtin_difftool_usage, PARSE_OPT_KEEP_UNKNOWN_OPT |
@@ -749,8 +766,8 @@  int cmd_difftool(int argc,
 
 	if (!no_index){
 		setup_work_tree();
-		setenv(GIT_DIR_ENVIRONMENT, absolute_path(repo_get_git_dir(the_repository)), 1);
-		setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(repo_get_work_tree(the_repository)), 1);
+		setenv(GIT_DIR_ENVIRONMENT, absolute_path(repo_get_git_dir(repo)), 1);
+		setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(repo_get_work_tree(repo)), 1);
 	} else if (dir_diff)
 		die(_("options '%s' and '%s' cannot be used together"), "--dir-diff", "--no-index");
 
@@ -783,7 +800,7 @@  int cmd_difftool(int argc,
 	}
 
 	setenv("GIT_DIFFTOOL_TRUST_EXIT_CODE",
-	       trust_exit_code ? "true" : "false", 1);
+	       dt_state.trust_exit_code ? "true" : "false", 1);
 
 	/*
 	 * In directory diff mode, 'git-difftool--helper' is called once
@@ -799,6 +816,6 @@  int cmd_difftool(int argc,
 	strvec_pushv(&child.args, argv);
 
 	if (dir_diff)
-		return run_dir_diff(extcmd, symlinks, prefix, &child);
+		return run_dir_diff(repo, &dt_state, extcmd, prefix, &child);
 	return run_file_diff(prompt, prefix, &child);
 }