Re: [GSoC PATCH v1] builtin: stop using the_repository

>
> Abraham Samuel Adekunle <abrahamadekunle50@gmail.com> writes:
>
> > The builtins use the_repository global variable which might
> > not work well when running many repos in the same process at once.
>
> This is true, but ...
>
> > Stop using the_repository in these builtins to align with the goal of
> > libification of Git.
>
> ... in general, each file under builtin/ is about a single command
> that _uses_ libified part of Git.  So it is perfectly fine for the
> libification goal to include "libified functions should not assume
> that it works on the_repository, but they should accept a repo
> parameter to tell them which repository to work with".  But it is
> not necessary, and I would say it is harmful, to subject builtin/*.c
> to the same criteria.  The builtin command implementations can call
> libified function by passing the_repository to libified API function
> that expects a repo parameter.

>
> > Signed-off-by: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
> > ---
> >  builtin/bugreport.c    | 13 ++++++-------
> >  builtin/bundle.c       | 13 ++++++-------
> >  builtin/check-attr.c   | 26 +++++++++++++-------------
> >  builtin/check-ignore.c | 27 +++++++++++++++------------
> >  4 files changed, 40 insertions(+), 39 deletions(-)
> >
> > diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> > index f78c3f2aed..77eb8bd9c1 100644
> > --- a/builtin/bugreport.c
> > +++ b/builtin/bugreport.c
> > @@ -1,4 +1,3 @@
> > -#define USE_THE_REPOSITORY_VARIABLE
> >  #include "builtin.h"
> >  #include "abspath.h"
> >  #include "editor.h"
> > @@ -37,7 +36,7 @@ static void get_system_info(struct strbuf *sys_info)
> >                   shell ? shell : "<unset>");
> >  }
> >
> > -static void get_populated_hooks(struct strbuf *hook_info, int nongit)
> > +static void get_populated_hooks(struct repository *repo, struct strbuf *hook_info, int nongit)
> >  {
> >       const char **p;
> >
> > @@ -50,7 +49,7 @@ static void get_populated_hooks(struct strbuf *hook_info, int nongit)
> >       for (p = hook_name_list; *p; p++) {
> >               const char *hook = *p;
> >
> > -             if (hook_exists(the_repository, hook))
> > +             if (hook_exists(repo, hook))
> >                       strbuf_addf(hook_info, "%s\n", hook);
> >       }
> >  }
>
> It is not strictly necessary to churn a file-scope static function
> like this one into taking an arbitrary repo parameter, as the only
> caller of the function, presumably cmd_foo() in the builtin/foo.c
> file, would pass the_repository anyway, whether it explicitly names
> the_repository or passes the repo parameter that it got from its
> caller, git.c:run_builtin().  We _can_ consider a change like the
> above as a preparation to potentially move these functions to the
> libified part of Git, so even though I said it is not necessary, it
> is also OK to perform such a change.

>
> > @@ -93,7 +92,7 @@ static void get_header(struct strbuf *buf, const char *title)
> >  int cmd_bugreport(int argc,
> >                 const char **argv,
> >                 const char *prefix,
> > -               struct repository *repo UNUSED)
> > +               struct repository *repo)
> >  {
> >       struct strbuf buffer = STRBUF_INIT;
> >       struct strbuf report_path = STRBUF_INIT;
> > @@ -141,7 +140,7 @@ int cmd_bugreport(int argc,
> >       }
> >       strbuf_addstr(&report_path, ".txt");
> >
> > -     switch (safe_create_leading_directories(the_repository, report_path.buf)) {
> > +     switch (safe_create_leading_directories(repo, report_path.buf)) {
> >       case SCLD_OK:
> >       case SCLD_EXISTS:
> >               break;
> > @@ -158,7 +157,7 @@ int cmd_bugreport(int argc,
> >               strbuf_addftime(&zip_path, option_suffix, localtime_r(&now, &tm), 0, 0);
> >               strbuf_addstr(&zip_path, ".zip");
> >
> > -             if (create_diagnostics_archive(the_repository, &zip_path, diagnose))
> > +             if (create_diagnostics_archive(repo, &zip_path, diagnose))
> >                       die_errno(_("unable to create diagnostics archive %s"), zip_path.buf);
> >
> >               strbuf_release(&zip_path);
> > @@ -171,7 +170,7 @@ int cmd_bugreport(int argc,
> >       get_system_info(&buffer);
> >
> >       get_header(&buffer, _("Enabled Hooks"));
> > -     get_populated_hooks(&buffer, !startup_info->have_repository);
> > +     get_populated_hooks(repo, &buffer, !startup_info->have_repository);
> >
> >       /* fopen doesn't offer us an O_EXCL alternative, except with glibc. */
> >       report = xopen(report_path.buf, O_CREAT | O_EXCL | O_WRONLY, 0666);
>
> All of the above look fine.
>
> > diff --git a/builtin/bundle.c b/builtin/bundle.c
> > index 1e170e9278..ef21ccfd89 100644
> > --- a/builtin/bundle.c
> > +++ b/builtin/bundle.c
>
> Is this patch meant as a microproject in preparation for applying
> for GSoC?  If so, we ask to limit one quality focused one per
> applicant.
>
> https://git.github.io/General-Microproject-Information/#only-one-quality-focused-microproject-per-applicant