Junio C Hamano <gitster@xxxxxxxxx> writes:
> "Glen Choo via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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.
