Re: [PATCH v2 0/5] Avoid hang if curl needs eof twice + minor related improvements

From: Jonathan Tan <jonathantanmy@google.com>
Date: Nov 15, 2023, 23:28 UTC
Message-ID: <20231115232824.2507648-1-jonathantanmy@google.com>
In-Reply-To: <CAGE_+C4mUw8U6YK0m6hRvcqriv4pWdsELpyRJBCY-LrdHjWwgw@mail.gmail.com>

Jiří Hruška <jirka@fud.cz> writes:

> Proposed changes split into several commits for clarity
> 
> Jiri Hruska (5):
>   remote-curl: avoid hang if curl asks for more data after eof

I've already reviewed this [1] so I'll summarize what I think of the rest.

[1] https://lore.kernel.org/git/20231115192027.2468887-1-jonathantanmy@google.com/

>   remote-curl: improve readability of curl callbacks
>   remote-curl: simplify rpc_out() - remove superfluous ifs
>   remote-curl: simplify rpc_out() - less nesting and rename
>   http: reset CURLOPT_POSTFIELDSIZE_LARGE between requests

Overall I can see how all of these make the code clearer, but in a long-lived project like Git where it is very common to look at code history to try to see why something was written the way it is, I'm a bit reluctant to include 2/5 and 4/5. I think 3/5 (removes an "if"), the part of 4/5 where we set "rpc.pos = 0;", and 5/5 (sets a parameter that one could expect to be set) have significant benefit and should be included, though. Having said that, I don't have strong opposition to including all of them.