threads / patch / 29924

v4, 5 partshttp: rename HTTP_REAUTH to HTTP_RETRY

Subject: [PATCH v4 5/5] http: rename HTTP_REAUTH to HTTP_RETRY

## tl;dr

8 messages between Mar 12, 2012 and Mar 14, 2012. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Nelson Benitez Leon· Mar 12, 2012, 17:30 UTC · lore

After adding the proxy authentication support in http, the semantics of HTTP_REAUTH changed more to a retry rather than a re-authentication, so we rename it to HTTP_RETRY.

Signed-off-by: Nelson Benitez Leon <nbenitezl@gmail.com>
---
 http.c |    6 +++---
 http.h |    2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)
Show changes to 2 files +4 −4

http.c, http.h

diff --git a/http.c b/http.c
index 9b98179..4aa5a46 100644
--- a/http.c
+++ b/http.c
@@ -823,7 +823,7 @@ static int http_request(const char *url, void *result, int target, int options)
 			} else {
 				credential_fill(&http_auth);
 				init_curl_http_auth(slot->curl);
-				ret = HTTP_REAUTH;
+				ret = HTTP_RETRY;
 			}
 		} else if (results.http_code == 407) { /* Proxy authentication failure */
 			if (proxy_auth.username && proxy_auth.password) {
@@ -832,7 +832,7 @@ static int http_request(const char *url, void *result, int target, int options)
 			} else {
 				credential_fill(&proxy_auth);
 				set_proxy_auth(slot->curl);
-				ret = HTTP_REAUTH;
+				ret = HTTP_RETRY;
 			}
 		} else {
 			if (!curl_errorstr[0])
@@ -862,7 +862,7 @@ static int http_request_reauth(const char *url, void *result, int target,
 
 	do {
 		ret = http_request(url, result, target, options);
-	} while (ret == HTTP_REAUTH);
+	} while (ret == HTTP_RETRY);
 
 	return ret;
 }
