diff --git a/include/param_op.h b/include/param_op.h index 22e8353c7..8b55f54f9 100644 --- a/include/param_op.h +++ b/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, diff --git a/meson.build b/meson.build index 85f6aabd3..47ca188b3 100644 --- a/meson.build +++ b/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']) diff --git a/src/executor.c b/src/executor.c index 44c27cbc7..ffb7b7cd1 100644 --- a/src/executor.c +++ b/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) { diff --git a/src/param_op.c b/src/param_op.c index 06962dbee..88ff766e5 100644 --- a/src/param_op.c +++ b/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); diff --git a/tests/integration/test_vector_substitution_spec.c b/tests/integration/test_vector_substitution_spec.c new file mode 100644 index 000000000..62225d9ce --- /dev/null +++ b/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 + */ + +#include +#include +#include +#include +#include +#include +#include + +#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 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 \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[@]//\\/}\"", ""); + check(lush, "684 the joined form deletes them too", + "arr=(a/b/c); printf '<%s>' \"${arr[*]//\\/}\"", ""); + check(lush, "684 positionals delete them too", + "set -- a/b/c; printf '<%s>' \"${@//\\/}\"", ""); + check(lush, "684 every element is transformed", + "arr=(a/b x/y); printf '<%s>' \"${arr[*]//\\/}\"", ""); + + /// 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; +}