Subject: Re: [PATCH v4 3/5] config: read protected config with `git_protected_config()`

From: Junio C Hamano <[email-masked]>
Date: Tue, 07 Jun 2022 15:49:26 -0700

"Glen Choo via GitGitGadget" <[email-masked]> writes:

> diff --git a/upload-pack.c b/upload-pack.c
> index 3a851b36066..09f48317b02 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -1321,18 +1321,27 @@ static int upload_pack_config(const char *var, const char *value, void *cb_data)
>  		data->advertise_sid = git_config_bool(var, value);
>  	}
>  
> -	if (current_config_scope() != CONFIG_SCOPE_LOCAL &&
> -	    current_config_scope() != CONFIG_SCOPE_WORKTREE) {
> -		if (!strcmp("uploadpack.packobjectshook", var))
> -			return git_config_string(&data->pack_objects_hook, var, value);
> -	}
> -

The lossage of this block is because this general git_config()
callback routine that is used to read from any scope is no longer
used to pick up the sensitive variable.  Instead, we need to get it
with a different API, namely, git_protected_config().

It is probably is good that in the new code we are not encouraging
folks to write random comparisons on current_config_scope(), and
instead uniformly use a git_protected_config().  That may promote
consistency.

An obvious alternative to achieve the same consistency would be to
introduce a helper, and rewrite (instead of removing) the above part
like so:

	if (in_protected_scope()) {
		... parse sensitive variable ...
	}

We would not need any other change to this file in this patch if we
go that route, I suspect.

>  	if (parse_object_filter_config(var, value, data) < 0)
>  		return -1;
>  
>  	return parse_hide_refs_config(var, value, "uploadpack");
>  }
>  
> +static int upload_pack_protected_config(const char *var, const char *value, void *cb_data)
> +{
> +	struct upload_pack_data *data = cb_data;
> +
> +	if (!strcmp("uploadpack.packobjectshook", var))
> +		return git_config_string(&data->pack_objects_hook, var, value);
> +	return 0;
> +}
> +
> +static void get_upload_pack_config(struct upload_pack_data *data)
> +{
> +	git_config(upload_pack_config, data);
> +	git_protected_config(upload_pack_protected_config, data);
> +}

Where we used to just do git_config(upload_pack_config), we now need
to do a separate git_protected_config().  It feels a bit wasteful to
iterate over the same configset twice, but it is not like we are
doing the IO and text file parsing multiple times.  This looks quite
straight-forward.