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

Hi Patrick,

On Fri, Sep 6, 2024 at 6:46 AM Patrick Steinhardt [off-list ref] wrote:

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

quoted

From: John Cai <redacted>

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.

quoted

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.

quoted

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

We do the check inside of run_builtin(), which calls the fn() directly.
There's a call to validate_cache_entries(repo->index) in run_builtin(), so
if we passed in NULL then we would need another guard to prevent a segfault
in run_builtin().

quoted

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.

indeed, I'll remove this in the next version.

quoted

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