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

[Glen Choo <chooglen@google.com>]

Junio C Hamano <gitster@pobox.com> writes:

> "Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> 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.

Yes, and as noted in the commit message, this approach seems to work for
`safe.directory` and `discovery.bare` too.

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

Yeah it's not optimal, but at the very least, I think it's easy enough
to understand that we could replace it with something more economical in
the future.