On 2023-01-13 10:10, Victoria Dye wrote:
> Matthew John Cheetham via GitGitGadget wrote:
>> +static int read_auth_config(const char *name, const char *val, void *data)
>> +{
>> + int ret = 0;
>> + char *scheme = NULL;
>> + char *token = NULL;
>> + char *challenge = NULL;
>> + struct auth_module *mod = NULL;
>> +
>> + if (!strcmp(name, "auth.challenge")) {
>> + if (split_auth_param(val, &scheme, &challenge, 0)) {
>> + ret = error("invalid auth challenge '%s'", val);
>> + goto cleanup;
>> + }
>> +
>> + mod = create_auth_module(scheme, challenge);
>> + if (add_auth_module(mod)) {
>> + ret = error("failed to add auth module '%s'", val);
>> + goto cleanup;
>> + }
>> + }
>> + if (!strcmp(name, "auth.token")) {
>> + if (split_auth_param(val, &scheme, &token, 1)) {
>> + ret = error("invalid auth token '%s'", val);
>> + goto cleanup;
>> + }
>> +
>> + mod = get_auth_module(scheme);
>> + if (!mod) {
>> + ret = error("auth scheme not defined '%s'\n", scheme);
>> + goto cleanup;
>> + }
>> +
>> + string_list_append(mod->tokens, token);
>> + }
>
> I don't think this addresses the implicit option ordering requirement noted
> in [3]; instead of needing '--auth' before '--auth-token', this now needs
> 'auth.challenge' before 'auth.token' in the config file. While I'd prefer it
> if this could be rearranged so that the auth setup happens after all config
> parsing (so the order doesn't matter), if you want to leave it as-is please
> add a comment somewhere in this file explaining that requirement and/or add
> a note to the "auth scheme not defined" error message.
>
> [3] https://lore.kernel.org/git/2a5d6586-3d2c-8af4-12be-a5a106f966b5@xxxxxxxxxx/
>
>> + if (!strcmp(name, "auth.allowanonymous")) {
>> + allow_anonymous = git_config_bool(name, val);
>> + }
>> + if (!strcmp(name, "auth.extraheader")) {
>> + strvec_push(&extra_headers, val);
>> + }
>
> Is it worth printing a warning if the option found isn't any of the above?
> Something like "ignoring <config option>". This is a test helper, so
> user-friendliness isn't quite as important as it is for builtins, but the
> warning might be helpful to developers trying to use it in the future.
I tried this suggestion of adding a warning, but it felt wrong. You are correct
in the first instance that it should really "just work" when specified in any
order. Watch for the next iteration where I'll make it such you can specify them
in any order :-)
Thanks,
Matthew
