On Thu, Sep 5, 2024 at 9:36 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> "Shubham Kanodia via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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.
Fair. I kept the initial value as `unset` as that could be interpreted
as a special case to do something else in the future — but I agree that keeping
it initialized to default keeps things clearer for now since such a
case doesn't arise.
Updating my patch — please let me know if there's anything else I can
improve here.
