Re: [PATCH v3 1/3] t/lib-httpd: dynamically detect httpd and modules path

From: Patrick Steinhardt <ps@pks.im>
Date: Nov 9, 2023, 07:36 UTC
Message-ID: <ZUyMFZ7c9_rlu5lk@tanuki>
In-Reply-To: <20231109073250.GA2698227@coredump.intra.peff.net>

On Thu, Nov 09, 2023 at 02:32:50AM -0500, Jeff King wrote:

Show 18 quoted lines

> On Thu, Nov 09, 2023 at 08:09:52AM +0100, Patrick Steinhardt wrote:
> 
> > -for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' '/usr/sbin/apache2'
> > +for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' \
> > +			  '/usr/sbin/apache2' \
> > +			  "$(command -v httpd)" \
> > +			  "$(command -v apache2)"
> >  do
> > -	if test -x "$DEFAULT_HTTPD_PATH"
> > +	if test -n "$DEFAULT_HTTPD_PATH" -a -x "$DEFAULT_HTTPD_PATH"
> 
> Sorry to be a pedant, but I'm not sure if we might have portability
> problems with "-a". It's an XSI extension, and POSIX labels it as
> obsolescent because it can create parsing ambiguities.
> 
> We do have a few instances, but only in corners of the test suite that
> probably don't get as much exposure (t/perf and valgrind/valgrind.sh).
> So maybe not worth worrying about, but it's easy to write it as:

Yeah, I was grepping for it in our codebase and saw other occurrences, so I assumed it was fair game. If we're going to convert it to the below, how about I send another patch on top that also converts the preexisting instances so that the next one grepping for it isn't going to repeat the same mistake?

Patrick

>   if test -n "$DEFAULT_HTTPD_PATH" && test -x "$DEFAULT_HTTPD_PATH"
> 
> -Peff