Re: [PATCH 1/3] builtin: add a repository parameter for builtin functions

From: Patrick Steinhardt <ps@pks.im>
Date: Sep 6, 2024, 10:46 UTC
Message-ID: <ZtrdljxBJtnaUEla@pks.im>
In-Reply-To: <3301a34f76303c43feaf4eb9d6913fbeec439e97.1725555468.git.gitgitgadget@gmail.com>

On Thu, Sep 05, 2024 at 04:57:45PM +0000, John Cai via GitGitGadget wrote:

Show 14 quoted lines

> From: John Cai <johncai86@gmail.com>
> diff --git a/builtin/add.c b/builtin/add.c
> index 40b61ef90d9..3b9bc93ed9a 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -358,7 +358,7 @@ static int add_files(struct dir_struct *dir, int flags)
>  	return exit_status;
>  }
>  
> -int cmd_add(int argc, const char **argv, const char *prefix)
> +int cmd_add(int argc, const char **argv, const char *prefix, struct repository *repository UNUSED)
>  {
>  	int exit_status = 0;
>  	struct pathspec pathspec;

Nit: all of these are now overly long as we typically wrap at 80
characters.

Show 37 quoted lines

> diff --git a/git.c b/git.c
> index 9a618a2740f..0ea6e351dfd 100644
> --- a/git.c
> +++ b/git.c
> @@ -31,7 +31,7 @@
>  
>  struct cmd_struct {
>  	const char *cmd;
> -	int (*fn)(int, const char **, const char *);
> +	int (*fn)(int, const char **, const char *, struct repository *);
>  	unsigned int option;
>  };
>  
> @@ -441,7 +441,7 @@ static int handle_alias(int *argcp, const char ***argv)
>  	return ret;
>  }
>  
> -static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
> +static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct repository *repo)
>  {
>  	int status, help;
>  	struct stat st;
> @@ -479,9 +479,11 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
>  	trace_argv_printf(argv, "trace: built-in: git");
>  	trace2_cmd_name(p->cmd);
>  
> -	validate_cache_entries(the_repository->index);
> -	status = p->fn(argc, argv, prefix);
> -	validate_cache_entries(the_repository->index);
> +	validate_cache_entries(repo->index);
> +
> +	status = p->fn(argc, argv, prefix, startup_info->have_repository? repo: NULL) ;
> +
> +	validate_cache_entries(repo->index);
>  
>  	if (status)
>  		return status;

Looks sensible.

Show 9 quoted lines

> @@ -736,7 +738,7 @@ static void handle_builtin(int argc, const char **argv)
>  
>  	builtin = get_builtin(cmd);
>  	if (builtin)
> -		exit(run_builtin(builtin, argc, argv));
> +		exit(run_builtin(builtin, argc, argv, the_repository));
>  	strvec_clear(&args);
>  }
>

Why don't we need a check for `startup_info->have_repository` here?

Show 12 quoted lines

> diff --git a/help.c b/help.c
> index c03863f2265..e7cdfab6432 100644
> --- a/help.c
> +++ b/help.c
> @@ -16,6 +16,7 @@
>  #include "parse-options.h"
>  #include "prompt.h"
>  #include "fsmonitor-ipc.h"
> +#include "repository.h"
>  
>  #ifndef NO_CURL
>  #include "git-curl-compat.h" /* For LIBCURL_VERSION only */

The include shouldn't be necessary. You can instead add a forward declaration.

Show 9 quoted lines

> @@ -775,7 +776,7 @@ void get_version_info(struct strbuf *buf, int show_build_options)
>  	}
>  }
>  
> -int cmd_version(int argc, const char **argv, const char *prefix)
> +int cmd_version(int argc, const char **argv, const char *prefix, struct repository *repository UNUSED)
>  {
>  	struct strbuf buf = STRBUF_INIT;
>  	int build_options = 0;

Patrick