Hi Patrick,
On Fri, Sep 6, 2024 at 6:46 AM Patrick Steinhardt <ps@xxxxxx> wrote:
>
> On Thu, Sep 05, 2024 at 04:57:45PM +0000, John Cai via GitGitGadget wrote:
> > From: John Cai <johncai86@xxxxxxxxx>
> > 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.
>
> > 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.
>
> > @@ -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().
>
> > 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.
>
> > @@ -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
