Re: [PATCH v2 1/3] difftool: eliminate use of global variables
- From
David Aguilar <davvid@gmail.com>
- Date
- Feb 7, 2025, 04:44 UTC
- Message-ID
- <Z6WPudTWInXagCYg@gmail.com>
- In-Reply-To
- <CABPp-BGnRgDxwgfagyvhwjso_kWVgcR-NxOUwUSJve5RHwyFZQ@mail.gmail.com>
On Thu, Feb 06, 2025 at 12:29:46AM -0800, Elijah Newren wrote:
Show 69 quoted lines
> 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.
Show 14 quoted lines
> > @@ -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