Fix curl version comparison

[sergiodj]

Thanks to @polkorny for identifying a bug in our curl version
comparison algorithm!

Previously, we'd have problems when curl bumps to version 9.0+ because
our logic would wrongly try to match the minor version to "-ge 16",
which wouldn't work until curl 9.16 is released.

Polkorny gave the idea to compose the major and minor versions into
one single number without the dot, which allows us to perform quick
numerical comparisons.

Signed-off-by: Sergio Durigan Junior [email protected]
Reported-by: Matheus Polkorny [email protected]
Co-Authored-by: Matheus Polkorny [email protected]

[sergiodj]

Cc @polkorny

[vszakats]

Here's a proposal that creates a '0123` (major-major-minor-minor) internally
zero-padded version number, resulting in shorter, easier to extend and IMO
easier to grok code. It also fixes handling <7 version inputs:

--- a/wcurl
+++ b/wcurl
@@ -205,28 +205,22 @@ exec_curl()
 
     # Store version to check if it supports --no-clobber, --parallel and --parallel-max-host.
     curl_version=$($CMD --version | cut -f2 -d' ' | head -n1)
-    curl_version_major=$(echo "$curl_version" | cut -f1 -d.)
-    curl_version_minor=$(echo "$curl_version" | cut -f2 -d.)
+    curl_version=$(printf '%02d%02d' "$(echo "$curl_version" | cut -f1 -d.)" "$(echo "$curl_version" | cut -f2 -d.)")
 
     CURL_NO_CLOBBER=""
     CURL_PARALLEL=""
-    # --no-clobber is only supported since 7.83.0.
-    # --parallel is only supported since 7.66.0.
-    # --parallel-max-host is only supported since 8.16.0.
-    if [ "${curl_version_major}" -ge 8 ]; then
-        CURL_NO_CLOBBER="--no-clobber"
+    if [ "${curl_version}" -ge 0766 ]; then
+        # --parallel is only supported since 7.66.0.
         CURL_PARALLEL="--parallel"
-        if [ "${curl_version_minor}" -ge 16 ]; then
-            CURL_PARALLEL="--parallel --parallel-max-host 5"
-        fi
-    elif [ "${curl_version_major}" -eq 7 ]; then
-        if [ "${curl_version_minor}" -ge 83 ]; then
-            CURL_NO_CLOBBER="--no-clobber"
-        fi
-        if [ "${curl_version_minor}" -ge 66 ]; then
-            CURL_PARALLEL="--parallel"
+        if [ "${curl_version}" -ge 0816 ]; then
+            # --parallel-max-host is only supported since 8.16.0.
+            CURL_PARALLEL="${CURL_PARALLEL} --parallel-max-host 5"
         fi
     fi
+    if [ "${curl_version}" -ge 0783 ]; then
+        # --no-clobber is only supported since 7.83.0.
+        CURL_NO_CLOBBER="--no-clobber"
+    fi
 
     # Detecting whether we need --parallel. It's easier to rely on
     # the shell's argument parsing.

What do you think?

[samueloph]

What do you think?

We actually considered this before raising the PR, I don't remember there being strong reasons for not using this approach, which was also noted by @sergiodj as being a common pattern in C.

If I remember correctly, the reasons were that this adds a constraint on how many digits the curl version can have before the comparison breaks (the printf decimal places) and that the comparisons end up harder to read if we want to support more than 2 decimal places to be on the safe side.

[vszakats]

Thanks for reviewing @samueloph!

The 2-digit is the limitation built into this, indeed, but the reason for bumping from 7.88 to 8 was
to keep the minor number "manageable" [1] and not to reach 100 [2]. This tells 3-digit is unlikely
to happen. To prepare for that anyway (the hex representation in LIBCURL_VERSION_NUM
allows going up to 255), we may pad to 3 digits [3], and keep the advantages.

[1] https://daniel.haxx.se/blog/2023/03/20/curl-8-0-0-is-here/
[2] https://www.youtube.com/watch?v=9zff4hWOxPE&t=2159
[3]

--- a/wcurl
+++ b/wcurl
@@ -205,19 +205,19 @@ exec_curl()
 
     # Store version to check if it supports --no-clobber, --parallel and --parallel-max-host.
     curl_version=$($CMD --version | cut -f2 -d' ' | head -n1)
-    curl_version=$(printf '%02d%02d' "$(echo "$curl_version" | cut -f1 -d.)" "$(echo "$curl_version" | cut -f2 -d.)")
+    curl_version=$(printf '%03d%03d' "$(echo "$curl_version" | cut -f1 -d.)" "$(echo "$curl_version" | cut -f2 -d.)")
 
     CURL_NO_CLOBBER=""
     CURL_PARALLEL=""
-    if [ "${curl_version}" -ge 0766 ]; then
+    if [ "${curl_version}" -ge 007066 ]; then
         # --parallel is only supported since 7.66.0.
         CURL_PARALLEL="--parallel"
-        if [ "${curl_version}" -ge 0816 ]; then
+        if [ "${curl_version}" -ge 008016 ]; then
             # --parallel-max-host is only supported since 8.16.0.
             CURL_PARALLEL="${CURL_PARALLEL} --parallel-max-host 5"
         fi
     fi
-    if [ "${curl_version}" -ge 0783 ]; then
+    if [ "${curl_version}" -ge 007083 ]; then
         # --no-clobber is only supported since 7.83.0.
         CURL_NO_CLOBBER="--no-clobber"
     fi

[sergiodj]

What do you think?

We actually considered this before raising the PR, I don't remember there being strong reasons for not using this approach, which was also noted by @sergiodj as being a common pattern in C.

Yeah, the reason we chose not to use this pattern is because at the time we wanted to get the fix out ASAP, and IIRC there was some discussion about the readability of such versions.

I actually have a patch that uses this pattern but implements things a bit differently. I'll post a PR soon.

[sergiodj]

@samueloph @vszakats I'll go ahead and merge this PR just to make sure we have the problem fixed in master. We can continue the discussion in the PR I'll file later if needed.