fix(#4112): portable grep in workflow markdown + submodule commit fix under diff.ignoreSubmodules (#4149)
* fix(#4112): ban GNU-only grep -P in workflow markdown to prevent macOS regressions Add scripts/lint-portable-grep.cjs, wire it into lint:ci, and add tests/lint-portable-grep.test.cjs. The #4112 shell-syntax fix newly exposed a pre-existing grep -oP invocation that silently resolves to "" on stock macOS's BSD grep (no -P support). This ratchet catches the same class of GNU-coreutils assumption before it merges, mirroring lint-portable-timeout.cjs. * fix(#4112): drop unneeded ls -l long-format that broke basename/dirname extraction The previous commit on this branch replaced grep -oP 'phases/\K[^/]+' (GNU-only, silently fails on macOS's BSD grep) with basename "$(dirname "$phase")"), but left ls -lt (long format, -l) in place. -l output is a full detail line (permissions, owner, size, date, path), not a bare path, so dirname on that string throws "illegal option -- r" (BSD) / errors under GNU coreutils too -- the extraction never produced a usable value, on any platform. -l was never needed here; only mtime-sort plus the bare path mattered. Dropping it to ls -t restores one-bare-path-per-line output, which basename/dirname actually requires. Confirmed on gsd-test's Linux bench: the #4112 regression test (tests/pause-work-context-detection.test.cjs) now passes phase, spike, and sketch resolution. Emitted-Drift-Ack-Growth: pause-work.md — portability fix (#4112): dropping grep -oP for a portable basename/dirname extraction is a few characters longer per line; no functional growth beyond the fix. Emitted-Drift-Ack-Growth: sync-skills.md — portability fix (#4112): replacing grep -oP '(?<=--from )\S+' with a portable sed -E capture-group equivalent (no PCRE lookbehind available) is a longer expression; growth is the direct cost of the fix, not new functionality. * fix(#3859): pin git commit itself against diff.ignoreSubmodules=all, not just the probe git 2.39.x (the exact version on the CI Linux bench) resolves a pathspec-scoped `git commit -- <path>` through the same diff.ignoreSubmodules-gated machinery as `git diff`, so a genuinely bumped submodule gitlink was silently dropped by the real commit even though the #3859 empty-diff probe correctly saw the change. The commit was misclassified as generic commit_failed because git's refusal text never says "nothing to commit". Prefix the pathspec-scoped commit invocation with -c diff.ignoreSubmodules=dirty, the same override the probe already carries, so the two can no longer disagree. Confirmed on git 2.50.1 (no-op) and git 2.39.5 via the actual gsd-tester-linux v1.8.0-node24 bench image (turns the silent refusal into a commit). * fix(#3859): carry the diff.ignoreSubmodules override via env, not argv The prior commit prepended `-c diff.ignoreSubmodules=dirty` to the real commit's argv, which shifted `commitArgs[0]` off `'commit'` and broke several pre-existing #3859 regression tests (tests/commit-files-pathspec.test.cjs) that assert on the raw argv captured at the execGit seam, e.g. `gitCalls.some((a) => a[0] === 'commit')`. Carry the same override via `GIT_CONFIG_COUNT` / `GIT_CONFIG_KEY_0` / `GIT_CONFIG_VALUE_0` env vars instead, which git honours identically and leaves argv untouched. Re-confirmed on the gsd-tester-linux v1.8.0-node24 bench (git 2.39.5): the submodule commit still succeeds. * test(#3859): pin the commitEnv GIT_CONFIG_* override to canScope directly The commitEnv/GIT_CONFIG_* override added in e935694fc/b3d37b929 was only exercised indirectly through pre-existing submodule integration tests. Add two dedicated regression tests that pin the canScope=false side of the scoping decision: a whole-index commit (no --files) and an --amend commit must both keep recording a bumped submodule gitlink under diff.ignoreSubmodules=all with no override applied, since neither shape carries a pathspec for that git internal check to consult. * fix(#4112): match grep invocations after then/do/else/elif shell keywords lint-portable-grep.cjs's GREP_INVOCATION_RE only anchored to line-start or right after `| & ; ( \` {`, so a grep call positioned right after `then`, `do`, `else`, or `elif` (e.g. `if x; then grep -oP '...'; fi`) was never flagged, letting the GNU-only grep -P defect this lint exists to catch reappear undetected in that shape. * fix(#3859): apply the diff.ignoreSubmodules override to every commit, not just scoped ones The commitEnv GIT_CONFIG_* override landed in e935694fc/b3d37b929 only when canScope was true, on the assumption that git 2.39.5's silent refusal of a bumped submodule gitlink under diff.ignoreSubmodules=all only affects a pathspec-limited `git commit -- <paths>`. Reproduced directly against the pinned CI tester image (ghcr.io/open-gsd/gsd-tester-linux:v1.8.0-node24, git 2.39.5): a bare whole-index `git commit -m ...` with no pathspec at all is refused identically when the only staged change is a submodule gitlink, since git's "nothing to commit" check is a real diff against HEAD that honours diff.ignoreSubmodules regardless of whether a pathspec narrows it. Apply the override unconditionally instead of gating it on canScope; it remains a confirmed no-op for --amend, which never hits this refusal at all. Caught by the new regression test added in 579ac9f8f, which failed against the real bench git version before this change. * fix(#3859): apply diff.ignoreSubmodules override to commit-to-subrepo and pr-subrepo cmdCommit already carries the GIT_CONFIG_* override that forces diff.ignoreSubmodules=dirty on the actual git commit call so a bumped submodule gitlink is not spuriously refused under git 2.39.5. The two sibling multi-repo commit paths, cmdCommitToSubrepo and cmdPrSubrepo, build the identical canScope-branched commit invocation but never carried the override, so the same refusal there surfaces as a generic commit_failed/error instead of a recorded gitlink. Apply the override unconditionally to both, matching the corrected cmdCommit shape. * fix(#3859): pin pr-subrepo's change detection against diff.ignoreSubmodules cmdPrSubrepo discovers what to commit via `git status --porcelain`, which (like the empty-diff probe fixed for cmdCommit) honors a local diff.ignoreSubmodules=all config. Under that config, a genuinely bumped submodule gitlink is invisible to the status scan, so changedFiles comes back empty and the function reports nothing_to_commit before ever reaching the commit call this same issue already fixed. Pin the status probe with --ignore-submodules=dirty, mirroring the flag cmdCommit's diff probe already uses, so a real gitlink bump is detected regardless of local config. Found while adding a regression test for the previous #3859 follow-up fix: the test failed not on the commit step but on this earlier detection step. * docs(#3859): update changeset to cover all three fixed commit sites * docs(#4112): backfill changeset PR number (pr:0 -> pr:4149) --------- Co-authored-by: sim <sim@local>
This commit is contained in:
@@ -2046,10 +2046,45 @@ function cmdCommit(cwd: string, message: string | undefined, files: string[] | u
|
||||
if (canScope) {
|
||||
commitArgs.push('--', ...stagedPaths);
|
||||
}
|
||||
// #3859 follow-up: on git 2.39.5 (confirmed on the CI Linux bench image,
|
||||
// ghcr.io/open-gsd/gsd-tester-linux:v1.8.0-node24; NOT reproducible on git
|
||||
// 2.50.1) `git commit` itself — not just `git diff` — consults
|
||||
// `diff.ignoreSubmodules` when deciding whether there is anything to
|
||||
// record. With a local `diff.ignoreSubmodules=all` and a submodule gitlink
|
||||
// genuinely bumped, that git version silently REFUSES the commit (prints a
|
||||
// `git status`-style "Changes to be committed" dump and exits 1, having
|
||||
// written nothing) even though the diff probe above (already pinned with
|
||||
// its own `--ignore-submodules=dirty`) correctly reported the change as
|
||||
// present. The result was misclassified as generic `commit_failed` because
|
||||
// git's refusal text does not contain "nothing to commit".
|
||||
// Originally scoped to `canScope` on the assumption that only a
|
||||
// PATHSPEC-LIMITED `git commit -- <paths>` exercises this git internal
|
||||
// path. That assumption was wrong: reproduced directly against the pinned
|
||||
// v1.8.0-node24 tester image, a bare WHOLE-INDEX `git commit -m ...` (no
|
||||
// pathspec at all) is refused identically when the only staged change is a
|
||||
// submodule gitlink and `diff.ignoreSubmodules=all` — git's "nothing to
|
||||
// commit" check is a real diff (HEAD vs. index) honouring
|
||||
// `diff.ignoreSubmodules` regardless of whether a pathspec narrows it.
|
||||
// `--amend` is the one shape confirmed NOT to hit this: it always
|
||||
// recreates the commit from the current index and never runs the
|
||||
// empty-diff refusal a plain `git commit` does, override or not. The
|
||||
// override is therefore applied unconditionally here (not gated on
|
||||
// `canScope`) — it is a documented no-op everywhere it is not needed
|
||||
// (dry-run, git 2.50.1, and `--amend` already behave this way with or
|
||||
// without it; see `#3859 follow-up (canScope gap)` regression tests).
|
||||
// The override rides in via `GIT_CONFIG_*` env vars rather than a `-c`
|
||||
// argv flag so `commitArgs[0]` stays `'commit'` — several #3859 regression
|
||||
// tests assert on the raw argv captured at the `execGit` seam (e.g.
|
||||
// `gitCalls.some((a) => a[0] === 'commit')`), and a leading `-c` would shift
|
||||
// every element and break that pinning. Same override the probe already
|
||||
// carries, so the two can never disagree again.
|
||||
const commitEnv: Record<string, string> = {
|
||||
GIT_CONFIG_COUNT: '1', GIT_CONFIG_KEY_0: 'diff.ignoreSubmodules', GIT_CONFIG_VALUE_0: 'dirty',
|
||||
};
|
||||
// #3886: `git commit` runs pre-commit hooks (husky/lint-staged routinely
|
||||
// idles ~4s on Windows before any task) — 10s is too tight, and a timeout
|
||||
// kill is NOT an ordinary failure. Same band as the push call below.
|
||||
const commitResult = execGit(commitArgs, { cwd, timeout: COMMIT_TIMEOUT_MS });
|
||||
const commitResult = execGit(commitArgs, { cwd, timeout: COMMIT_TIMEOUT_MS, env: commitEnv });
|
||||
if (commitResult.exitCode !== 0) {
|
||||
// #3886: a SIGTERM'd git commit is a timeout, not commit_failed — the
|
||||
// partial stderr it flushed (often incidental CRLF warnings) is noise,
|
||||
@@ -2213,7 +2248,12 @@ function cmdCommitToSubrepo(cwd: string, message: string | undefined, files: str
|
||||
const commitArgs = canScopeSub
|
||||
? ['commit', '-m', message as string, '--', ...stagedRelPaths]
|
||||
: ['commit', '-m', message as string];
|
||||
const commitResult = execGit(commitArgs, { cwd: repoCwd, timeout: COMMIT_TIMEOUT_MS });
|
||||
// #3859 follow-up fix as cmdCommit above (line ~2081) — git 2.39.5 needs
|
||||
// this override for pathspec-scoped AND whole-index commits alike.
|
||||
const commitEnvSub: Record<string, string> = {
|
||||
GIT_CONFIG_COUNT: '1', GIT_CONFIG_KEY_0: 'diff.ignoreSubmodules', GIT_CONFIG_VALUE_0: 'dirty',
|
||||
};
|
||||
const commitResult = execGit(commitArgs, { cwd: repoCwd, timeout: COMMIT_TIMEOUT_MS, env: commitEnvSub });
|
||||
if (commitResult.exitCode !== 0) {
|
||||
if (isSpawnTimeout(commitResult)) {
|
||||
// #3886 (subrepo counterpart): timeout ≠ error; surface the stale-lock
|
||||
@@ -2301,7 +2341,19 @@ function cmdPrSubrepo(
|
||||
|
||||
// 1. Collect changed files via porcelain status — explicit, never git add -A.
|
||||
// ?? (untracked) lines are excluded — only stage tracked modifications.
|
||||
const statusResult = execGit(['-c', 'core.quotePath=false', 'status', '--porcelain'], { cwd: repoCwd });
|
||||
// #3859 follow-up: `git status --porcelain` honors `diff.ignoreSubmodules`
|
||||
// the same way the empty-diff probe fixed for cmdCommit did — under a local
|
||||
// `diff.ignoreSubmodules=all`, a genuinely bumped submodule gitlink is
|
||||
// invisible here too, so `changedFiles` comes back empty and the function
|
||||
// reports `nothing_to_commit` before ever reaching the (now-fixed) commit
|
||||
// call. `--ignore-submodules=dirty` pins this the same way, reported
|
||||
// verbatim: `git -C repo status --porcelain` (no flag) shows nothing for a
|
||||
// pure gitlink bump under `diff.ignoreSubmodules=all`, while
|
||||
// `--ignore-submodules=dirty` reports ` M nested` (reproduced directly).
|
||||
const statusResult = execGit(
|
||||
['-c', 'core.quotePath=false', 'status', '--porcelain', '--ignore-submodules=dirty'],
|
||||
{ cwd: repoCwd },
|
||||
);
|
||||
if (statusResult.exitCode !== 0) {
|
||||
error(`git status failed in ${repo}: ${statusResult.stderr}`);
|
||||
}
|
||||
@@ -2381,7 +2433,12 @@ function cmdPrSubrepo(
|
||||
const commitArgs = canScopePr
|
||||
? ['commit', '-m', commitMessage as string, '--', ...changedFiles]
|
||||
: ['commit', '-m', commitMessage as string];
|
||||
const commitResult = execGit(commitArgs, { cwd: repoCwd, timeout: COMMIT_TIMEOUT_MS });
|
||||
// #3859 follow-up fix as cmdCommit above (line ~2081) — git 2.39.5 needs
|
||||
// this override for pathspec-scoped AND whole-index commits alike.
|
||||
const commitEnvPr: Record<string, string> = {
|
||||
GIT_CONFIG_COUNT: '1', GIT_CONFIG_KEY_0: 'diff.ignoreSubmodules', GIT_CONFIG_VALUE_0: 'dirty',
|
||||
};
|
||||
const commitResult = execGit(commitArgs, { cwd: repoCwd, timeout: COMMIT_TIMEOUT_MS, env: commitEnvPr });
|
||||
if (commitResult.exitCode !== 0) {
|
||||
rollback();
|
||||
if (isSpawnTimeout(commitResult)) {
|
||||
|
||||
Reference in New Issue
Block a user