Re: [PATCH v2] remote: prefetch config

>
> "Shubham Kanodia via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > diff --git a/builtin/gc.c b/builtin/gc.c
> > index 427faf1cfe1..2ca3a3e7d6a 100644
> > --- a/builtin/gc.c
> > +++ b/builtin/gc.c
> > @@ -1027,6 +1027,9 @@ static int fetch_remote(struct remote *remote, void *cbdata)
> >       if (remote->skip_default_update)
> >               return 0;
> >
> > +     if (!remote->prefetch)
> > +             return 0;
>
> This, while better than ane xplicit comparison with "== 0", is a bit
> tricky in this patch, as it is not saying "if we are told to prefetch,
> fall through to the rest of the function".  It is saying "leave if
> and only if we are explicitly configured not to prefetch".
>
> It might warrant a comment.
>
> > diff --git a/remote.c b/remote.c
> > index 8f3dee13186..05edb3a5f40 100644
> > --- a/remote.c
> > +++ b/remote.c
> > @@ -140,6 +140,7 @@ static struct remote *make_remote(struct remote_state *remote_state,
> >       CALLOC_ARRAY(ret, 1);
> >       ret->prune = -1;  /* unspecified */
> >       ret->prune_tags = -1;  /* unspecified */
> > +     ret->prefetch = -1;  /* unspecified */
>
> Or, we can just assign "1" (and drop "unspecified" comment).
>
>         ret->prefetch = 1; /* enabled by default */
>
> If I understand it correctly, we want this to default to true...
>
> >       ret->name = xstrndup(name, len);
> >       refspec_init(&ret->push, REFSPEC_PUSH);
> >       refspec_init(&ret->fetch, REFSPEC_FETCH);
> > @@ -456,6 +457,8 @@ static int handle_config(const char *key, const char *value,
> >               remote->prune = git_config_bool(key, value);
> >       else if (!strcmp(subkey, "prunetags"))
> >               remote->prune_tags = git_config_bool(key, value);
> > +     else if (!strcmp(subkey, "prefetch"))
> > +             remote->prefetch = git_config_bool(key, value);
>
> ... with a way for the user to turn it off.
>
> > diff --git a/remote.h b/remote.h
> > index b901b56746d..4522fdec354 100644
> > --- a/remote.h
> > +++ b/remote.h
> > @@ -77,6 +77,15 @@ struct remote {
> >
> >       struct refspec fetch;
> >
> > +     /*
> > +      * The setting for whether to prefetch from a remote
> > +      * when a fetch is invoked with a prefetch flag.
> > +      *  -1 = unset
> > +      *   0 = don't prefetch from this remote
> > +      *   1 = prefetch from this remote
> > +      */
> > +     int prefetch;
>
> And then we can get rid of "-1 unset" from this list.  The comment
> can become a lot more brief, as such a change would make it a simple
> Boolean flag that everybody would understand immediately.
>
> "prefetch" in the comment is superfluous, as that is the name of the
> member anyway.  "from this remote" is superfluous, as that is the
> point of having the member in "struct remote" that gives settings
> that are per-remote.
>
>         int prefetch; /* is prefetch enabled? */
>
> If we really want to have "unspecified yet" state, what we commonly
> do is
>
>  * to initialize the variable to -1 to signal "unspecified yet",
>    which you did in this patch.
>
>  * after the configuration reader returns, check if the variable is
>    still -1, and then explicitly reset it to the default value,
>    which your patch does not do.
>
>  * the code that uses the variable assumes it is either 0 or 1 and
>    there shoudl be no "unspecified yet" value.  It indeed is a bug
>    that the ariable is left unspecified as it is a sign that the
>    code to do previous step was somehow skipped.
>
> But I do not think it is needed in this case; initializing the
> .prefetch member to whichever is the default should be sufficient.
>
> Thanks.