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

From: Patrick Steinhardt <ps@pks.im>
Date: Feb 5, 2025, 07:30 UTC
Message-ID: <Z6MThg8oEEtx5xur@pks.im>
In-Reply-To: <20250204225501.597873-1-davvid@gmail.com>

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

Show 29 quoted lines

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

Show 12 quoted lines

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

Show 8 quoted lines

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

Show 6 quoted lines

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