Re: [PATCH v2] remote: prefetch config

[Junio C Hamano <gitster@pobox.com>]

"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.