On Wed, Feb 5, 2025 at 8:20 PM David Aguilar <davvid@xxxxxxxxx> wrote:
>
> Move difftool's global variables into a difftools_option struct
> in preparation for removal of USE_THE_REPOSITORY_VARIABLE.
Thanks for splitting these out.
> Signed-off-by: David Aguilar <davvid@xxxxxxxxx>
> ---
> builtin/difftool.c | 51 ++++++++++++++++++++++++++++++----------------
> 1 file changed, 33 insertions(+), 18 deletions(-)
>
> diff --git a/builtin/difftool.c b/builtin/difftool.c
> index 03a8bb92a9..0b6b92aee0 100644
> --- a/builtin/difftool.c
> +++ b/builtin/difftool.c
> @@ -36,18 +36,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_options {
> + 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_options *dt_options = (struct difftool_options *)cb;
> if (!strcmp(var, "difftool.trustexitcode")) {
> - trust_exit_code = git_config_bool(var, value);
> + dt_options->trust_exit_code = git_config_bool(var, value);
> + return 0;
> + }
> + if (!strcmp(var, "core.symlinks")) {
> + dt_options->has_symlinks = git_config_bool(var, value);
It appears that the only use for has_symlinks....
> return 0;
> }
>
> @@ -291,13 +300,14 @@ 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 difftool_options *dt_options,
> + 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_options->has_symlinks) {
Why is this based on dt_options->has_symlinks rather than dt_options->symlinks?
(I guess this question is equivalent to asking why the preimage code
was using has_symlinks, instead of the symlinks parameter set from the
command line option. As far as I can see, has_symlinks is supposed to
merely function as a default value for symlinks in the case no command
line parameter is passed...but this is the one counter-example. But
was it an intentional counter-example, or an accident?)
That said, fixing this, if fixing is needed, doesn't belong in this
patch; it'd probably be better as a preparatory patch. But, it trips
up reviewers (looks like Patrick was wondering about the same thing on
v1 of your series), so it at least would probably be helpful to
mention in the commit message if no other cleanup is needed with
these.
> if (strbuf_readlink(&link, path, strlen(path)))
> die(_("could not read symlink %s"), path);
> } else if (strbuf_read_file(&link, path, 128))
> @@ -355,7 +365,8 @@ 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 difftool_options *dt_options,
> + const char *extcmd, const char *prefix,
> struct child_process *child)
> {
> struct strbuf info = STRBUF_INIT, lpath = STRBUF_INIT;
> @@ -469,13 +480,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(dt_options, &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(dt_options, &roid, dst_path);
> add_left_or_right(&symlinks2, dst_path, content, 1);
> free(content);
> }
> @@ -528,7 +539,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_options->symlinks) {
> if (symlink(wtdir.buf, rdir.buf)) {
> ret = error_errno("could not symlink '%s' to '%s'", wtdir.buf, rdir.buf);
> goto finish;
> @@ -614,7 +625,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_options->symlinks && S_ISLNK(st.st_mode)) || !S_ISREG(st.st_mode))
> continue;
>
> if (!indices_loaded) {
> @@ -704,9 +715,13 @@ int cmd_difftool(int argc,
> const char *prefix,
> struct repository *repo UNUSED)
> {
> - 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_options dt_options = {
> + .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 +732,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_options.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_options.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 +749,8 @@ int cmd_difftool(int argc,
> };
> struct child_process child = CHILD_PROCESS_INIT;
>
> - git_config(difftool_config, NULL);
> - symlinks = has_symlinks;
> + git_config(difftool_config, &dt_options);
> + dt_options.symlinks = dt_options.has_symlinks;
If the get_symlink() function should have been using
dt_options.symlinks instead of dt_options.has_symlinks, then
dt_options.has_symlinks is merely functioning as a default, but would
actually be superfluous. A follow-up patch could remove that extra
field.
>
> argc = parse_options(argc, argv, prefix, builtin_difftool_options,
> builtin_difftool_usage, PARSE_OPT_KEEP_UNKNOWN_OPT |
> @@ -783,7 +798,7 @@ int cmd_difftool(int argc,
> }
>
> setenv("GIT_DIFFTOOL_TRUST_EXIT_CODE",
> - trust_exit_code ? "true" : "false", 1);
> + dt_options.trust_exit_code ? "true" : "false", 1);
>
> /*
> * In directory diff mode, 'git-difftool--helper' is called once
> @@ -799,6 +814,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(&dt_options, extcmd, prefix, &child);
> return run_file_diff(prompt, prefix, &child);
> }
> --
