Date: Sun, 22 Jul 2018 10:38:57 -0700
From: Noah Misch <noah@leadboat.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Victor Wagner <vitus@wagner.pp.ru>,
	pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: Non-portable shell code in pg_upgrade tap tests
Message-ID: <20180722173857.GA3998719@rfd.leadboat.com>
References: <20180720153820.69e9ae6c@fafnir.local.vm>
 <17833.1532096747@sss.pgh.pa.us>
 <20180720175508.07776219@fafnir.local.vm>
 <20015.1532099353@sss.pgh.pa.us>
 <20180722072904.GA3951499@rfd.leadboat.com>
 <6519.1532270763@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <6519.1532270763@sss.pgh.pa.us>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk

On Sun, Jul 22, 2018 at 10:46:03AM -0400, Tom Lane wrote:
> Noah Misch <noah@leadboat.com> writes:
> > I'd say the right way to fix this is the one specified in
> > https://www.gnu.org/software/autoconf/manual/autoconf-2.69/html_node/The-Make-Macro-SHELL.html,
> > in particular:
> 
> >   Using @SHELL@ means that your makefile will benefit from the same improved
> >   shell, such as bash or ksh, that was discovered during configure, so that
> >   you aren't fighting two different sets of shell bugs between the two
> >   contexts.
> 
> The pg_upgrade makefile does in fact use $(SHELL), so it will default to
> whatever shell configure used.

It will not, because we don't set $(SHELL) anywhere.  $(SHELL) is not @SHELL@.
In our makefiles, $(SHELL) is always /bin/sh, the GNU make default.

> The bigger picture here is that it's worth maintaining a common coding
> style in this script.  Randomly using `...` in some places and $(...)
> in others just increases the reader's cognitive load, as does spelling
> conditional blocks in multiple styles.  So I'd have felt these changes
> were appropriate on style grounds even if there were no portability
> complaint.

For 986127e narrowly, I agree.  However, I do want folks able to use $(...)
where it saves backslashes; that's more valuable than `...` uniformity.  For
commit 0426932, as the comment above the changed code indicates, the
conditional blocks are copies of "configure" code.  Consistency with their
source was more valuable than style, particularly since the code is
security-critical.