From: Junio C Hamano <gitster@pobox.com>
To: Simon.Richter@hogyros.de
Cc: git@vger.kernel.org
Subject: Re: [PATCH 1/3] Rename proxy_authmethods -> authmethods
Date: Fri, 13 May 2022 12:50:31 -0700 [thread overview]
Message-ID: <xmqq7d6p9pq0.fsf@gitster.g> (raw)
In-Reply-To: <20220513070416.37235-2-Simon.Richter@hogyros.de> (Simon Richter's message of "Fri, 13 May 2022 09:04:14 +0200")
Simon.Richter@hogyros.de writes:
> From: Simon Richter <Simon.Richter@hogyros.de>
>
> Curl also allows specifying a list of acceptable auth methods for the
> request itself, so this isn't specific to proxy authentication.
While that is true, given that it is ONLY used to sanity check the
http_proxy_authmethod variable and use CURLOPT_PROXYAUTH thing, the
above alone is not a good excuse to rename this array.
I haven't read the later patches, but I would imagine that this is
so that you'd create another consumer that is about authentication
method that is not for the proxyauth. And if that is the case, the
proposed log message for this change should explicitly say so to
justify this change.
We are about to reuse this table of authmethods to parse the
non-proxy authentication in a later step in this series. Let's
rename it to just "authmethod[]".
appended as the second paragraph after the above might be sufficient.
Two things that comes to mind:
* In general, an array whose elements are accessed individually is
better named in singular, i.e. "type thing[]", not "type
things[]" (an exception is when the most prevalent use of the
array is to pass it as a whole to functions as a bag of things,
instead of accessing individual element). This is because it is
more natural to see the zeroth thing to be spelled "thing[0]",
and not "things[0]". If we are renaming this array anyway, it
may make sense to rename it to authmethod[].
* If the reason why this rename is warranted is because there will
be another user of this table that maps a string name to its
corresponding CURLAUTH_* constant, it would probably make sense
to extract a helper function out of this loop to do just that,
something along the lines of ...
static int parse_authmethod(const char *name, long *auth_param)
{
int i;
for (i = 0; i < ARRAY_SIZE(authmethod); i++)
if (!strcmp(name, authmethod[i].name)) {
*auth_param = authmethod[i].curlauth_param;
return i;
}
return -1;
}
Then the existing code can become
if (http_proxy_authmethod) {
long auth_param;
if (parse_authmethod(http_proxy_authmethod, &auth_param) < 0) {
warning("unsupported ... %s: using anyauth", http_proxy_authmethod);
auth_param = CURLAUTH_ANY;
}
curl_easy_setopt(result, CURLOPT_PROXYAUTH, auth_param);
}
It is probably OK to do so in the same patch as renaming of the
table, but the focus of the step will then become "factor out
parsing of authmethod from string to CURLAUTH_* constants" and
the patch should be retitled accordingly. Then you do not have
to justify the rename of the table based on the future plan.
> Subject: Re: [PATCH 1/3] Rename proxy_authmethods -> authmethods
The title of a patch in this project follows certain convention.
cf. Documentation/SubmittingPatches.
Subject: [PATCH 1/n] http: factor out parsing of authmethod
In order to support CURLOPT_PROXYAUTH, there is a code to parse
the name of an authentication method given as a string into one
of the CURLAUTH_* constant. The next step of this series wants
to reuse the same parser to support CURLOPT_HTTPAUTH in a
similar way.
Factor out the loop into a separate helper function. Since the
table of authentication methods no longer is only for proxy
authentication, drop "proxy" prefix from its name while we are
at it.
or something like that, perhaps.
> Signed-off-by: Simon Richter <Simon.Richter@hogyros.de>
> ---
> http.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/http.c b/http.c
> index 229da4d148..318dc5daea 100644
> --- a/http.c
> +++ b/http.c
> @@ -79,7 +79,7 @@ static int proxy_ssl_cert_password_required;
> static struct {
> const char *name;
> long curlauth_param;
> -} proxy_authmethods[] = {
> +} authmethods[] = {
> { "basic", CURLAUTH_BASIC },
> { "digest", CURLAUTH_DIGEST },
> { "negotiate", CURLAUTH_GSSNEGOTIATE },
> @@ -470,14 +470,14 @@ static void init_curl_proxy_auth(CURL *result)
>
> if (http_proxy_authmethod) {
> int i;
> - for (i = 0; i < ARRAY_SIZE(proxy_authmethods); i++) {
> - if (!strcmp(http_proxy_authmethod, proxy_authmethods[i].name)) {
> + for (i = 0; i < ARRAY_SIZE(authmethods); i++) {
> + if (!strcmp(http_proxy_authmethod, authmethods[i].name)) {
> curl_easy_setopt(result, CURLOPT_PROXYAUTH,
> - proxy_authmethods[i].curlauth_param);
> + authmethods[i].curlauth_param);
> break;
> }
> }
> - if (i == ARRAY_SIZE(proxy_authmethods)) {
> + if (i == ARRAY_SIZE(authmethods)) {
> warning("unsupported proxy authentication method %s: using anyauth",
> http_proxy_authmethod);
> curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
next prev parent reply other threads:[~2022-05-13 19:52 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-13 7:04 [PATCH 0/3] Allow configuration of HTTP authentication method Simon.Richter
2022-05-13 7:04 ` [PATCH 1/3] Rename proxy_authmethods -> authmethods Simon.Richter
2022-05-13 19:50 ` Junio C Hamano [this message]
2022-05-13 7:04 ` [PATCH 2/3] Add config option/env var to limit HTTP auth methods Simon.Richter
2022-05-13 20:26 ` Junio C Hamano
2022-05-13 7:04 ` [RFC PATCH 3/3] Allow empty user name in HTTP authentication Simon.Richter
2022-05-13 23:51 ` brian m. carlson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqq7d6p9pq0.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=Simon.Richter@hogyros.de \
--cc=git@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).