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

From: John Cai <johncai86@gmail.com>
Date: Sep 9, 2024, 21:08 UTC
Message-ID: <CAOCgCUJWrOeYK1ugVVe3wdj23+KgBWtMD0XH0EZHc2rZgRGVtg@mail.gmail.com>
In-Reply-To: <ZtrdljxBJtnaUEla@pks.im>

Hi Patrick,

On Fri, Sep 6, 2024 at 6:46 AM Patrick Steinhardt <ps@pks.im> wrote:

Show 71 quoted lines

>
> On Thu, Sep 05, 2024 at 04:57:45PM +0000, John Cai via GitGitGadget wrote:
> > 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.
>
> > 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().

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

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

Show 12 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