Re: [PATCH v4 3/5] config: read protected config with `git_protected_config()`
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 7, 2022, 22:49 UTC
- Message-ID
- <xmqqbkv4t7gp.fsf@gitster.g>
- In-Reply-To
- <94b40907e66b2f6e0874ab49f8b73fdd58eb06d5.1654635432.git.gitgitgadget@gmail.com>
"Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 14 quoted lines
> 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.
Show 20 quoted lines
> 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.