Re: [PATCH 3/4] apply: remove the_repository global variable

>
> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: John Cai <johncai86@gmail.com>
> >
> > Remove the_repository global variable in favor of the repository
> > argument that gets passed in through the builtin function.
> >
> > Signed-off-by: John Cai <johncai86@gmail.com>
> > ---
> >  builtin/apply.c | 9 ++++-----
> >  1 file changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/builtin/apply.c b/builtin/apply.c
> > index 84f1863d3ac..d0bafbec7e4 100644
> > --- a/builtin/apply.c
> > +++ b/builtin/apply.c
> > @@ -1,4 +1,3 @@
> > -#define USE_THE_REPOSITORY_VARIABLE
> >  #include "builtin.h"
> >  #include "gettext.h"
> >  #include "hash.h"
> > @@ -12,14 +11,14 @@ static const char * const apply_usage[] = {
> >  int cmd_apply(int argc,
> >             const char **argv,
> >             const char *prefix,
> > -           struct repository *repo UNUSED)
> > +           struct repository *repo)
> >  {
> >       int force_apply = 0;
> >       int options = 0;
> >       int ret;
> >       struct apply_state state;
> >
> > -     if (init_apply_state(&state, the_repository, prefix))
> > +     if (init_apply_state(&state, repo, prefix))
> >               exit(128);
>
> Hmph, the reason why we do not segfault with this patch is because
> repo will _always_ be the_repository due to the previous change.
>
> I am not sure if [1/4] is an improvement, though.  We used to be
> able to tell if we were running in a repository, or we were running
> in "nongit" mode, by looking at the NULL-ness of repo (which was
> UNUSED because we weren't taking advantage of that).
>
> With [1/4], it no longer is possible.  From the point of view of API
> to call into builtin implementations, it smells like a regression.

>
> A more honest change for this hunk would rather be something like:
>
>         -       if (init_apply_state(&state, the_repository, prefix))
>         +       if (!repo)
>         +               repo = the_repository;
>         +       if (init_apply_state(&state, repo, prefix))
>
> without [1/4].  This change does not address "apply still depends on
> having access to the_repository even when it is being used as a better
> GNU patch" issue at all.
>
> So, no, while I earlier said I was happy with [1/4], I no longer am
> enthused by the change.