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