diff --git a/http.h b/http.h
index 0b61653..6499397 100644
--- a/http.h
+++ b/http.h
@@ -123,7 +123,7 @@ extern char *get_remote_object_url(const char *url, const char *hex,
 #define HTTP_MISSING_TARGET	1
 #define HTTP_ERROR		2
 #define HTTP_START_FAILED	3
-#define HTTP_REAUTH	4
+#define HTTP_RETRY	4
 #define HTTP_NOAUTH	5
 
 /*

-- 
1.7.7.6
Junio C Hamano· Mar 12, 2012, 20:06 UTC · re: Nelson Benitez Leon · lore

Re: [PATCH v4 5/5] http: rename HTTP_REAUTH to HTTP_RETRY

Whatever new token you use, please keep AUTH as a substring of it.

We may want to retry a request to deal with intermittent failures on the server side or the network between us and the server; HTTP_RETRY would be a good name to signal such condition after we see a failure response from the library.

Nelson Benitez Leon· Mar 13, 2012, 12:47 UTC · re: Junio C Hamano · lore

Re: [PATCH v4 5/5] http: rename HTTP_REAUTH to HTTP_RETRY

On 03/12/2012 09:06 PM, Junio C Hamano wrote:
Show 6 quoted lines
> Whatever new token you use, please keep AUTH as a substring of it.
> 
> We may want to retry a request to deal with intermittent failures on
> the server side or the network between us and the server; HTTP_RETRY
> would be a good name to signal such condition after we see a failure
> response from the library.

HTTP_REAUTH and HTTP_AUTH_RETRY seems like the same thing, so imo not deserving the rename, maybe Jeff can suggest a better name as he was who suggest the rename.

Junio C Hamano· Mar 13, 2012, 17:51 UTC · re: Nelson Benitez Leon · lore

Re: [PATCH v4 5/5] http: rename HTTP_REAUTH to HTTP_RETRY

Nelson Benitez Leon <nelsonjesus.benitez@seap.minhap.es> writes:
Show 11 quoted lines
> On 03/12/2012 09:06 PM, Junio C Hamano wrote:
>> Whatever new token you use, please keep AUTH as a substring of it.
>> 
>> We may want to retry a request to deal with intermittent failures on
>> the server side or the network between us and the server; HTTP_RETRY
>> would be a good name to signal such condition after we see a failure
>> response from the library.
>
> HTTP_REAUTH and HTTP_AUTH_RETRY seems like the same thing, so imo not 
> deserving the rename, maybe Jeff can suggest a better name as he was
> who suggest the rename.

Either has AUTH as a substring in it, and leaves a door open for us to later introduce HTTP_RETRY to tell the machinery that drives cURL library to retry the request, so in that sense I am OK with either, but as your log message said, we want to make it clear that this is not about doing the authentication again (re-auth) but retrying the authentication, so HTTP_AUTH_RETRY would be more logical name.

Jeff King· Mar 13, 2012, 22:04 UTC · re: Junio C Hamano · lore

Re: [PATCH v4 5/5] http: rename HTTP_REAUTH to HTTP_RETRY

On Tue, Mar 13, 2012 at 10:51:33AM -0700, Junio C Hamano wrote:
Show 18 quoted lines
> > On 03/12/2012 09:06 PM, Junio C Hamano wrote:
> >> Whatever new token you use, please keep AUTH as a substring of it.
> >> 
> >> We may want to retry a request to deal with intermittent failures on
> >> the server side or the network between us and the server; HTTP_RETRY
> >> would be a good name to signal such condition after we see a failure
> >> response from the library.
> >
> > HTTP_REAUTH and HTTP_AUTH_RETRY seems like the same thing, so imo not 
> > deserving the rename, maybe Jeff can suggest a better name as he was
> > who suggest the rename.
> 
> Either has AUTH as a substring in it, and leaves a door open for us to
> later introduce HTTP_RETRY to tell the machinery that drives cURL library
> to retry the request, so in that sense I am OK with either, but as your
> log message said, we want to make it clear that this is not about doing
> the authentication again (re-auth) but retrying the authentication, so
> HTTP_AUTH_RETRY would be more logical name.

I suggested RETRY because that is all the caller needs to know: the http_request machinery said "please call me again". Keep in mind that this is a private interface within http.c, and this return code should never make it out at all. Nor is it something anybody else would feed us.

I am half-tempted to suggest refactoring it to return the actual error code, and let the caller handle 401 and 407. That would be more readable overall, I think. But it's a little complicated, because getting the exact answer depends on the curl handle, which is local to http_request, and I don't want to hold Nelson's actual feature improvement hostage to such refactoring.

-Peff
Junio C Hamano· Mar 13, 2012, 23:05 UTC · re: Jeff King · lore

Re: [PATCH v4 5/5] http: rename HTTP_REAUTH to HTTP_RETRY

Jeff King <peff@peff.net> writes:
Show 14 quoted lines
> On Tue, Mar 13, 2012 at 10:51:33AM -0700, Junio C Hamano wrote:
>
>> Either has AUTH as a substring in it, and leaves a door open for us to
>> later introduce HTTP_RETRY to tell the machinery that drives cURL library
>> to retry the request, so in that sense I am OK with either, but as your
>> log message said, we want to make it clear that this is not about doing
>> the authentication again (re-auth) but retrying the authentication, so
>> HTTP_AUTH_RETRY would be more logical name.
>
> I suggested RETRY because that is all the caller needs to know: the
> http_request machinery said "please call me again". Keep in mind that
> this is a private interface within http.c, and this return code should
> never make it out at all. Nor is it something anybody else would feed
> us.

Oh, the potential "retry when a request failed" in the future I had in mind was also contained within http.c. Perhaps HTTP_RETRY could be used for the same purpose? The places I had in mind that we may potentially want to retry are where we got 50x from one of the servers in the pool that serves the name we are accessing, we got 401 from the server to let us realize we gave it a wrong credential, or we got 407 from the proxy to notify a similar situation, and all are potential candidate for retrying in the client may help. The credential might have been mistyped for 40x, or we may hit a healthy server in the same pool for 50x.

Nelson Benitez Leon· Mar 14, 2012, 11:11 UTC · re: Junio C Hamano · lore

Re: [PATCH v4 5/5] http: rename HTTP_REAUTH to HTTP_RETRY

After adding the proxy authentication support in http, the semantics of HTTP_REAUTH changed more to a retry rather than a re-authentication, so we rename it to HTTP_AUTH_RETRY.

Signed-off-by: Nelson Benitez Leon <nbenitezl@gmail.com>
---
Ok this is a new 5/5 patch that have HTTP_AUTH_RETRY as
Junio suggested, is responding with this patch good or
do I need to send a new re-roll just for this?
thanks, 
 http.c |    6 +++---
 http.h |    2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)
Show changes to 2 files +4 −4

http.c, http.h

diff --git a/http.c b/http.c
index 12dcaa1..7468cdb 100644
--- a/http.c
+++ b/http.c
@@ -837,7 +837,7 @@ static int http_request(const char *url, void *result, int target, int options)
 			} else {
 				credential_fill(&http_auth);
 				init_curl_http_auth(slot->curl);
-				ret = HTTP_REAUTH;
+				ret = HTTP_AUTH_RETRY;
 			}
 		} else if (results.http_code == 407) { /* Proxy authentication failure */
 			if (proxy_auth.username && proxy_auth.password) {
@@ -846,7 +846,7 @@ static int http_request(const char *url, void *result, int target, int options)
 			} else {
 				credential_fill(&proxy_auth);
 				set_proxy_auth(slot->curl);
-				ret = HTTP_REAUTH;
+				ret = HTTP_AUTH_RETRY;
 			}
 		} else {
 			if (!curl_errorstr[0])
@@ -876,7 +876,7 @@ static int http_request_reauth(const char *url, void *result, int target,
 
 	do {
 		ret = http_request(url, result, target, options);
-	} while (ret == HTTP_REAUTH);
+	} while (ret == HTTP_AUTH_RETRY);
 
 	return ret;
 }
