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

[David Aguilar <davvid@gmail.com>]

On Thu, Feb 06, 2025 at 12:29:46AM -0800, Elijah Newren wrote:
> On Wed, Feb 5, 2025 at 8:20 PM David Aguilar <davvid@gmail.com> wrote:
> > 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.

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.


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