Re: [PATCH v2 1/3] difftool: eliminate use of global variables

On Thu, Feb 06, 2025 at 12:29:46AM -0800, Elijah Newren wrote:

On Wed, Feb 5, 2025 at 8:20 PM David Aguilar [off-list ref] wrote:

quoted

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

quoted

                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.

Agreed. If we fix this it should be done in a separate patch and
we can explain why they were separate variables as part of that
commit message. I don't necessarily agree that it belongs in this patch.

Combining these two fields leads to test errors which is why
it wasn't touched in this round.

quoted

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

`has_symlinks` is currently providing both a default value and
controlling the behavior of the dir-diff mode, so it's not quite
merely functioning as a default.

My eyes gloss over comments because I completely missed the following
explanation in the comment above `get_symlink()`.
This comment explain why we have a separate `have_symlinks` field:

/*
 * Unconditional writing of a plain regular file is what
 * "git difftool --dir-diff" wants to do for symlinks.  We are preparing two
 * temporary directories to be fed to a Git-unaware tool that knows how to
 * show a diff of two directories (e.g. "diff -r A B").
 *
 * Because the tool is Git-unaware, if a symbolic link appears in either of
 * these temporary directories, it will try to dereference and show the
 * difference of the target of the symbolic link, which is not what we want,
 * as the goal of the dir-diff mode is to produce an output that is logically
 * equivalent to what "git diff" produces.
 *
 * Most importantly, we want to get textual comparison of the result of the
 * readlink(2).  get_symlink() provides that---it returns the contents of
 * the symlink that gets written to a regular file to force the external tool
 * to compare the readlink(2) result as text, even on a filesystem that is
 * capable of doing a symbolic link.
 */

In other words, we intetionally take the extra step to readlink(2)
symlinks in the dir-diff mode irrespective of the command-line option on
systems that support symlinks. That's why `has_symlinks` has to be
tracked separately.

In light of this, I suspect that we won't be combining these fields
because this behavior is intentional and necessary.

`git blame` claims that I wrote this comment 8 years ago, but that's
news to me!

Thanks for the thorough review. I'm not planning a re-roll since it
seems like this is fine as-is, but let me know if y'all feel otherwise.

One thing I would maybe change would be to rename `dt_options` to
`options`, but I also appreciate the verbosity of the dt_ prefix.

Interestingly, the `struct difftool_state` and `dt_state` names in the
original patch were chosen because the struct contained more than just
options. Specifically, it contains the `has_symlinks` field.
I'm not really sure it's worth splitting hairs over that detail,
but I'm all ears. struct difftool_options doesn't really bother me.

cheers,
-- 
David