autotools: tidy-up if expressions

[vszakats]

• drop x-hacks for curl internal variables and certain autotools ones
  that do not hold custom values.
• make x-hacks consistently use "x$var" = "xval" style.
• add a few x-hacks for input/external variables that may hold custom
  values.
• prefer -z and -n to test empty/non-empty.
  This also makes some x-hacks unnecessary.
• optimized negated test -z and -n options.
• prefer && and || over -a and -o.
  For better POSIX compatibility:
  https://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html
• quote variables passed to test, where missing.
• quote string literals in comparisons.
• fix some indentation, whitespace.

Note that a few case statements also use the x-hack, which looks
unnecessary. This patch does not change them.

Verified by comparing feature detection results with a reference CI run
from before this patch (PR #19922).

Refs:
https://www.shellcheck.net/wiki/SC2268
https://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html
https://www.vidarholen.net/contents/blog/?p=1035
https://mywiki.wooledge.org/BashPitfalls#A.5B_.24foo_.3D_.22bar.22_.5D

---

https://github.com/curl/curl/pull/18189/files?w=1

• merge tests where possible. → [NO, -a and -o are POSIX extension, deprecated and have limitations]
• tidy up these ifs to use a common style? (how to quote the literal value)
• drop test -a and -o in favor of multiple tests?
• convert comparisons with an empty string to test -n and test -z.
  Both already used in configure scripts.

Originally meant to delete the x-hack from ifs, subsequently toned
down to a tidy-up, and keeping them for values coming from the outside,
also adding them in a few places, not to make shell script Gods angry.
Though, its use had already been applied fairly accidentally. Some
places still miss them, and by most accounts the issues have been fixed
either very long ago, or years ago in remaining niche cases. Also to hit
it, a custom value needs to be intentionally skewed, meaning it seems
unlikely to happen with well-formed inputs. curl keeps rolling the x,
except for a few places that seemed internal. Some of them may be
possible to override externally, but with IMO an even lower risk.

[dfandrich]

IIUC this is to support very old UNIX systems whose shells don't handle empty strings correctly. curl's configure seems to use the x style of test fairly consistently (outside nettle detection) from what I see, so this will likely have an impact on those systems. I have no idea if any such system is still a viable curl target (e.g. it has 64-bit int support, etc.) and therefore whether this could cause configure failure there.

[vszakats]

I wonder if it was used consistently? The impression I got was that it was accidental, at best incomplete. The internet says this hack was necessary for systems where a - initial char in values could confuse very old, non-POSIX shells. Could such value appear in yes/no/1/0 style variables? Most of the x chars were used in such places. For directories or other freeform values an x could be kept, but this is just a small fragment of uses. A path or filename starting with dash also seems like a very rare corner case. And it may break other commands besides test.

edit: historical reason for the x-hack explained via this shellcheck comment:
"Very old shells were confused when the first argument began with "-" as well as when it was simply "!" or "("."
koalaman/shellcheck#2689 (comment)

[dfandrich]

What's the reason for removing them?

[vszakats]

What's the reason for removing them?

shellcheck now warns about this, and this hack has been deleted from curl-config a couple of days ago.

Aside from that, they make the source code difficult to read, grep or edit. The simplest boolean comparisons are living in the code in dozens of slightly different variations. Without a particular reason or consistency. Mixed with modern/normal syntax, which would have failed on such very old system anyway.

edit: more info:
https://www.shellcheck.net/wiki/SC2268
https://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html
https://www.vidarholen.net/contents/blog/?p=1035
https://mywiki.wooledge.org/BashPitfalls#A.5B_.24foo_.3D_.22bar.22_.5D

Excerpt:

Bash 1.14 from 1992 incorrectly fails this test. This was fixed for Bash 2.0 in 1996:
Dash 0.5.4 from 2007 incorrectly passes this test. This was fixed for Dash 0.5.5 in 2008:
Zsh (while not supported by ShellCheck) fixed the same problem in 2015.

[bagder]

I removed them from curl-config (because they made red CI jobs). But I think shellcheck shouldn't complain about them because they are not errors.

[vszakats]

They do add friction to maintaining autotools scripts. I agree with shellcheck flagging it.

It seems unlikely that an ancient/old shell combined with rare/crafted input would pass
the autotools code anyway. For this the x-hack would need to be applied consistently.

Which would be another option to improve this, but that'd assume those ancient systems
meet all documented curl requirements, and all the other accidental ones. My impression
(with builds) is that once something isn't tested, it decays and breaks relatively quickly.

Is there a known env or system that requires this? Has there been a report about a missing
x-hack?

[dfandrich]

Has there been a report about a missing x-hack?

We'll find out once this is submitted!

[vszakats]

The ./configure generated by recent autotools also has plenty of normal ifs.
Very roughly 10% of the 4000+ in total has an x-hack. It'd be interesting to see
what an older supported automake would generate. And how conscious the use
is. On a quick glance the pattern looks fairly accidental.

[vszakats]

Detailed write-up, with systems and versions and case descriptions:
https://www.vidarholen.net/contents/blog/?p=1035

[vszakats]

Reworked this to keep the x-hacks for cases where the value is coming
from the outside (env, withval, enableval, etc).