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

[Knut Franke <k.franke@science-computing.de>]

On 2015-11-02 14:46, Junio C Hamano wrote:
> > Reviewed-by: Junio C Hamano <gitster@pobox.com>
> > Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
> 
> 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.

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