{"thread":{"id":"62910","subject":"[PATCH v2 1/3] difftool: eliminate use of global variables","startedAt":"2025-02-06T04:20:14Z","lastAt":"2025-02-07T17:49:17Z","messageCount":13,"participants":["David Aguilar","Elijah Newren","Junio C Hamano","Patrick Steinhardt"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"511923","messageId":"20250206042010.865947-1-davvid@gmail.com","threadId":"62910","inReplyTo":null,"subject":"[PATCH v2 1/3] difftool: eliminate use of global variables","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2025-02-06T04:20:08Z","receivedAt":"2025-02-06T04:20:14Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Move difftool's global variables into a difftools_option struct\nin preparation for removal of USE_THE_REPOSITORY_VARIABLE.\n\nSigned-off-by: David Aguilar \n---\n builtin/difftool.c | 51 ++++++++++++++++++++++++++++++----------------\n 1 file changed, 33 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex 03a8bb92a9..0b6b92aee0 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -36,18 +36,27 @@\n #include \"entry.h\"\n #include \"setup.h\"\n \n-static int trust_exit_code;\n-\n static const char *const builtin_difftool_usage[] = {\n \tN_(\"git difftool [] [ []] [--] [...]\"),\n \tNULL\n };\n \n+struct difftool_options {\n+\tint has_symlinks;\n+\tint symlinks;\n+\tint trust_exit_code;\n+};\n+\n static int difftool_config(const char *var, const char *value,\n \t\t\t const struct config_context *ctx, void *cb)\n {\n+\tstruct difftool_options *dt_options = (struct difftool_options *)cb;\n \tif (!strcmp(var, \"difftool.trustexitcode\")) {\n-\t\ttrust_exit_code = git_config_bool(var, value);\n+\t\tdt_options->trust_exit_code = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\tif (!strcmp(var, \"core.symlinks\")) {\n+\t\tdt_options->has_symlinks = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n \n@@ -291,13 +300,14 @@ static int ensure_leading_directories(char *path)\n * to compare the readlink(2) result as text, even on a filesystem that is\n * capable of doing a symbolic link.\n */\n-static char *get_symlink(const struct object_id *oid, const char *path)\n+static char *get_symlink(struct difftool_options *dt_options,\n+\t\t\t const struct object_id *oid, const char *path)\n {\n \tchar *data;\n \tif (is_null_oid(oid)) {\n \t\t/* The symlink is unknown to Git so read from the filesystem */\n \t\tstruct strbuf link = STRBUF_INIT;\n-\t\tif (has_symlinks) {\n+\t\tif (dt_options->has_symlinks) {\n \t\t\tif (strbuf_readlink(&link, path, strlen(path)))\n \t\t\t\tdie(_(\"could not read symlink %s\"), path);\n \t\t} else if (strbuf_read_file(&link, path, 128))\n@@ -355,7 +365,8 @@ static void write_standin_files(struct pair_entry *entry,\n \t\twrite_file_in_directory(rdir, rdir_len, entry->path, entry->right);\n }\n \n-static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n+static int run_dir_diff(struct difftool_options *dt_options,\n+\t\t\tconst char *extcmd, const char *prefix,\n \t\t\tstruct child_process *child)\n {\n \tstruct strbuf info = STRBUF_INIT, lpath = STRBUF_INIT;\n@@ -469,13 +480,13 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t\t}\n \n \t\tif (S_ISLNK(lmode)) {\n-\t\t\tchar *content = get_symlink(&loid, src_path);\n+\t\t\tchar *content = get_symlink(dt_options, &loid, src_path);\n \t\t\tadd_left_or_right(&symlinks2, src_path, content, 0);\n \t\t\tfree(content);\n \t\t}\n \n \t\tif (S_ISLNK(rmode)) {\n-\t\t\tchar *content = get_symlink(&roid, dst_path);\n+\t\t\tchar *content = get_symlink(dt_options, &roid, dst_path);\n \t\t\tadd_left_or_right(&symlinks2, dst_path, content, 1);\n \t\t\tfree(content);\n \t\t}\n@@ -528,7 +539,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t\t\t\t\tgoto finish;\n \t\t\t\t}\n \t\t\t\tadd_path(&wtdir, wtdir_len, dst_path);\n-\t\t\t\tif (symlinks) {\n+\t\t\t\tif (dt_options->symlinks) {\n \t\t\t\t\tif (symlink(wtdir.buf, rdir.buf)) {\n \t\t\t\t\t\tret = error_errno(\"could not symlink '%s' to '%s'\", wtdir.buf, rdir.buf);\n \t\t\t\t\t\tgoto finish;\n@@ -614,7 +625,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t\tif (lstat(rdir.buf, &st))\n \t\t\tcontinue;\n \n-\t\tif ((symlinks && S_ISLNK(st.st_mode)) || !S_ISREG(st.st_mode))\n+\t\tif ((dt_options->symlinks && S_ISLNK(st.st_mode)) || !S_ISREG(st.st_mode))\n \t\t\tcontinue;\n \n \t\tif (!indices_loaded) {\n@@ -704,9 +715,13 @@ int cmd_difftool(int argc,\n \t\t const char *prefix,\n \t\t struct repository *repo UNUSED)\n {\n-\tint use_gui_tool = -1, dir_diff = 0, prompt = -1, symlinks = 0,\n-\t tool_help = 0, no_index = 0;\n+\tint use_gui_tool = -1, dir_diff = 0, prompt = -1, tool_help = 0, no_index = 0;\n \tstatic char *difftool_cmd = NULL, *extcmd = NULL;\n+\tstruct difftool_options dt_options = {\n+\t\t.has_symlinks = 1,\n+\t\t.symlinks = 1,\n+\t\t.trust_exit_code = 0\n+\t};\n \tstruct option builtin_difftool_options[] = {\n \t\tOPT_BOOL('g', \"gui\", &use_gui_tool,\n \t\t\t N_(\"use `diff.guitool` instead of `diff.tool`\")),\n@@ -717,14 +732,14 @@ int cmd_difftool(int argc,\n \t\t\t0, PARSE_OPT_NONEG),\n \t\tOPT_SET_INT_F(0, \"prompt\", &prompt, NULL,\n \t\t\t1, PARSE_OPT_NONEG | PARSE_OPT_HIDDEN),\n-\t\tOPT_BOOL(0, \"symlinks\", &symlinks,\n+\t\tOPT_BOOL(0, \"symlinks\", &dt_options.symlinks,\n \t\t\t N_(\"use symlinks in dir-diff mode\")),\n \t\tOPT_STRING('t', \"tool\", &difftool_cmd, N_(\"tool\"),\n \t\t\t N_(\"use the specified diff tool\")),\n \t\tOPT_BOOL(0, \"tool-help\", &tool_help,\n \t\t\t N_(\"print a list of diff tools that may be used with \"\n \t\t\t \"`--tool`\")),\n-\t\tOPT_BOOL(0, \"trust-exit-code\", &trust_exit_code,\n+\t\tOPT_BOOL(0, \"trust-exit-code\", &dt_options.trust_exit_code,\n \t\t\t N_(\"make 'git-difftool' exit when an invoked diff \"\n \t\t\t \"tool returns a non-zero exit code\")),\n \t\tOPT_STRING('x', \"extcmd\", &extcmd, N_(\"command\"),\n@@ -734,8 +749,8 @@ int cmd_difftool(int argc,\n \t};\n \tstruct child_process child = CHILD_PROCESS_INIT;\n \n-\tgit_config(difftool_config, NULL);\n-\tsymlinks = has_symlinks;\n+\tgit_config(difftool_config, &dt_options);\n+\tdt_options.symlinks = dt_options.has_symlinks;\n \n \targc = parse_options(argc, argv, prefix, builtin_difftool_options,\n \t\t\t builtin_difftool_usage, PARSE_OPT_KEEP_UNKNOWN_OPT |\n@@ -783,7 +798,7 @@ int cmd_difftool(int argc,\n \t}\n \n \tsetenv(\"GIT_DIFFTOOL_TRUST_EXIT_CODE\",\n-\t trust_exit_code ? \"true\" : \"false\", 1);\n+\t dt_options.trust_exit_code ? \"true\" : \"false\", 1);\n \n \t/*\n \t * In directory diff mode, 'git-difftool--helper' is called once\n@@ -799,6 +814,6 @@ int cmd_difftool(int argc,\n \tstrvec_pushv(&child.args, argv);\n \n \tif (dir_diff)\n-\t\treturn run_dir_diff(extcmd, symlinks, prefix, &child);\n+\t\treturn run_dir_diff(&dt_options, extcmd, prefix, &child);\n \treturn run_file_diff(prompt, prefix, &child);\n }\n-- \n2.48.1.461.g612e419e04\n\n"},{"id":"511924","messageId":"20250206042010.865947-2-davvid@gmail.com","threadId":"62910","inReplyTo":"20250206042010.865947-1-davvid@gmail.com","subject":"[PATCH v2 2/3] difftool: eliminate use of the_repository","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2025-02-06T04:20:09Z","receivedAt":"2025-02-06T04:20:16Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Make callers pass a repository struct into each function instead\nof relying on the global the_repository variable.\n\nSigned-off-by: David Aguilar \n---\n builtin/difftool.c | 54 +++++++++++++++++++++++++---------------------\n 1 file changed, 29 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex 0b6b92aee0..81d733dfdf 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -72,7 +72,8 @@ static int print_tool_help(void)\n \treturn run_command(&cmd);\n }\n \n-static int parse_index_info(char *p, int *mode1, int *mode2,\n+static int parse_index_info(struct repository *repo,\n+\t\t\t char *p, int *mode1, int *mode2,\n \t\t\t struct object_id *oid1, struct object_id *oid2,\n \t\t\t char *status)\n {\n@@ -84,11 +85,11 @@ static int parse_index_info(char *p, int *mode1, int *mode2,\n \t*mode2 = (int)strtol(p + 1, &p, 8);\n \tif (*p != ' ')\n \t\treturn error(\"expected ' ', got '%c'\", *p);\n-\tif (parse_oid_hex(++p, oid1, (const char **)&p))\n+\tif (parse_oid_hex_algop(++p, oid1, (const char **)&p, repo->hash_algo))\n \t\treturn error(\"expected object ID, got '%s'\", p);\n \tif (*p != ' ')\n \t\treturn error(\"expected ' ', got '%c'\", *p);\n-\tif (parse_oid_hex(++p, oid2, (const char **)&p))\n+\tif (parse_oid_hex_algop(++p, oid2, (const char **)&p, repo->hash_algo))\n \t\treturn error(\"expected object ID, got '%s'\", p);\n \tif (*p != ' ')\n \t\treturn error(\"expected ' ', got '%c'\", *p);\n@@ -115,7 +116,8 @@ static void add_path(struct strbuf *buf, size_t base_len, const char *path)\n /*\n * Determine whether we can simply reuse the file in the worktree.\n */\n-static int use_wt_file(const char *workdir, const char *name,\n+static int use_wt_file(struct repository *repo,\n+\t\t const char *workdir, const char *name,\n \t\t struct object_id *oid)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -130,7 +132,7 @@ static int use_wt_file(const char *workdir, const char *name,\n \t\tint fd = open(buf.buf, O_RDONLY);\n \n \t\tif (fd >= 0 &&\n-\t\t !index_fd(the_repository->index, &wt_oid, fd, &st, OBJ_BLOB, name, 0)) {\n+\t\t !index_fd(repo->index, &wt_oid, fd, &st, OBJ_BLOB, name, 0)) {\n \t\t\tif (is_null_oid(oid)) {\n \t\t\t\toidcpy(oid, &wt_oid);\n \t\t\t\tuse = 1;\n@@ -221,13 +223,14 @@ static int path_entry_cmp(const void *cmp_data UNUSED,\n \treturn strcmp(a->path, key ? key : b->path);\n }\n \n-static void changed_files(struct hashmap *result, const char *index_path,\n+static void changed_files(struct repository *repo,\n+\t\t\t struct hashmap *result, const char *index_path,\n \t\t\t const char *workdir)\n {\n \tstruct child_process update_index = CHILD_PROCESS_INIT;\n \tstruct child_process diff_files = CHILD_PROCESS_INIT;\n \tstruct strbuf buf = STRBUF_INIT;\n-\tconst char *git_dir = absolute_path(repo_get_git_dir(the_repository));\n+\tconst char *git_dir = absolute_path(repo_get_git_dir(repo));\n \tFILE *fp;\n \n \tstrvec_pushl(&update_index.args,\n@@ -300,7 +303,8 @@ static int ensure_leading_directories(char *path)\n * to compare the readlink(2) result as text, even on a filesystem that is\n * capable of doing a symbolic link.\n */\n-static char *get_symlink(struct difftool_options *dt_options,\n+static char *get_symlink(struct repository *repo,\n+\t\t\t struct difftool_options *dt_options,\n \t\t\t const struct object_id *oid, const char *path)\n {\n \tchar *data;\n@@ -317,8 +321,7 @@ static char *get_symlink(struct difftool_options *dt_options,\n \t} else {\n \t\tenum object_type type;\n \t\tunsigned long size;\n-\t\tdata = repo_read_object_file(the_repository, oid, &type,\n-\t\t\t\t\t &size);\n+\t\tdata = repo_read_object_file(repo, oid, &type, &size);\n \t\tif (!data)\n \t\t\tdie(_(\"could not read object %s for symlink %s\"),\n \t\t\t\toid_to_hex(oid), path);\n@@ -365,7 +368,8 @@ static void write_standin_files(struct pair_entry *entry,\n \t\twrite_file_in_directory(rdir, rdir_len, entry->path, entry->right);\n }\n \n-static int run_dir_diff(struct difftool_options *dt_options,\n+static int run_dir_diff(struct repository *repo,\n+\t\t\tstruct difftool_options *dt_options,\n \t\t\tconst char *extcmd, const char *prefix,\n \t\t\tstruct child_process *child)\n {\n@@ -386,7 +390,7 @@ static int run_dir_diff(struct difftool_options *dt_options,\n \tstruct hashmap symlinks2 = HASHMAP_INIT(pair_cmp, NULL);\n \tstruct hashmap_iter iter;\n \tstruct pair_entry *entry;\n-\tstruct index_state wtindex = INDEX_STATE_INIT(the_repository);\n+\tstruct index_state wtindex = INDEX_STATE_INIT(repo);\n \tstruct checkout lstate, rstate;\n \tint err = 0;\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n@@ -394,7 +398,7 @@ static int run_dir_diff(struct difftool_options *dt_options,\n \tstruct hashmap tmp_modified = HASHMAP_INIT(path_entry_cmp, NULL);\n \tint indices_loaded = 0;\n \n-\tworkdir = repo_get_work_tree(the_repository);\n+\tworkdir = repo_get_work_tree(repo);\n \n \t/* Setup temp directories */\n \ttmp = getenv(\"TMPDIR\");\n@@ -449,8 +453,7 @@ static int run_dir_diff(struct difftool_options *dt_options,\n \t\t\t \"not supported in\\n\"\n \t\t\t \"directory diff mode ('-d' and '--dir-diff').\"));\n \n-\t\tif (parse_index_info(info.buf, &lmode, &rmode, &loid, &roid,\n-\t\t\t\t &status))\n+\t\tif (parse_index_info(repo, info.buf, &lmode, &rmode, &loid, &roid, &status))\n \t\t\tbreak;\n \t\tif (strbuf_getline_nul(&lpath, fp))\n \t\t\tbreak;\n@@ -480,13 +483,13 @@ static int run_dir_diff(struct difftool_options *dt_options,\n \t\t}\n \n \t\tif (S_ISLNK(lmode)) {\n-\t\t\tchar *content = get_symlink(dt_options, &loid, src_path);\n+\t\t\tchar *content = get_symlink(repo, dt_options, &loid, src_path);\n \t\t\tadd_left_or_right(&symlinks2, src_path, content, 0);\n \t\t\tfree(content);\n \t\t}\n \n \t\tif (S_ISLNK(rmode)) {\n-\t\t\tchar *content = get_symlink(dt_options, &roid, dst_path);\n+\t\t\tchar *content = get_symlink(repo, dt_options, &roid, dst_path);\n \t\t\tadd_left_or_right(&symlinks2, dst_path, content, 1);\n \t\t\tfree(content);\n \t\t}\n@@ -511,7 +514,7 @@ static int run_dir_diff(struct difftool_options *dt_options,\n \t\t\t}\n \t\t\thashmap_add(&working_tree_dups, &entry->entry);\n \n-\t\t\tif (!use_wt_file(workdir, dst_path, &roid)) {\n+\t\t\tif (!use_wt_file(repo, workdir, dst_path, &roid)) {\n \t\t\t\tif (checkout_path(rmode, &roid, dst_path,\n \t\t\t\t\t\t &rstate)) {\n \t\t\t\t\tret = error(\"could not write '%s'\",\n@@ -637,9 +640,9 @@ static int run_dir_diff(struct difftool_options *dt_options,\n \t\t\t\tret = error(\"could not write %s\", buf.buf);\n \t\t\t\tgoto finish;\n \t\t\t}\n-\t\t\tchanged_files(&wt_modified, buf.buf, workdir);\n+\t\t\tchanged_files(repo, &wt_modified, buf.buf, workdir);\n \t\t\tstrbuf_setlen(&rdir, rdir_len);\n-\t\t\tchanged_files(&tmp_modified, buf.buf, rdir.buf);\n+\t\t\tchanged_files(repo, &tmp_modified, buf.buf, rdir.buf);\n \t\t\tadd_path(&rdir, rdir_len, name);\n \t\t\tindices_loaded = 1;\n \t\t}\n@@ -713,7 +716,7 @@ static int run_file_diff(int prompt, const char *prefix,\n int cmd_difftool(int argc,\n \t\t const char **argv,\n \t\t const char *prefix,\n-\t\t struct repository *repo UNUSED)\n+\t\t struct repository *repo)\n {\n \tint use_gui_tool = -1, dir_diff = 0, prompt = -1, tool_help = 0, no_index = 0;\n \tstatic char *difftool_cmd = NULL, *extcmd = NULL;\n@@ -749,7 +752,8 @@ int cmd_difftool(int argc,\n \t};\n \tstruct child_process child = CHILD_PROCESS_INIT;\n \n-\tgit_config(difftool_config, &dt_options);\n+\tif (repo)\n+\t\trepo_config(repo, difftool_config, &dt_options);\n \tdt_options.symlinks = dt_options.has_symlinks;\n \n \targc = parse_options(argc, argv, prefix, builtin_difftool_options,\n@@ -764,8 +768,8 @@ int cmd_difftool(int argc,\n \n \tif (!no_index){\n \t\tsetup_work_tree();\n-\t\tsetenv(GIT_DIR_ENVIRONMENT, absolute_path(repo_get_git_dir(the_repository)), 1);\n-\t\tsetenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(repo_get_work_tree(the_repository)), 1);\n+\t\tsetenv(GIT_DIR_ENVIRONMENT, absolute_path(repo_get_git_dir(repo)), 1);\n+\t\tsetenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(repo_get_work_tree(repo)), 1);\n \t} else if (dir_diff)\n \t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--dir-diff\", \"--no-index\");\n \n@@ -814,6 +818,6 @@ int cmd_difftool(int argc,\n \tstrvec_pushv(&child.args, argv);\n \n \tif (dir_diff)\n-\t\treturn run_dir_diff(&dt_options, extcmd, prefix, &child);\n+\t\treturn run_dir_diff(repo, &dt_options, extcmd, prefix, &child);\n \treturn run_file_diff(prompt, prefix, &child);\n }\n-- \n2.48.1.461.g612e419e04\n\n"},{"id":"511925","messageId":"20250206042010.865947-3-davvid@gmail.com","threadId":"62910","inReplyTo":"20250206042010.865947-1-davvid@gmail.com","subject":"[PATCH v2 3/3] difftool: eliminate use of USE_THE_REPOSITORY_VARIABLE","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2025-02-06T04:20:10Z","receivedAt":"2025-02-06T04:20:18Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Remove the USE_THE_REPOSITORY_VARIABLE #define now that all\nstate is passed to each function from callers.\n\nSigned-off-by: David Aguilar \n---\n builtin/difftool.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex 81d733dfdf..41cd00066c 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -12,8 +12,6 @@\n * Copyright (C) 2016 Johannes Schindelin\n */\n \n-#define USE_THE_REPOSITORY_VARIABLE\n-\n #include \"builtin.h\"\n \n #include \"abspath.h\"\n-- \n2.48.1.461.g612e419e04\n\n"},{"id":"511986","messageId":"CABPp-BGnRgDxwgfagyvhwjso_kWVgcR-NxOUwUSJve5RHwyFZQ@mail.gmail.com","threadId":"62910","inReplyTo":"20250206042010.865947-1-davvid@gmail.com","subject":"Re: [PATCH v2 1/3] difftool: eliminate use of global variables","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-02-06T08:29:46Z","receivedAt":"2025-02-06T08:29:58Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Feb 5, 2025 at 8:20 PM David Aguilar wrote:\n>\n> Move difftool's global variables into a difftools_option struct\n> in preparation for removal of USE_THE_REPOSITORY_VARIABLE.\n\nThanks for splitting these out.\n\n> Signed-off-by: David Aguilar \n> ---\n> builtin/difftool.c | 51 ++++++++++++++++++++++++++++++----------------\n> 1 file changed, 33 insertions(+), 18 deletions(-)\n>\n> diff --git a/builtin/difftool.c b/builtin/difftool.c\n> index 03a8bb92a9..0b6b92aee0 100644\n> --- a/builtin/difftool.c\n> +++ b/builtin/difftool.c\n> @@ -36,18 +36,27 @@\n> #include \"entry.h\"\n> #include \"setup.h\"\n>\n> -static int trust_exit_code;\n> -\n> static const char *const builtin_difftool_usage[] = {\n> N_(\"git difftool [] [ []] [--] [...]\"),\n> NULL\n> };\n>\n> +struct difftool_options {\n> + int has_symlinks;\n> + int symlinks;\n> + int trust_exit_code;\n> +};\n> +\n> static int difftool_config(const char *var, const char *value,\n> const struct config_context *ctx, void *cb)\n> {\n> + struct difftool_options *dt_options = (struct difftool_options *)cb;\n> if (!strcmp(var, \"difftool.trustexitcode\")) {\n> - trust_exit_code = git_config_bool(var, value);\n> + dt_options->trust_exit_code = git_config_bool(var, value);\n> + return 0;\n> + }\n> + if (!strcmp(var, \"core.symlinks\")) {\n> + dt_options->has_symlinks = git_config_bool(var, value);\n\nIt appears that the only use for has_symlinks....\n\n> return 0;\n> }\n>\n> @@ -291,13 +300,14 @@ static int ensure_leading_directories(char *path)\n> * to compare the readlink(2) result as text, even on a filesystem that is\n> * capable of doing a symbolic link.\n> */\n> -static char *get_symlink(const struct object_id *oid, const char *path)\n> +static char *get_symlink(struct difftool_options *dt_options,\n> + const struct object_id *oid, const char *path)\n> {\n> char *data;\n> if (is_null_oid(oid)) {\n> /* The symlink is unknown to Git so read from the filesystem */\n> struct strbuf link = STRBUF_INIT;\n> - if (has_symlinks) {\n> + if (dt_options->has_symlinks) {\n\nWhy is this based on dt_options->has_symlinks rather than dt_options->symlinks?\n\n(I guess this question is equivalent to asking why the preimage code\nwas using has_symlinks, instead of the symlinks parameter set from the\ncommand line option. As far as I can see, has_symlinks is supposed to\nmerely function as a default value for symlinks in the case no command\nline parameter is passed...but this is the one counter-example. But\nwas it an intentional counter-example, or an accident?)\n\nThat said, fixing this, if fixing is needed, doesn't belong in this\npatch; it'd probably be better as a preparatory patch. But, it trips\nup reviewers (looks like Patrick was wondering about the same thing on\nv1 of your series), so it at least would probably be helpful to\nmention in the commit message if no other cleanup is needed with\nthese.\n\n> if (strbuf_readlink(&link, path, strlen(path)))\n> die(_(\"could not read symlink %s\"), path);\n> } else if (strbuf_read_file(&link, path, 128))\n> @@ -355,7 +365,8 @@ static void write_standin_files(struct pair_entry *entry,\n> write_file_in_directory(rdir, rdir_len, entry->path, entry->right);\n> }\n>\n> -static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n> +static int run_dir_diff(struct difftool_options *dt_options,\n> + const char *extcmd, const char *prefix,\n> struct child_process *child)\n> {\n> struct strbuf info = STRBUF_INIT, lpath = STRBUF_INIT;\n> @@ -469,13 +480,13 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n> }\n>\n> if (S_ISLNK(lmode)) {\n> - char *content = get_symlink(&loid, src_path);\n> + char *content = get_symlink(dt_options, &loid, src_path);\n> add_left_or_right(&symlinks2, src_path, content, 0);\n> free(content);\n> }\n>\n> if (S_ISLNK(rmode)) {\n> - char *content = get_symlink(&roid, dst_path);\n> + char *content = get_symlink(dt_options, &roid, dst_path);\n> add_left_or_right(&symlinks2, dst_path, content, 1);\n> free(content);\n> }\n> @@ -528,7 +539,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n> goto finish;\n> }\n> add_path(&wtdir, wtdir_len, dst_path);\n> - if (symlinks) {\n> + if (dt_options->symlinks) {\n> if (symlink(wtdir.buf, rdir.buf)) {\n> ret = error_errno(\"could not symlink '%s' to '%s'\", wtdir.buf, rdir.buf);\n> goto finish;\n> @@ -614,7 +625,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n> if (lstat(rdir.buf, &st))\n> continue;\n>\n> - if ((symlinks && S_ISLNK(st.st_mode)) || !S_ISREG(st.st_mode))\n> + if ((dt_options->symlinks && S_ISLNK(st.st_mode)) || !S_ISREG(st.st_mode))\n> continue;\n>\n> if (!indices_loaded) {\n> @@ -704,9 +715,13 @@ int cmd_difftool(int argc,\n> const char *prefix,\n> struct repository *repo UNUSED)\n> {\n> - int use_gui_tool = -1, dir_diff = 0, prompt = -1, symlinks = 0,\n> - tool_help = 0, no_index = 0;\n> + int use_gui_tool = -1, dir_diff = 0, prompt = -1, tool_help = 0, no_index = 0;\n> static char *difftool_cmd = NULL, *extcmd = NULL;\n> + struct difftool_options dt_options = {\n> + .has_symlinks = 1,\n> + .symlinks = 1,\n> + .trust_exit_code = 0\n> + };\n> struct option builtin_difftool_options[] = {\n> OPT_BOOL('g', \"gui\", &use_gui_tool,\n> N_(\"use `diff.guitool` instead of `diff.tool`\")),\n> @@ -717,14 +732,14 @@ int cmd_difftool(int argc,\n> 0, PARSE_OPT_NONEG),\n> OPT_SET_INT_F(0, \"prompt\", &prompt, NULL,\n> 1, PARSE_OPT_NONEG | PARSE_OPT_HIDDEN),\n> - OPT_BOOL(0, \"symlinks\", &symlinks,\n> + OPT_BOOL(0, \"symlinks\", &dt_options.symlinks,\n> N_(\"use symlinks in dir-diff mode\")),\n> OPT_STRING('t', \"tool\", &difftool_cmd, N_(\"tool\"),\n> N_(\"use the specified diff tool\")),\n> OPT_BOOL(0, \"tool-help\", &tool_help,\n> N_(\"print a list of diff tools that may be used with \"\n> \"`--tool`\")),\n> - OPT_BOOL(0, \"trust-exit-code\", &trust_exit_code,\n> + OPT_BOOL(0, \"trust-exit-code\", &dt_options.trust_exit_code,\n> N_(\"make 'git-difftool' exit when an invoked diff \"\n> \"tool returns a non-zero exit code\")),\n> OPT_STRING('x', \"extcmd\", &extcmd, N_(\"command\"),\n> @@ -734,8 +749,8 @@ int cmd_difftool(int argc,\n> };\n> struct child_process child = CHILD_PROCESS_INIT;\n>\n> - git_config(difftool_config, NULL);\n> - symlinks = has_symlinks;\n> + git_config(difftool_config, &dt_options);\n> + dt_options.symlinks = dt_options.has_symlinks;\n\nIf the get_symlink() function should have been using\ndt_options.symlinks instead of dt_options.has_symlinks, then\ndt_options.has_symlinks is merely functioning as a default, but would\nactually be superfluous. A follow-up patch could remove that extra\nfield.\n\n>\n> argc = parse_options(argc, argv, prefix, builtin_difftool_options,\n> builtin_difftool_usage, PARSE_OPT_KEEP_UNKNOWN_OPT |\n> @@ -783,7 +798,7 @@ int cmd_difftool(int argc,\n> }\n>\n> setenv(\"GIT_DIFFTOOL_TRUST_EXIT_CODE\",\n> - trust_exit_code ? \"true\" : \"false\", 1);\n> + dt_options.trust_exit_code ? \"true\" : \"false\", 1);\n>\n> /*\n> * In directory diff mode, 'git-difftool--helper' is called once\n> @@ -799,6 +814,6 @@ int cmd_difftool(int argc,\n> strvec_pushv(&child.args, argv);\n>\n> if (dir_diff)\n> - return run_dir_diff(extcmd, symlinks, prefix, &child);\n> + return run_dir_diff(&dt_options, extcmd, prefix, &child);\n> return run_file_diff(prompt, prefix, &child);\n> }\n> --\n"},{"id":"511987","messageId":"CABPp-BE_DAopJQPMR1s1m1pvBhsKUUe48GtXJgmhVr_VfvYR5w@mail.gmail.com","threadId":"62910","inReplyTo":"20250206042010.865947-2-davvid@gmail.com","subject":"Re: [PATCH v2 2/3] difftool: eliminate use of the_repository","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-02-06T08:30:54Z","receivedAt":"2025-02-06T08:31:07Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Feb 5, 2025 at 8:20 PM David Aguilar wrote:\n>\n> Make callers pass a repository struct into each function instead\n> of relying on the global the_repository variable.\n>\n> Signed-off-by: David Aguilar \n> ---\n> builtin/difftool.c | 54 +++++++++++++++++++++++++---------------------\n> 1 file changed, 29 insertions(+), 25 deletions(-)\n>\n> diff --git a/builtin/difftool.c b/builtin/difftool.c\n> index 0b6b92aee0..81d733dfdf 100644\n> --- a/builtin/difftool.c\n> +++ b/builtin/difftool.c\n> @@ -72,7 +72,8 @@ static int print_tool_help(void)\n> return run_command(&cmd);\n> }\n>\n> -static int parse_index_info(char *p, int *mode1, int *mode2,\n> +static int parse_index_info(struct repository *repo,\n> + char *p, int *mode1, int *mode2,\n> struct object_id *oid1, struct object_id *oid2,\n> char *status)\n> {\n> @@ -84,11 +85,11 @@ static int parse_index_info(char *p, int *mode1, int *mode2,\n> *mode2 = (int)strtol(p + 1, &p, 8);\n> if (*p != ' ')\n> return error(\"expected ' ', got '%c'\", *p);\n> - if (parse_oid_hex(++p, oid1, (const char **)&p))\n> + if (parse_oid_hex_algop(++p, oid1, (const char **)&p, repo->hash_algo))\n> return error(\"expected object ID, got '%s'\", p);\n> if (*p != ' ')\n> return error(\"expected ' ', got '%c'\", *p);\n> - if (parse_oid_hex(++p, oid2, (const char **)&p))\n> + if (parse_oid_hex_algop(++p, oid2, (const char **)&p, repo->hash_algo))\n> return error(\"expected object ID, got '%s'\", p);\n> if (*p != ' ')\n> return error(\"expected ' ', got '%c'\", *p);\n> @@ -115,7 +116,8 @@ static void add_path(struct strbuf *buf, size_t base_len, const char *path)\n> /*\n> * Determine whether we can simply reuse the file in the worktree.\n> */\n> -static int use_wt_file(const char *workdir, const char *name,\n> +static int use_wt_file(struct repository *repo,\n> + const char *workdir, const char *name,\n> struct object_id *oid)\n> {\n> struct strbuf buf = STRBUF_INIT;\n> @@ -130,7 +132,7 @@ static int use_wt_file(const char *workdir, const char *name,\n> int fd = open(buf.buf, O_RDONLY);\n>\n> if (fd >= 0 &&\n> - !index_fd(the_repository->index, &wt_oid, fd, &st, OBJ_BLOB, name, 0)) {\n> + !index_fd(repo->index, &wt_oid, fd, &st, OBJ_BLOB, name, 0)) {\n> if (is_null_oid(oid)) {\n> oidcpy(oid, &wt_oid);\n> use = 1;\n> @@ -221,13 +223,14 @@ static int path_entry_cmp(const void *cmp_data UNUSED,\n> return strcmp(a->path, key ? key : b->path);\n> }\n>\n> -static void changed_files(struct hashmap *result, const char *index_path,\n> +static void changed_files(struct repository *repo,\n> + struct hashmap *result, const char *index_path,\n> const char *workdir)\n> {\n> struct child_process update_index = CHILD_PROCESS_INIT;\n> struct child_process diff_files = CHILD_PROCESS_INIT;\n> struct strbuf buf = STRBUF_INIT;\n> - const char *git_dir = absolute_path(repo_get_git_dir(the_repository));\n> + const char *git_dir = absolute_path(repo_get_git_dir(repo));\n> FILE *fp;\n>\n> strvec_pushl(&update_index.args,\n> @@ -300,7 +303,8 @@ static int ensure_leading_directories(char *path)\n> * to compare the readlink(2) result as text, even on a filesystem that is\n> * capable of doing a symbolic link.\n> */\n> -static char *get_symlink(struct difftool_options *dt_options,\n> +static char *get_symlink(struct repository *repo,\n> + struct difftool_options *dt_options,\n> const struct object_id *oid, const char *path)\n> {\n> char *data;\n> @@ -317,8 +321,7 @@ static char *get_symlink(struct difftool_options *dt_options,\n> } else {\n> enum object_type type;\n> unsigned long size;\n> - data = repo_read_object_file(the_repository, oid, &type,\n> - &size);\n> + data = repo_read_object_file(repo, oid, &type, &size);\n> if (!data)\n> die(_(\"could not read object %s for symlink %s\"),\n> oid_to_hex(oid), path);\n> @@ -365,7 +368,8 @@ static void write_standin_files(struct pair_entry *entry,\n> write_file_in_directory(rdir, rdir_len, entry->path, entry->right);\n> }\n>\n> -static int run_dir_diff(struct difftool_options *dt_options,\n> +static int run_dir_diff(struct repository *repo,\n> + struct difftool_options *dt_options,\n> const char *extcmd, const char *prefix,\n> struct child_process *child)\n> {\n> @@ -386,7 +390,7 @@ static int run_dir_diff(struct difftool_options *dt_options,\n> struct hashmap symlinks2 = HASHMAP_INIT(pair_cmp, NULL);\n> struct hashmap_iter iter;\n> struct pair_entry *entry;\n> - struct index_state wtindex = INDEX_STATE_INIT(the_repository);\n> + struct index_state wtindex = INDEX_STATE_INIT(repo);\n> struct checkout lstate, rstate;\n> int err = 0;\n> struct child_process cmd = CHILD_PROCESS_INIT;\n> @@ -394,7 +398,7 @@ static int run_dir_diff(struct difftool_options *dt_options,\n> struct hashmap tmp_modified = HASHMAP_INIT(path_entry_cmp, NULL);\n> int indices_loaded = 0;\n>\n> - workdir = repo_get_work_tree(the_repository);\n> + workdir = repo_get_work_tree(repo);\n>\n> /* Setup temp directories */\n> tmp = getenv(\"TMPDIR\");\n> @@ -449,8 +453,7 @@ static int run_dir_diff(struct difftool_options *dt_options,\n> \"not supported in\\n\"\n> \"directory diff mode ('-d' and '--dir-diff').\"));\n>\n> - if (parse_index_info(info.buf, &lmode, &rmode, &loid, &roid,\n> - &status))\n> + if (parse_index_info(repo, info.buf, &lmode, &rmode, &loid, &roid, &status))\n> break;\n> if (strbuf_getline_nul(&lpath, fp))\n> break;\n> @@ -480,13 +483,13 @@ static int run_dir_diff(struct difftool_options *dt_options,\n> }\n>\n> if (S_ISLNK(lmode)) {\n> - char *content = get_symlink(dt_options, &loid, src_path);\n> + char *content = get_symlink(repo, dt_options, &loid, src_path);\n> add_left_or_right(&symlinks2, src_path, content, 0);\n> free(content);\n> }\n>\n> if (S_ISLNK(rmode)) {\n> - char *content = get_symlink(dt_options, &roid, dst_path);\n> + char *content = get_symlink(repo, dt_options, &roid, dst_path);\n> add_left_or_right(&symlinks2, dst_path, content, 1);\n> free(content);\n> }\n> @@ -511,7 +514,7 @@ static int run_dir_diff(struct difftool_options *dt_options,\n> }\n> hashmap_add(&working_tree_dups, &entry->entry);\n>\n> - if (!use_wt_file(workdir, dst_path, &roid)) {\n> + if (!use_wt_file(repo, workdir, dst_path, &roid)) {\n> if (checkout_path(rmode, &roid, dst_path,\n> &rstate)) {\n> ret = error(\"could not write '%s'\",\n> @@ -637,9 +640,9 @@ static int run_dir_diff(struct difftool_options *dt_options,\n> ret = error(\"could not write %s\", buf.buf);\n> goto finish;\n> }\n> - changed_files(&wt_modified, buf.buf, workdir);\n> + changed_files(repo, &wt_modified, buf.buf, workdir);\n> strbuf_setlen(&rdir, rdir_len);\n> - changed_files(&tmp_modified, buf.buf, rdir.buf);\n> + changed_files(repo, &tmp_modified, buf.buf, rdir.buf);\n> add_path(&rdir, rdir_len, name);\n> indices_loaded = 1;\n> }\n> @@ -713,7 +716,7 @@ static int run_file_diff(int prompt, const char *prefix,\n> int cmd_difftool(int argc,\n> const char **argv,\n> const char *prefix,\n> - struct repository *repo UNUSED)\n> + struct repository *repo)\n> {\n> int use_gui_tool = -1, dir_diff = 0, prompt = -1, tool_help = 0, no_index = 0;\n> static char *difftool_cmd = NULL, *extcmd = NULL;\n> @@ -749,7 +752,8 @@ int cmd_difftool(int argc,\n> };\n> struct child_process child = CHILD_PROCESS_INIT;\n>\n> - git_config(difftool_config, &dt_options);\n> + if (repo)\n> + repo_config(repo, difftool_config, &dt_options);\n> dt_options.symlinks = dt_options.has_symlinks;\n>\n> argc = parse_options(argc, argv, prefix, builtin_difftool_options,\n> @@ -764,8 +768,8 @@ int cmd_difftool(int argc,\n>\n> if (!no_index){\n> setup_work_tree();\n> - setenv(GIT_DIR_ENVIRONMENT, absolute_path(repo_get_git_dir(the_repository)), 1);\n> - setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(repo_get_work_tree(the_repository)), 1);\n> + setenv(GIT_DIR_ENVIRONMENT, absolute_path(repo_get_git_dir(repo)), 1);\n> + setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(repo_get_work_tree(repo)), 1);\n> } else if (dir_diff)\n> die(_(\"options '%s' and '%s' cannot be used together\"), \"--dir-diff\", \"--no-index\");\n>\n> @@ -814,6 +818,6 @@ int cmd_difftool(int argc,\n> strvec_pushv(&child.args, argv);\n>\n> if (dir_diff)\n> - return run_dir_diff(&dt_options, extcmd, prefix, &child);\n> + return run_dir_diff(repo, &dt_options, extcmd, prefix, &child);\n> return run_file_diff(prompt, prefix, &child);\n> }\n> --\n> 2.48.1.461.g612e419e04\n\nThis is so much easier to read with the separate option struct being\nsplit out into a separate patch. Looks good to me.\n"},{"id":"511988","messageId":"CABPp-BGi51EQvPOgaz8p0RjAJ0zDDeTpMDp__puz_wQH5wowXw@mail.gmail.com","threadId":"62910","inReplyTo":"20250206042010.865947-3-davvid@gmail.com","subject":"Re: [PATCH v2 3/3] difftool: eliminate use of USE_THE_REPOSITORY_VARIABLE","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-02-06T08:31:14Z","receivedAt":"2025-02-06T08:31:25Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Feb 5, 2025 at 8:20 PM David Aguilar wrote:\n>\n> Remove the USE_THE_REPOSITORY_VARIABLE #define now that all\n> state is passed to each function from callers.\n>\n> Signed-off-by: David Aguilar \n> ---\n> builtin/difftool.c | 2 --\n> 1 file changed, 2 deletions(-)\n>\n> diff --git a/builtin/difftool.c b/builtin/difftool.c\n> index 81d733dfdf..41cd00066c 100644\n> --- a/builtin/difftool.c\n> +++ b/builtin/difftool.c\n> @@ -12,8 +12,6 @@\n> * Copyright (C) 2016 Johannes Schindelin\n> */\n>\n> -#define USE_THE_REPOSITORY_VARIABLE\n> -\n> #include \"builtin.h\"\n>\n> #include \"abspath.h\"\n> --\n> 2.48.1.461.g612e419e04\n\n\nNice; thanks for working on this.\n"},{"id":"511997","messageId":"xmqqy0yjjix4.fsf@gitster.g","threadId":"62910","inReplyTo":"20250206042010.865947-1-davvid@gmail.com","subject":"Re: [PATCH v2 1/3] difftool: eliminate use of global variables","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-02-06T13:34:47Z","receivedAt":"2025-02-06T13:34:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar writes:\n\n> Move difftool's global variables into a difftools_option struct\n> in preparation for removal of USE_THE_REPOSITORY_VARIABLE.\n\nBoth may be good things, but I am puzzled by the \"in preparation\nfor\" part of the above description. Would it require we lose these\nthree global variables if we wanted to pass through a repository\ninstance through the callchain instead of relying on implicit use of\nthe_repository?\n\nAren't these pretty much independent and orthogonal?\n\n\n>\n> Signed-off-by: David Aguilar \n> ---\n> builtin/difftool.c | 51 ++++++++++++++++++++++++++++++----------------\n> 1 file changed, 33 insertions(+), 18 deletions(-)\n>\n> diff --git a/builtin/difftool.c b/builtin/difftool.c\n> index 03a8bb92a9..0b6b92aee0 100644\n> --- a/builtin/difftool.c\n> +++ b/builtin/difftool.c\n> @@ -36,18 +36,27 @@\n> #include \"entry.h\"\n> #include \"setup.h\"\n> \n> -static int trust_exit_code;\n> -\n> static const char *const builtin_difftool_usage[] = {\n> \tN_(\"git difftool [] [ []] [--] [...]\"),\n> \tNULL\n> };\n> \n> +struct difftool_options {\n> +\tint has_symlinks;\n> +\tint symlinks;\n> +\tint trust_exit_code;\n> +};\n> +\n> static int difftool_config(const char *var, const char *value,\n> \t\t\t const struct config_context *ctx, void *cb)\n> {\n> +\tstruct difftool_options *dt_options = (struct difftool_options *)cb;\n> \tif (!strcmp(var, \"difftool.trustexitcode\")) {\n> -\t\ttrust_exit_code = git_config_bool(var, value);\n> +\t\tdt_options->trust_exit_code = git_config_bool(var, value);\n> +\t\treturn 0;\n> +\t}\n> +\tif (!strcmp(var, \"core.symlinks\")) {\n> +\t\tdt_options->has_symlinks = git_config_bool(var, value);\n> \t\treturn 0;\n> \t}\n> \n> @@ -291,13 +300,14 @@ static int ensure_leading_directories(char *path)\n> * to compare the readlink(2) result as text, even on a filesystem that is\n> * capable of doing a symbolic link.\n> */\n> -static char *get_symlink(const struct object_id *oid, const char *path)\n> +static char *get_symlink(struct difftool_options *dt_options,\n> +\t\t\t const struct object_id *oid, const char *path)\n> {\n> \tchar *data;\n> \tif (is_null_oid(oid)) {\n> \t\t/* The symlink is unknown to Git so read from the filesystem */\n> \t\tstruct strbuf link = STRBUF_INIT;\n> -\t\tif (has_symlinks) {\n> +\t\tif (dt_options->has_symlinks) {\n> \t\t\tif (strbuf_readlink(&link, path, strlen(path)))\n> \t\t\t\tdie(_(\"could not read symlink %s\"), path);\n> \t\t} else if (strbuf_read_file(&link, path, 128))\n> @@ -355,7 +365,8 @@ static void write_standin_files(struct pair_entry *entry,\n> \t\twrite_file_in_directory(rdir, rdir_len, entry->path, entry->right);\n> }\n> \n> -static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n> +static int run_dir_diff(struct difftool_options *dt_options,\n> +\t\t\tconst char *extcmd, const char *prefix,\n> \t\t\tstruct child_process *child)\n> {\n> \tstruct strbuf info = STRBUF_INIT, lpath = STRBUF_INIT;\n> @@ -469,13 +480,13 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n> \t\t}\n> \n> \t\tif (S_ISLNK(lmode)) {\n> -\t\t\tchar *content = get_symlink(&loid, src_path);\n> +\t\t\tchar *content = get_symlink(dt_options, &loid, src_path);\n> \t\t\tadd_left_or_right(&symlinks2, src_path, content, 0);\n> \t\t\tfree(content);\n> \t\t}\n> \n> \t\tif (S_ISLNK(rmode)) {\n> -\t\t\tchar *content = get_symlink(&roid, dst_path);\n> +\t\t\tchar *content = get_symlink(dt_options, &roid, dst_path);\n> \t\t\tadd_left_or_right(&symlinks2, dst_path, content, 1);\n> \t\t\tfree(content);\n> \t\t}\n> @@ -528,7 +539,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n> \t\t\t\t\tgoto finish;\n> \t\t\t\t}\n> \t\t\t\tadd_path(&wtdir, wtdir_len, dst_path);\n> -\t\t\t\tif (symlinks) {\n> +\t\t\t\tif (dt_options->symlinks) {\n> \t\t\t\t\tif (symlink(wtdir.buf, rdir.buf)) {\n> \t\t\t\t\t\tret = error_errno(\"could not symlink '%s' to '%s'\", wtdir.buf, rdir.buf);\n> \t\t\t\t\t\tgoto finish;\n> @@ -614,7 +625,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n> \t\tif (lstat(rdir.buf, &st))\n> \t\t\tcontinue;\n> \n> -\t\tif ((symlinks && S_ISLNK(st.st_mode)) || !S_ISREG(st.st_mode))\n> +\t\tif ((dt_options->symlinks && S_ISLNK(st.st_mode)) || !S_ISREG(st.st_mode))\n> \t\t\tcontinue;\n> \n> \t\tif (!indices_loaded) {\n> @@ -704,9 +715,13 @@ int cmd_difftool(int argc,\n> \t\t const char *prefix,\n> \t\t struct repository *repo UNUSED)\n> {\n> -\tint use_gui_tool = -1, dir_diff = 0, prompt = -1, symlinks = 0,\n> -\t tool_help = 0, no_index = 0;\n> +\tint use_gui_tool = -1, dir_diff = 0, prompt = -1, tool_help = 0, no_index = 0;\n> \tstatic char *difftool_cmd = NULL, *extcmd = NULL;\n> +\tstruct difftool_options dt_options = {\n> +\t\t.has_symlinks = 1,\n> +\t\t.symlinks = 1,\n> +\t\t.trust_exit_code = 0\n> +\t};\n> \tstruct option builtin_difftool_options[] = {\n> \t\tOPT_BOOL('g', \"gui\", &use_gui_tool,\n> \t\t\t N_(\"use `diff.guitool` instead of `diff.tool`\")),\n> @@ -717,14 +732,14 @@ int cmd_difftool(int argc,\n> \t\t\t0, PARSE_OPT_NONEG),\n> \t\tOPT_SET_INT_F(0, \"prompt\", &prompt, NULL,\n> \t\t\t1, PARSE_OPT_NONEG | PARSE_OPT_HIDDEN),\n> -\t\tOPT_BOOL(0, \"symlinks\", &symlinks,\n> +\t\tOPT_BOOL(0, \"symlinks\", &dt_options.symlinks,\n> \t\t\t N_(\"use symlinks in dir-diff mode\")),\n> \t\tOPT_STRING('t', \"tool\", &difftool_cmd, N_(\"tool\"),\n> \t\t\t N_(\"use the specified diff tool\")),\n> \t\tOPT_BOOL(0, \"tool-help\", &tool_help,\n> \t\t\t N_(\"print a list of diff tools that may be used with \"\n> \t\t\t \"`--tool`\")),\n> -\t\tOPT_BOOL(0, \"trust-exit-code\", &trust_exit_code,\n> +\t\tOPT_BOOL(0, \"trust-exit-code\", &dt_options.trust_exit_code,\n> \t\t\t N_(\"make 'git-difftool' exit when an invoked diff \"\n> \t\t\t \"tool returns a non-zero exit code\")),\n> \t\tOPT_STRING('x', \"extcmd\", &extcmd, N_(\"command\"),\n> @@ -734,8 +749,8 @@ int cmd_difftool(int argc,\n> \t};\n> \tstruct child_process child = CHILD_PROCESS_INIT;\n> \n> -\tgit_config(difftool_config, NULL);\n> -\tsymlinks = has_symlinks;\n> +\tgit_config(difftool_config, &dt_options);\n> +\tdt_options.symlinks = dt_options.has_symlinks;\n> \n> \targc = parse_options(argc, argv, prefix, builtin_difftool_options,\n> \t\t\t builtin_difftool_usage, PARSE_OPT_KEEP_UNKNOWN_OPT |\n> @@ -783,7 +798,7 @@ int cmd_difftool(int argc,\n> \t}\n> \n> \tsetenv(\"GIT_DIFFTOOL_TRUST_EXIT_CODE\",\n> -\t trust_exit_code ? \"true\" : \"false\", 1);\n> +\t dt_options.trust_exit_code ? \"true\" : \"false\", 1);\n> \n> \t/*\n> \t * In directory diff mode, 'git-difftool--helper' is called once\n> @@ -799,6 +814,6 @@ int cmd_difftool(int argc,\n> \tstrvec_pushv(&child.args, argv);\n> \n> \tif (dir_diff)\n> -\t\treturn run_dir_diff(extcmd, symlinks, prefix, &child);\n> +\t\treturn run_dir_diff(&dt_options, extcmd, prefix, &child);\n> \treturn run_file_diff(prompt, prefix, &child);\n> }\n"},{"id":"512009","messageId":"CABPp-BHpqSRy=G4HB+QtbFuP8Bohw6Cd99va2++PawehCDt0Aw@mail.gmail.com","threadId":"62910","inReplyTo":"xmqqy0yjjix4.fsf@gitster.g","subject":"Re: [PATCH v2 1/3] difftool: eliminate use of global variables","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-02-06T18:08:29Z","receivedAt":"2025-02-06T18:08:42Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Feb 6, 2025 at 5:34 AM Junio C Hamano wrote:\n>\n> David Aguilar writes:\n>\n> > Move difftool's global variables into a difftools_option struct\n> > in preparation for removal of USE_THE_REPOSITORY_VARIABLE.\n>\n> Both may be good things, but I am puzzled by the \"in preparation\n> for\" part of the above description. Would it require we lose these\n> three global variables if we wanted to pass through a repository\n> instance through the callchain instead of relying on implicit use of\n> the_repository?\n>\n> Aren't these pretty much independent and orthogonal?\n\nThe declaration of 'extern int has_symlinks;' in environment.h is\nguarded by an #ifdef USE_THE_REPOSITORY_VARIABLE, so if you want to\nstop declaring that, you need to both pass a repository through and\nstop using that global variable. (The change to trust_exit_code and\nsymlinks vars do seem to be independent, but kind of make sense to\nhandle at the same time you are changing how has_symlinks is treated.)\n"},{"id":"512012","messageId":"xmqq8qqiki2a.fsf@gitster.g","threadId":"62910","inReplyTo":"CABPp-BHpqSRy=G4HB+QtbFuP8Bohw6Cd99va2++PawehCDt0Aw@mail.gmail.com","subject":"Re: [PATCH v2 1/3] difftool: eliminate use of global variables","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-02-06T19:07:57Z","receivedAt":"2025-02-06T19:08:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> stop using that global variable. (The change to trust_exit_code and\n> symlinks vars do seem to be independent, but kind of make sense to\n> handle at the same time you are changing how has_symlinks is treated.)\n\nFair enough. Thanks.\n"},{"id":"512024","messageId":"Z6WPudTWInXagCYg@gmail.com","threadId":"62910","inReplyTo":"CABPp-BGnRgDxwgfagyvhwjso_kWVgcR-NxOUwUSJve5RHwyFZQ@mail.gmail.com","subject":"Re: [PATCH v2 1/3] difftool: eliminate use of global variables","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2025-02-07T04:44:41Z","receivedAt":"2025-02-07T04:44:45Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Thu, Feb 06, 2025 at 12:29:46AM -0800, Elijah Newren wrote:\n> On Wed, Feb 5, 2025 at 8:20 PM David Aguilar wrote:\n> > diff --git a/builtin/difftool.c b/builtin/difftool.c\n> > index 03a8bb92a9..0b6b92aee0 100644\n> > --- a/builtin/difftool.c\n> > +++ b/builtin/difftool.c\n> > @@ -36,18 +36,27 @@\n> > #include \"entry.h\"\n> > #include \"setup.h\"\n> >\n> > -static int trust_exit_code;\n> > -\n> > static const char *const builtin_difftool_usage[] = {\n> > N_(\"git difftool [] [ []] [--] [...]\"),\n> > NULL\n> > };\n> >\n> > +struct difftool_options {\n> > + int has_symlinks;\n> > + int symlinks;\n> > + int trust_exit_code;\n> > +};\n> > +\n> > static int difftool_config(const char *var, const char *value,\n> > const struct config_context *ctx, void *cb)\n> > {\n> > + struct difftool_options *dt_options = (struct difftool_options *)cb;\n> > if (!strcmp(var, \"difftool.trustexitcode\")) {\n> > - trust_exit_code = git_config_bool(var, value);\n> > + dt_options->trust_exit_code = git_config_bool(var, value);\n> > + return 0;\n> > + }\n> > + if (!strcmp(var, \"core.symlinks\")) {\n> > + dt_options->has_symlinks = git_config_bool(var, value);\n> \n> It appears that the only use for has_symlinks....\n> \n> > return 0;\n> > }\n> >\n> > @@ -291,13 +300,14 @@ static int ensure_leading_directories(char *path)\n> > * to compare the readlink(2) result as text, even on a filesystem that is\n> > * capable of doing a symbolic link.\n> > */\n> > -static char *get_symlink(const struct object_id *oid, const char *path)\n> > +static char *get_symlink(struct difftool_options *dt_options,\n> > + const struct object_id *oid, const char *path)\n> > {\n> > char *data;\n> > if (is_null_oid(oid)) {\n> > /* The symlink is unknown to Git so read from the filesystem */\n> > struct strbuf link = STRBUF_INIT;\n> > - if (has_symlinks) {\n> > + if (dt_options->has_symlinks) {\n> \n> Why is this based on dt_options->has_symlinks rather than dt_options->symlinks?\n> \n> (I guess this question is equivalent to asking why the preimage code\n> was using has_symlinks, instead of the symlinks parameter set from the\n> command line option. As far as I can see, has_symlinks is supposed to\n> merely function as a default value for symlinks in the case no command\n> line parameter is passed...but this is the one counter-example. But\n> was it an intentional counter-example, or an accident?)\n> \n> That said, fixing this, if fixing is needed, doesn't belong in this\n> patch; it'd probably be better as a preparatory patch. But, it trips\n> up reviewers (looks like Patrick was wondering about the same thing on\n> v1 of your series), so it at least would probably be helpful to\n> mention in the commit message if no other cleanup is needed with\n> these.\n\nAgreed. If we fix this it should be done in a separate patch and\nwe can explain why they were separate variables as part of that\ncommit message. I don't necessarily agree that it belongs in this patch.\n\nCombining these two fields leads to test errors which is why\nit wasn't touched in this round.\n\n\n> > @@ -734,8 +749,8 @@ int cmd_difftool(int argc,\n> > };\n> > struct child_process child = CHILD_PROCESS_INIT;\n> >\n> > - git_config(difftool_config, NULL);\n> > - symlinks = has_symlinks;\n> > + git_config(difftool_config, &dt_options);\n> > + dt_options.symlinks = dt_options.has_symlinks;\n> \n> If the get_symlink() function should have been using\n> dt_options.symlinks instead of dt_options.has_symlinks, then\n> dt_options.has_symlinks is merely functioning as a default, but would\n> actually be superfluous. A follow-up patch could remove that extra\n> field.\n\n`has_symlinks` is currently providing both a default value and\ncontrolling the behavior of the dir-diff mode, so it's not quite\nmerely functioning as a default.\n\nMy eyes gloss over comments because I completely missed the following\nexplanation in the comment above `get_symlink()`.\nThis comment explain why we have a separate `have_symlinks` field:\n\n/*\n * Unconditional writing of a plain regular file is what\n * \"git difftool --dir-diff\" wants to do for symlinks. We are preparing two\n * temporary directories to be fed to a Git-unaware tool that knows how to\n * show a diff of two directories (e.g. \"diff -r A B\").\n *\n * Because the tool is Git-unaware, if a symbolic link appears in either of\n * these temporary directories, it will try to dereference and show the\n * difference of the target of the symbolic link, which is not what we want,\n * as the goal of the dir-diff mode is to produce an output that is logically\n * equivalent to what \"git diff\" produces.\n *\n * Most importantly, we want to get textual comparison of the result of the\n * readlink(2). get_symlink() provides that---it returns the contents of\n * the symlink that gets written to a regular file to force the external tool\n * to compare the readlink(2) result as text, even on a filesystem that is\n * capable of doing a symbolic link.\n */\n\nIn other words, we intetionally take the extra step to readlink(2)\nsymlinks in the dir-diff mode irrespective of the command-line option on\nsystems that support symlinks. That's why `has_symlinks` has to be\ntracked separately.\n\nIn light of this, I suspect that we won't be combining these fields\nbecause this behavior is intentional and necessary.\n\n`git blame` claims that I wrote this comment 8 years ago, but that's\nnews to me!\n\nThanks for the thorough review. I'm not planning a re-roll since it\nseems like this is fine as-is, but let me know if y'all feel otherwise.\n\nOne thing I would maybe change would be to rename `dt_options` to\n`options`, but I also appreciate the verbosity of the dt_ prefix.\n\nInterestingly, the `struct difftool_state` and `dt_state` names in the\noriginal patch were chosen because the struct contained more than just\noptions. Specifically, it contains the `has_symlinks` field.\nI'm not really sure it's worth splitting hairs over that detail,\nbut I'm all ears. struct difftool_options doesn't really bother me.\n\ncheers,\n-- \nDavid\n"},{"id":"512033","messageId":"Z6WoDpX5bvXa21qo@pks.im","threadId":"62910","inReplyTo":"CABPp-BHpqSRy=G4HB+QtbFuP8Bohw6Cd99va2++PawehCDt0Aw@mail.gmail.com","subject":"Re: [PATCH v2 1/3] difftool: eliminate use of global variables","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-07T06:28:30Z","receivedAt":"2025-02-07T06:28:35Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Feb 06, 2025 at 10:08:29AM -0800, Elijah Newren wrote:\n> On Thu, Feb 6, 2025 at 5:34 AM Junio C Hamano wrote:\n> >\n> > David Aguilar writes:\n> >\n> > > Move difftool's global variables into a difftools_option struct\n> > > in preparation for removal of USE_THE_REPOSITORY_VARIABLE.\n> >\n> > Both may be good things, but I am puzzled by the \"in preparation\n> > for\" part of the above description. Would it require we lose these\n> > three global variables if we wanted to pass through a repository\n> > instance through the callchain instead of relying on implicit use of\n> > the_repository?\n> >\n> > Aren't these pretty much independent and orthogonal?\n> \n> The declaration of 'extern int has_symlinks;' in environment.h is\n> guarded by an #ifdef USE_THE_REPOSITORY_VARIABLE, so if you want to\n> stop declaring that, you need to both pass a repository through and\n> stop using that global variable. (The change to trust_exit_code and\n> symlinks vars do seem to be independent, but kind of make sense to\n> handle at the same time you are changing how has_symlinks is treated.)\n\nAh, that makes sense. It raises the question whether the refactoring\nthus breaks something because we don't use that global variable anymore,\ne.g. if we were munging it in the preimage. But as far as I can see we\ndon't modify it at all, so this should be fine.\n\nPatrick\n"},{"id":"512034","messageId":"Z6WoEjEo9Seffm_q@pks.im","threadId":"62910","inReplyTo":"CABPp-BGi51EQvPOgaz8p0RjAJ0zDDeTpMDp__puz_wQH5wowXw@mail.gmail.com","subject":"Re: [PATCH v2 3/3] difftool: eliminate use of USE_THE_REPOSITORY_VARIABLE","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-07T06:28:34Z","receivedAt":"2025-02-07T06:28:38Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Feb 06, 2025 at 12:31:14AM -0800, Elijah Newren wrote:\n> On Wed, Feb 5, 2025 at 8:20 PM David Aguilar wrote:\n> >\n> > Remove the USE_THE_REPOSITORY_VARIABLE #define now that all\n> > state is passed to each function from callers.\n> >\n> > Signed-off-by: David Aguilar \n> > ---\n> > builtin/difftool.c | 2 --\n> > 1 file changed, 2 deletions(-)\n> >\n> > diff --git a/builtin/difftool.c b/builtin/difftool.c\n> > index 81d733dfdf..41cd00066c 100644\n> > --- a/builtin/difftool.c\n> > +++ b/builtin/difftool.c\n> > @@ -12,8 +12,6 @@\n> > * Copyright (C) 2016 Johannes Schindelin\n> > */\n> >\n> > -#define USE_THE_REPOSITORY_VARIABLE\n> > -\n> > #include \"builtin.h\"\n> >\n> > #include \"abspath.h\"\n> > --\n> > 2.48.1.461.g612e419e04\n> \n> \n> Nice; thanks for working on this.\n\nYup, thanks for rerolling. This version is much easier to read indeed\nand looks good to me.\n\nPatrick\n"},{"id":"512110","messageId":"xmqq4j15hch1.fsf@gitster.g","threadId":"62910","inReplyTo":"Z6WoEjEo9Seffm_q@pks.im","subject":"Re: [PATCH v2 3/3] difftool: eliminate use of USE_THE_REPOSITORY_VARIABLE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-02-07T17:49:14Z","receivedAt":"2025-02-07T17:49:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Thu, Feb 06, 2025 at 12:31:14AM -0800, Elijah Newren wrote:\n>> On Wed, Feb 5, 2025 at 8:20 PM David Aguilar wrote:\n>> ...\n>> \n>> Nice; thanks for working on this.\n>\n> Yup, thanks for rerolling. This version is much easier to read indeed\n> and looks good to me.\n\nThanks, all, for working on this and reviewing the patches.\nLooking very good.\n\nLet me mark the topic for 'next'.\n"}]}