Re: [PATCH] Add unix domain socket support to HTTP transport.

From: Leslie Cheng <leslie.cheng5@gmail.com>
Date: Feb 22, 2024, 03:04 UTC
Message-ID: <5a857e76-659d-4d94-ae54-f5c22c9917c6@gmail.com>
In-Reply-To: <20240221220945.M686016@dcvr>

> No need for trailing `.' in commit message titles

Will fix in the next patch, sorry!

Show 5 quoted lines

> Personally, I'd hoist the #ifdef part into a standalone function
> since I find mixing CPP and C conditionals confusing.
> 
> disclaimer: I'm an easily confused person and don't usually
> program in C, though.

I considered extracting it out, but the other conditionals in this function follow a similar pattern so I didn't want to change it. However, my use here is also the first time there's both an #ifdef and nested #ifndef, which I agree makes it a bit confusing to grok.

I'm open to changing it, but I'll let it sit and marinate for a bit.

Show 12 quoted lines

> `nc' isn't widely installed, its supported flags vary between
> implementations, and our test suite doesn't currently use it.
> I suggest either using a small bit of Perl or writing a t/helper
> program to do its job.
> 
> Finally, hard tabs should be used for indentation throughout.
> 
> I'll wait on others to comment since I haven't looked at git
> hacking in a while.
> 
> Anyways, I think this feature could be useful for me, too :>
> Thanks.

Good catch, I'll fix in the next patch. I've subbed `nc` out for a simple Perl script to pipe back and forth, just making sure CI is happy about this before submitting.