Re: [PATCH] difftool: eliminate use of global variables

[Patrick Steinhardt <ps@pks.im>]

On Tue, Feb 04, 2025 at 02:55:00PM -0800, David Aguilar wrote:
> Remove the 'USE_THE_REPOSITORY_VARIABLE' macro now that all state is
> passed to each function from callers.

I think this patch deserves to be split up into multiple patches, as
you're doing multiple things at once:

  - You remove global state located in "builtin/difftool.c", which is
    itself not required to drop `USE_THE_REPOSITORY_VARIABLE`.

  - You introduce `struct difftool_state` to encapsulate state.

  - You replace callsites for interfaces that depend on
    `the_repository`.

All of these changes are great from my point of view, but it's a bit
hard to follow. It doesn't necessarily have to be three commits, but it
would be great to split up the refactorings that introduce the struct
and the refactorings that adapt users of `the_repository`.

> diff --git a/builtin/difftool.c b/builtin/difftool.c
> index 03a8bb92a9..cd1e5882e3 100644
> --- a/builtin/difftool.c
> +++ b/builtin/difftool.c
> @@ -12,8 +12,6 @@
>   * Copyright (C) 2016 Johannes Schindelin
>   */
>  
> -#define USE_THE_REPOSITORY_VARIABLE
> -
>  #include "builtin.h"
>  
>  #include "abspath.h"
> @@ -36,18 +34,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_state {
> +	int has_symlinks;
> +	int symlinks;
> +	int trust_exit_code;
> +};

Why do we have both `has_symlinks` and `symlinks`? The latter gets set
to `has_symlinks` anyway, so it's a confusing to have both.

Also, I think it would make sense to rename the struct to
`difftool_options`, as it tracks options rather than state.

> @@ -734,8 +750,9 @@ int cmd_difftool(int argc,
>  	};
>  	struct child_process child = CHILD_PROCESS_INIT;
>  
> -	git_config(difftool_config, NULL);
> -	symlinks = has_symlinks;
> +	if (repo)
> +		repo_config(repo, difftool_config, &dt_state);
> +	dt_state.symlinks = dt_state.has_symlinks;
>  
>  	argc = parse_options(argc, argv, prefix, builtin_difftool_options,
>  			     builtin_difftool_usage, PARSE_OPT_KEEP_UNKNOWN_OPT |

Okay, you're being careful in the case where we don't have a repository,
good.

> @@ -749,8 +766,8 @@ int cmd_difftool(int argc,
>  
>  	if (!no_index){
>  		setup_work_tree();
> -		setenv(GIT_DIR_ENVIRONMENT, absolute_path(repo_get_git_dir(the_repository)), 1);
> -		setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(repo_get_work_tree(the_repository)), 1);
> +		setenv(GIT_DIR_ENVIRONMENT, absolute_path(repo_get_git_dir(repo)), 1);
> +		setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(repo_get_work_tree(repo)), 1);

Okay. Here you unconditionall deref the repository, but we would have
exited already if we had no repo with `!!no_index`.

> @@ -799,6 +816,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(repo, &dt_state, extcmd, prefix, &child);

This one is a bit curious. We never assert that we have a repo when
`dir_diff` is set. But we make `no_index` and `dir_diff` exclusive with
one another, and thus we know that we'll have called `setup_work_tree()`
at an earlier point, which causes us to die in case there is none.

Thanks for working on this!

Patrick