diff --git a/.changes/source-control/fix-landed-work-empty-base-commit-3c9e71a4.md b/.changes/source-control/fix-landed-work-empty-base-commit-3c9e71a4.md new file mode 100644 index 00000000000..b3880cbc841 --- /dev/null +++ b/.changes/source-control/fix-landed-work-empty-base-commit-3c9e71a4.md @@ -0,0 +1,7 @@ +--- +bump: patch +--- + +### Fixed + +- The landed-work engine behind `/source-control:worktree cleanup` and the daily worktree reap no longer discards its base patch-id set when the base holds an empty commit, such as an empty squash merge. Such a commit has no patch to hash; it is now excused once `git diff-tree` shows its diff is empty, so every worktree stops rebuilding the base set and falling through to the merged-PR read. A failed merged-PR read now names gh's own error in its reason. diff --git a/plugins/source-control/scripts/landed-work.sh b/plugins/source-control/scripts/landed-work.sh index 9bafa5457d9..5fd8109b95c 100755 --- a/plugins/source-control/scripts/landed-work.sh +++ b/plugins/source-control/scripts/landed-work.sh @@ -364,15 +364,33 @@ resolve_base() { # equals the squash commit's, and still matches after the base advances. Both # verified in a fixture rather than assumed. patch_ids() { - local p="$1" range="$2" out="$3" patches="$WORKDIR/patches.$$" rc=0 + local p="$1" range="$2" out="$3" commits_out="${4:-}" patches="$WORKDIR/patches.$$" pairs="$WORKDIR/pairs.$$" rc=0 : >"$out" + [[ -n "$commits_out" ]] && : >"$commits_out" git -C "$p" log -p --no-merges --no-color "$range" >"$patches" 2>/dev/null || rc=1 # An empty stream is a legitimate empty range, so only a non-empty one is fed # to patch-id; $out stays truncated either way. if ((rc == 0)) && [[ -s "$patches" ]]; then - git patch-id --verbatim <"$patches" | cut -d' ' -f1 | sort >"$out" || rc=1 + git patch-id --verbatim <"$patches" >"$pairs" || rc=1 + cut -d' ' -f1 "$pairs" | sort >"$out" + [[ -n "$commits_out" ]] && cut -d' ' -f2 "$pairs" | sort >"$commits_out" fi - rm -f "$patches" + rm -f "$patches" "$pairs" + return "$rc" +} + +# only_empty_commits_unhashed : succeed when every +# non-merge commit in that patch-id did not hash has an empty diff. Such a +# commit has no patch to hash, so it cannot be a rendering failure. +only_empty_commits_unhashed() { + local p="$1" range="$2" hashed="$3" all="$WORKDIR/range-commits.$$" c rc=0 + git -C "$p" rev-list --no-merges "$range" 2>/dev/null | sort >"$all" || rc=1 + if ((rc == 0)); then + while read -r c; do + git -C "$p" diff-tree --quiet --root --no-commit-id -r "$c" 2>/dev/null || { rc=1; break; } + done < <(comm -23 "$all" "$hashed") + fi + rm -f "$all" return "$rc" } @@ -435,8 +453,8 @@ classify_landed() { L_REASON="base-range-unreadable" return 0 fi - if ! patch_ids "$p" "$mb..$base_sha" "$base_ids"; then - rm -f "$base_ids" + if ! patch_ids "$p" "$mb..$base_sha" "$base_ids" "$base_ids.commits"; then + rm -f "$base_ids" "$base_ids.commits" L_REASON="base-patchid-failed" return 0 fi @@ -445,14 +463,17 @@ classify_landed() { # between `yes` and `no` today — it is here so the two sides cannot silently # diverge if either is ever refactored, and so a base range that failed to # render is named rather than quietly narrowing the id set it is compared - # against. + # against. An empty commit on the base (an empty squash merge) has no patch, + # so it is excused only after diff-tree shows its diff really is empty. local base_count base_count=$(wc -l <"$base_ids" | tr -d '[:space:]') - if [[ "$base_count" -ne "$base_expected" ]]; then - rm -f "$base_ids" + if [[ "$base_count" -ne "$base_expected" ]] && + ! only_empty_commits_unhashed "$p" "$mb..$base_sha" "$base_ids.commits"; then + rm -f "$base_ids" "$base_ids.commits" L_REASON="base-patchid-incomplete-$base_count-of-$base_expected-commits" return 0 fi + rm -f "$base_ids.commits" fi # Branch side. The COMPLETENESS check runs before any verdict, because both @@ -615,8 +636,12 @@ merged_pr_proof() { fi if ! (cd "$p" && gh pr list --state merged --head "$branch" --limit 20 \ --json headRefOid,baseRefName,commits \ - --jq '.[] | "pr " + .baseRefName, "head " + .headRefOid, (.commits[] | "commit " + .oid)') >"$out" 2>/dev/null; then - L_REASON="${L_REASON:+$L_REASON; }merged-pr-read-failed:gh-pr-list" + --jq '.[] | "pr " + .baseRefName, "head " + .headRefOid, (.commits[] | "commit " + .oid)') >"$out" 2>"$out.err"; then + # Name gh's own error so a failed run states its cause; tabs and semicolons + # would break the record's columns and reason list. + local err + err=$(head -n 1 "$out.err" | tr '\t;' ' ' | cut -c1-120) + L_REASON="${L_REASON:+$L_REASON; }merged-pr-read-failed:gh-pr-list${err:+ ($err)}" return 1 fi local in_base=false diff --git a/plugins/source-control/scripts/landed-work.test.sh b/plugins/source-control/scripts/landed-work.test.sh index a13ab8bde97..c230e059d6f 100755 --- a/plugins/source-control/scripts/landed-work.test.sh +++ b/plugins/source-control/scripts/landed-work.test.sh @@ -141,6 +141,18 @@ assert_eq "squash: still landed after the base advances" "yes" "$(col "$R" $C_LA assert_eq "squash: still by range patch-id after the base advances" \ "range-patchid" "$(col "$R" $C_METHOD)" +# An empty commit on the base (an empty squash merge) has no patch to hash. It is +# excused from the base-side count, so the base set stays usable; a commit whose +# diff fails to render still is not (base-patchid-incomplete). +git -C "$W" commit -q --allow-empty -m "empty squash on the base" >/dev/null 2>&1 +git -C "$W" push -q origin main >/dev/null 2>&1 + +OUT="$(bash "$ENGINE" --repo-dir "$W" --no-peers)" +R="$(row "$OUT" "wt-squash")" +assert_eq "squash: still landed past an empty base commit" "yes" "$(col "$R" $C_LANDED)" +assert_not_contains "and the base set is not reported incomplete" \ + "$(col "$R" $C_REASON)" "base-patchid-incomplete" + # -------------------------------------------------------------------------- # Negative control — a genuinely unmerged branch is NOT landed # -------------------------------------------------------------------------- @@ -621,7 +633,7 @@ PR_BIN="$TEST_TMPDIR/pr-bin" mkdir -p "$PR_BIN" cat >"$PR_BIN/gh" <<'STUB' #!/usr/bin/env bash -[[ -n "${GH_STUB_FAIL:-}" ]] && { echo "gh stub: forced failure" >&2; exit 1; } +[[ -n "${GH_STUB_FAIL:-}" ]] && { printf 'gh stub:\tforced; failure\nsecond line\n' >&2; exit 1; } cat "${GH_STUB_OUT:-/dev/null}" STUB chmod +x "$PR_BIN/gh" @@ -676,6 +688,12 @@ assert_contains "pr proof: the reason says why" "$(col "$R" $C_REASON)" "local-c R="$(row "$(run_pr "$TEST_TMPDIR/pr-commits-only" 1)" "wt-pr")" assert_eq "pr proof: a failed read keeps the prior verdict" "no" "$(col "$R" $C_LANDED)" assert_contains "pr proof: the failed read is named" "$(col "$R" $C_REASON)" "merged-pr-read-failed:gh-pr-list" +# gh's first error line rides along, its tab and semicolon blanked so the row's +# columns and the reason list keep their shape; later lines are dropped. +assert_contains "pr proof: gh's own error is named" "$(col "$R" $C_REASON)" "gh-pr-list (gh stub: forced failure)" +assert_not_contains "pr proof: only gh's first error line is kept" "$(col "$R" $C_REASON)" "second line" +assert_eq "pr proof: the error text adds no column" \ + "$(head -n 1 <<<"$OUT" | awk -F'\t' '{print NF}')" "$(awk -F'\t' '{print NF}' <<<"$R")" OUT="$(bash "$ENGINE" --repo-dir "$W" --no-peers)" # Bare-name invocation from the scripts directory still loads the library.