On 2024-02-23 12:37 a.m., Junio C Hamano wrote: > How about following that convention, perhaps like: > > In some corporate environments, the proxy server listens to a > local unix domain socket for requests, instead of listening to a > network port. Even though we have http.proxy (and more > destination specific http.<url>.proxy) configuration variables > to specify the network address/port of a proxy, that would not > help if your proxy does not listen to the network. > > Introduce an `http.unixSocket` (and `http.<url>.unixSocket`) > configuration variables that specify the path to a unix domain > socket for such a proxy. Recent versions of libcURL library > added CURLOPT_UNIX_SOCKET_PATH to support "curl --unix-socket > <path>"---use the same mechanism to implement it. This is excellent, thanks for the guidance (and all the other suggestions prior)! I'll update in the next patch. > Unlike NO_UNIX_SOCKETS, GIT_CURL_HAVE_CURLOPT_UNIX_SOCKET_PATH is > entirely internal to your implementation and not surfaced to neither > the end-users or the binary packagers. Because of that, I suspect > that any description that has to use that name probably falls on the > other side of "too much implementation details" to be useful to help > future developers.. That's reasonable, I figured it would fit as a cover letter detail but I agree it's not relevant as a commit message that lives in the history of the project. I'll also update this in the next patch. > Talking about precedence between this and http.proxy is good thing, > but one very important piece of information is missing. What value > does it take? > > The absolute path of a unix-domain socket to pass the HTTP > traffic over, instead of using the network. > > or something, perhaps? I like that wording, I'll update in the next patch. > It might make the code easier to follow if you did: > > #if !defined(NO_CURLOPT_UNIX_SOCKET_PATH) && !defined(NO_UNIX_SOCKETS) > #if defined(GIT_CURL_HAVE_CURLOPT_UNIX_SOCKET_PATH) > #define USE_CURLOPT_UNIX_SOCKET_PATH > #endif > #endif > > The points are > > (1) the users can decline to use CURLOPT_UNIX_SOCKET_PATH while > still using unix domain socket for other purposes, and > > (2) you do not have to care if you HAVE it or not most of time; > what matters more often is if the user told you to USE it. > > Hmm? Do you think this functionality is worth adding another macro to conditionally include it in the build? It felt out-of-the-way enough that we could just use the same `NO_UNIX_SOCKETS` macro to control for environments that don't support unix domain sockets. >> +#if defined(GIT_CURL_HAVE_CURLOPT_UNIX_SOCKET_PATH) && !defined(NO_UNIX_SOCKETS) >> +static const char *curl_unix_socket_path; >> +#endif > > The guard here would become "#ifdef USE_CURLOPT_UNIX_SOCKET_PATH" if > we wanted this to be conditional, but I think it is easier to make > the variable unconditionally available; see below. Agreed in general, I was looking to other patterns for conditional variables in file, e.g. https://github.com/git/git/blob/3c2a3fdc388747b9eaf4a4a4f2035c1c9ddb26d0/http.c#L66-L68 But, revisiting, this looks like an exception rather than the norm. > In general, it is inadvisable to issue a warning in the codepath > that parses configuration variables, as the values we read may not > be necessarily used. We could instead accept the given path into a > variable unconditionally, and complain only before it gets used, > near the call to curl_easy_setopt(). Similar to above, I followed what was already done for certain configuration variables (e.g. https://github.com/git/git/blob/3c2a3fdc388747b9eaf4a4a4f2035c1c9ddb26d0/http.c#L485-L501), but I agree with your feedback since this would result in constant warnings. To summarize, I'll do the following in the next patch: - change the wording of the commit message - move the conditional variables and parsing to a check at `curl_easy_setopt()` time I'm still undecided on whether I should introduce another macro specifically for this functionality, and I'd like to hear your thoughts on why it might make sense.
