Re: [PATCH 1/2] http: allow selection of proxy authentication method

On 2015-11-02 14:46, Junio C Hamano wrote:

quoted

Reviewed-by: Junio C Hamano <redacted>
Reviewed-by: Eric Sunshine <redacted>

Please add these only when you are doing the final submission,
sending the same version reviewed by these people after they said
the patch(es) look good.  To credit others for helping you to polish
your patch, Helped-by: would be more appropriate.

Sorry about that.

However, may I suggest that Documentation/SubmittingPatches could do with a
little rewording in this respect?

Do not forget to add trailers such as "Acked-by:", "Reviewed-by:" and
"Tested-by:" lines as necessary to credit people who helped your
patch.

"Helped-by:" isn't even mentioned.

quoted

+static void init_curl_proxy_auth(CURL *result)
+{
+	env_override(&http_proxy_authmethod, "GIT_HTTP_PROXY_AUTHMETHOD");

Shouldn't this also be part of the #if/#endif?

The idea here was to have as little code as possible within the #if/#endif, as a
matter of principle. It may be a little construed in this case, but supposing
there's some subtle bug with env_override, or a future change introduces one,
having it occur only for certain CURL versions would tend to make it harder to
track down.

and this code would be:

	if (remote)
		var_override(&http_proxy_authmethod, remote->http_proxy_authmethod);

Good catch.


Cheers,
Knut
-- 
Vorstandsvorsitzender/Chairman of the board of management:
Gerd-Lothar Leonhart
Vorstand/Board of Management:
Dr. Bernd Finkbeiner, Dr. Arno Steitz
Vorsitzender des Aufsichtsrats/
Chairman of the Supervisory Board:
Philippe Miltin
Sitz/Registered Office: Tuebingen
Registergericht/Registration Court: Stuttgart
Registernummer/Commercial Register No.: HRB 382196