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

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

[Nelson Benitez Leon]

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(-)

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

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

[Junio C Hamano]

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.

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

[Nelson Benitez Leon]

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.

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

[Junio C Hamano]

Nelson Benitez Leon <nelsonjesus.benitez@seap.minhap.es> writes:

> 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.

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

[Jeff King]

On Tue, Mar 13, 2012 at 10:51:33AM -0700, Junio C Hamano wrote:

> > 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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

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

[Nelson Benitez Leon]

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(-)

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

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

[Junio C Hamano]

Nelson Benitez Leon <nelsonjesus.benitez@seap.minhap.es> writes:

> 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.