param-op: one substitution-spec splitter, not two (#684)

include/param_op.h

@@ -149,6 +149,18 @@ char *lush_case_pattern(const char *str, const char *pattern, bool to_upper,
 /// substitution pattern may match beyond a single literal run.
 bool lush_pattern_opens_extglob_group(const char *pattern);
 
+/// Split a `pattern/replacement` substitution spec at its first UNESCAPED
+/// `/`, returning a malloc'd pattern with `\\/` canonicalized to `/` and
+/// pointing @p replacement at the bytes after the separator (the empty string
+/// when there is none, i.e. a delete).
+///
+/// Exported because the per-element vector dispatch in the executor needs the
+/// same split the scalar path gets. It carried its own copy, and the copy
+/// diverged: its no-separator branch skipped the `\\/` canonicalization, so
+/// `${arr[@]//\\/}` left the slashes that `${v//\\/}` removed (issue #684).
+char *lush_param_op_split_substitution_spec(const char *spec,
+                                            const char **replacement);
+
 /// Replace the first (global == false) or every occurrence of glob
 /// @p pattern in @p str with @p replacement. Honors the `#` / `%` anchors.
 char *lush_pattern_substitute(const char *str, const char *pattern,

meson.build

@@ -3235,6 +3235,16 @@ if fs.exists('tests/integration/test_array_assign_location.c')
        timeout: 120)
 endif
 
+if fs.exists('tests/integration/test_vector_substitution_spec.c')
+  test_vector_substitution_spec = executable('test_vector_substitution_spec',
+    ['tests/integration/test_vector_substitution_spec.c'])
+  test('element and scalar substitution split the spec identically',
+       test_vector_substitution_spec,
+       args: [lush_exe.full_path()],
+       suite: 'integration',
+       timeout: 120)
+endif
+
 if fs.exists('tests/integration/test_dangling_dollar.c')
   test_dangling_dollar = executable('test_dangling_dollar',
     ['tests/integration/test_dangling_dollar.c'])

src/executor.c

@@ -16039,53 +16039,20 @@ static char *parse_parameter_expansion(executor_t *executor,
                     case 15: /// /// replace all
                     case 16: /// /  replace first
                     {
-                        /// NOTE: this per-element copy of the spec split has
-                        /// NOT been folded onto split_substitution_spec in
-                        /// param_op.c, and it already diverges -- its
-                        /// no-separator branch skips the `\/` -> `/`
-                        /// canonicalization, so `${arr[@]//\/}` leaves the
-                        /// slashes the scalar `${v//\/}` removes. Tracked as
-                        /// #684; folding it is a behavior change and belongs
-                        /// in its own commit.
-                        /// Split expanded_default at first unescaped '/'.
-                        char *sep = NULL;
-                        for (char *p = expanded_default; p && *p; p++) {
-                            if (*p == '\\' && p[1] == '/') {
-                                p++;
-                                continue;
-                            }
-                            if (*p == '/') {
-                                sep = p;
-                                break;
-                            }
-                        }
-                        bool global = (op_type == 15);
-                        char *pattern = NULL;
+                        /// Split the `pattern/replacement` spec with the SAME
+                        /// function the scalar path uses. This arm carried its
+                        /// own copy, and the copy had drifted: its
+                        /// no-separator branch skipped the `\/` -> `/`
+                        /// canonicalization, so `${arr[@]//\/}` left the
+                        /// slashes that `${v//\/}` removes. One spec, one
+                        /// splitter -- they cannot diverge again (issue #684).
                         const char *replacement = "";
-                        if (sep) {
-                            size_t plen = (size_t)(sep - expanded_default);
-                            pattern = malloc(plen + 1);
-                            if (pattern) {
-                                size_t pj = 0;
-                                for (size_t pi = 0; pi < plen; pi++) {
-                                    if (expanded_default[pi] == '\\' &&
-                                        pi + 1 < plen &&
-                                        expanded_default[pi + 1] == '/') {
-                                        pattern[pj++] = '/';
-                                        pi++;
-                                    } else {
-                                        pattern[pj++] = expanded_default[pi];
-                                    }
-                                }
-                                pattern[pj] = '\0';
-                                replacement = sep + 1;
-                            }
-                        } else if (expanded_default) {
-                            pattern = strdup(expanded_default);
-                        }
+                        char *pattern = lush_param_op_split_substitution_spec(
+                            expanded_default ? expanded_default : "",
+                            &replacement);
                         if (pattern) {
                             converted = lush_pattern_substitute(
-                                elems[i], pattern, replacement, global);
+                                elems[i], pattern, replacement, op_type == 15);
                             free(pattern);
                         }
                         if (!converted) {

src/param_op.c

@@ -927,8 +927,8 @@ bool lush_param_op_is_pure(int op_type) {
 /// matcher, which handles them per the glob spec. Issue #96.
 /// Returns the owned pattern; *replacement points into @p spec (or "" when the
 /// spec carries no separator, i.e. delete the match).
-static char *split_substitution_spec(const char *spec,
-                                     const char **replacement) {
+char *lush_param_op_split_substitution_spec(const char *spec,
+                                            const char **replacement) {
     *replacement = "";
     const char *sep = NULL;
     for (const char *p = spec; *p; p++) {
@@ -1127,7 +1127,8 @@ char *lush_param_op_apply(int op_type, const char *var_value,
     case 16: /// ${var/pattern/replacement}  - replace the first occurrence
         if (var_value) {
             const char *replacement = "";
-            char *pattern = split_substitution_spec(deflt, &replacement);
+            char *pattern =
+                lush_param_op_split_substitution_spec(deflt, &replacement);
             if (pattern) {
                 result = lush_pattern_substitute(var_value, pattern,
                                                  replacement, op_type == 15);

tests/integration/test_vector_substitution_spec.c

@@ -0,0 +1,179 @@
+/**
+ * @file test_vector_substitution_spec.c
+ * @brief The element and scalar substitution paths split the spec identically.
+ *
+ * `${v/p/r}` and `${arr[@]/p/r}` take the same `pattern/replacement` spec, so
+ * they must split it the same way. The per-element dispatch carried its own
+ * copy of that split, and the copy had drifted: its no-separator branch
+ * skipped the `\/` -> `/` canonicalization, so
+ *
+ *     v=a/b/c; arr=(a/b/c)
+ *     ${v//\/}        -> abc
+ *     ${arr[@]//\/}   -> a/b/c     (the slashes survived)
+ *
+ * The element path now calls lush_param_op_split_substitution_spec, the same
+ * function the scalar path uses, so the two cannot diverge again (issue #684).
+ *
+ * These checks compare the scalar and element results against EACH OTHER as
+ * well as against a literal, because the contract is that they agree -- a
+ * future change that breaks both in the same way should still fail here.
+ *
+ * NOTE: `"${arr[@]}"` with an operator currently yields ONE field where bash
+ * and zsh yield N (issue #749, pre-existing and separate -- the element values
+ * are right, the field boundary is lost). These tests therefore compare
+ * CONTENT, and the field count is pinned in that issue rather than here.
+ *
+ * Usage: test_vector_substitution_spec <lush-binary-path>
+ */
+
+#include <fcntl.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/wait.h>
+#include <unistd.h>
+
+#define TEST "test_vector_substitution_spec"
+
+static bool run_c(const char *lush, const char *script, char *out,
+                  size_t out_sz) {
+    int pfd[2];
+    if (pipe(pfd) != 0) {
+        return false;
+    }
+    pid_t pid = fork();
+    if (pid < 0) {
+        close(pfd[0]);
+        close(pfd[1]);
+        return false;
+    }
+    if (pid == 0) {
+        dup2(pfd[1], STDOUT_FILENO);
+        dup2(pfd[1], STDERR_FILENO);
+        close(pfd[0]);
+        close(pfd[1]);
+        int devnull = open("/dev/null", O_RDONLY);
+        if (devnull >= 0) {
+            dup2(devnull, STDIN_FILENO);
+            close(devnull);
+        }
+        setenv("HOME", "/nonexistent", 1);
+        unsetenv("XDG_CONFIG_HOME");
+        unsetenv("ENV");
+        execl(lush, "lush", "-c", script, (char *)NULL);
+        _exit(127);
+    }
+    close(pfd[1]);
+    size_t len = 0;
+    ssize_t n;
+    while (len + 1 < out_sz &&
+           (n = read(pfd[0], out + len, out_sz - 1 - len)) > 0) {
+        len += (size_t)n;
+    }
+    out[len] = '\0';
+    close(pfd[0]);
+    waitpid(pid, NULL, 0);
+    return true;
+}
+
+static int failures = 0;
+
+/// `spec` is the operator text after the name, e.g. "//\\/" -- applied to a
+/// scalar and to a one-element array holding the same value. The two results
+/// are printed as <scalar><element> and must be identical AND equal to `want`.
+static void check_agree(const char *lush, const char *label, const char *value,
+                        const char *spec, const char *want) {
+    char script[1024];
+    char out[4096];
+    char expect[512];
+    snprintf(script, sizeof(script),
+             "v=%s; arr=(%s); printf '<%%s><%%s>' \"${v%s}\" \"${arr[0]%s}\"",
+             value, value, spec, spec);
+    if (!run_c(lush, script, out, sizeof(out))) {
+        fprintf(stderr, "FAIL %s [%s]: harness error\n", TEST, label);
+        failures++;
+        return;
+    }
+    snprintf(expect, sizeof(expect), "<%s><%s>", want, want);
+    if (strcmp(out, expect) != 0) {
+        fprintf(stderr, "FAIL %s [%s]: wanted \"%s\", got \"%.200s\"\n", TEST,
+                label, expect, out);
+        failures++;
+        return;
+    }
+    fprintf(stderr, "ok   %s [%s]\n", TEST, label);
+}
+
+static void check(const char *lush, const char *label, const char *script,
+                  const char *want) {
+    char out[4096];
+    if (!run_c(lush, script, out, sizeof(out))) {
+        fprintf(stderr, "FAIL %s [%s]: harness error\n", TEST, label);
+        failures++;
+        return;
+    }
+    if (strcmp(out, want) != 0) {
+        fprintf(stderr, "FAIL %s [%s]: wanted \"%s\", got \"%.200s\"\n", TEST,
+                label, want, out);
+        failures++;
+        return;
+    }
+    fprintf(stderr, "ok   %s [%s]\n", TEST, label);
+}
+
+int main(int argc, char **argv) {
+    if (argc < 2) {
+        fprintf(stderr, "usage: %s <lush-binary-path>\n", argv[0]);
+        return 2;
+    }
+    const char *lush = argv[1];
+
+    /// THE DISCRIMINATING CASE: a `\/` pattern with NO separator (a delete).
+    /// This is the branch the element copy had drifted on.
+    check_agree(lush, "684 escaped slash, delete", "a/b/c", "//\\/", "abc");
+    check_agree(lush, "684 escaped slash, replace-first", "a/b/c", "/\\//-",
+                "a-b/c");
+    check_agree(lush, "684 escaped slash, replace-all", "a/b/c", "//\\//-",
+                "a-b-c");
+
+    /// The rest of the family, to prove the shared splitter is used
+    /// everywhere and not just on the reported shape.
+    check_agree(lush, "684 plain delete-all", "aXbXc", "//X", "abc");
+    check_agree(lush, "684 plain replace-all", "aXbXc", "//X/-", "a-b-c");
+    check_agree(lush, "684 plain replace-first", "aXbXc", "/X/-", "a-bXc");
+    check_agree(lush, "684 empty replacement is a delete", "aXbXc", "//X/",
+                "abc");
+    check_agree(lush, "684 prefix anchor", "abc", "/#a/X", "Xbc");
+    check_agree(lush, "684 suffix anchor", "abc", "/%c/X", "abX");
+    check_agree(lush, "684 a bracket class pattern", "abc", "//[bc]/X", "aXX");
+    check_agree(lush, "684 a glob pattern", "abc", "/b*/X", "aX");
+    check_agree(lush, "684 no match leaves the value alone", "abc", "//z/X",
+                "abc");
+
+    /// The vector forms produce the same CONTENT as the scalar. Field count
+    /// is issue #749 and deliberately not asserted here.
+    check(lush, "684 the vector form deletes the slashes",
+          "arr=(a/b/c); printf '<%s>' \"${arr[@]//\\/}\"", "<abc>");
+    check(lush, "684 the joined form deletes them too",
+          "arr=(a/b/c); printf '<%s>' \"${arr[*]//\\/}\"", "<abc>");
+    check(lush, "684 positionals delete them too",
+          "set -- a/b/c; printf '<%s>' \"${@//\\/}\"", "<abc>");
+    check(lush, "684 every element is transformed",
+          "arr=(a/b x/y); printf '<%s>' \"${arr[*]//\\/}\"", "<ab xy>");
+
+    /// An escaped separator in the REPLACEMENT half: the two paths must give
+    /// the same answer, which is this fix's contract. They currently agree on
+    /// keeping the backslash, where bash and zsh drop it -- pre-existing on
+    /// BOTH paths and filed as #750, so the VALUE is pinned there rather than
+    /// asserted here.
+    check_agree(lush, "684 an escaped slash in the replacement agrees", "aXb",
+                "//X/\\/", "a\\/b");
+
+    if (failures) {
+        fprintf(stderr, "%s: %d failure(s)\n", TEST, failures);
+        return 1;
+    }
+    fprintf(stderr, "%s: all checks passed\n", TEST);
+    return 0;
+}