diff --git a/http.h b/http.h
index 303eafb..6e3ea59 100644
--- a/http.h
+++ b/http.h
@@ -123,7 +123,7 @@ extern char *get_remote_object_url(const char *url, const char *hex,
 #define HTTP_MISSING_TARGET	1
 #define HTTP_ERROR		2
 #define HTTP_START_FAILED	3
-#define HTTP_REAUTH	4
+#define HTTP_AUTH_RETRY	4
 #define HTTP_NOAUTH	5
 
 /*

-- 
1.7.7.6
Junio C Hamano· Mar 14, 2012, 18:19 UTC · re: Nelson Benitez Leon · lore

Re: [PATCH v4 5/5] http: rename HTTP_REAUTH to HTTP_RETRY

Nelson Benitez Leon <nelsonjesus.benitez@seap.minhap.es> writes:
Show 10 quoted lines
> After adding the proxy authentication support in
> http, the semantics of HTTP_REAUTH changed more to
> a retry rather than a re-authentication, so we
> rename it to HTTP_AUTH_RETRY.
>
> Signed-off-by: Nelson Benitez Leon <nbenitezl@gmail.com>
> ---
> Ok this is a new 5/5 patch that have HTTP_AUTH_RETRY as
> Junio suggested, is responding with this patch good or
> do I need to send a new re-roll just for this?

Heh, HTTP_AUTH_RETRY was not something I suggested ;-) The name comes from your http://mid.gmane.org/4F5F41FF.4000204@seap.minhap.es message.

Regarding whether to re-send everything or only a selected subset, please follow your best judgement, like you did this time.

In general, when you know that the participants in the discussion are keeping closer eyes on the progress of the series, and they are likely to understand what you mean when you say "I am replacing the last one in the v3 series I sent earlier with this patch" when you send "[PATCH v4 5/5]", it is appropriate to send only the updated one(s).

It makes only two small differences if I am or I am not among the participants in the discussion.

 - When I happen to be involved in a topic and keeping closer eyes on it,
   an earlier iteration of it is likely to appear on 'pu', so you have one
   more clue to tell if it is OK to send just an update, compared to a
   series that is discussed only on the list without anybody tracking the
   most recent state of the series.
 - When I am not involved in a discussion, often I am sitting on the
   sideline (a recent example is the topic around svn-fe/fast-import
   regarding "ls" command on an empty path), letting the stakeholders in
   the series figure out the details and waiting for the final outcome of
   the discussion [*1*].  For such a topic, I may request a full resend of
   the final version when it is time for me to queue it.

Thanks. I replaced the corresponding patch with this, after fixing the subject line.

[Footnote]

*1* This happens when I have more confidence in them than in myself to judge the best direction for the series.

← back to recent threads