diff --git a/.github/workflows/assets/lib.sh b/.github/workflows/assets/lib.sh index da24cbc..2ba0cef 100644 --- a/.github/workflows/assets/lib.sh +++ b/.github/workflows/assets/lib.sh @@ -202,13 +202,28 @@ update_translations_submodule() { fi } +# `git push -h` exits 129, so capture the help text first (|| true) and grep it +# separately. Piping straight into grep would fail the probe under `set -o +# pipefail` (which the workflows enable) even when the flag is present. +git_push_help_mentions() { + local help + help="$(git push -h 2>&1 || true)" + grep -qF "$1" <<<"$help" +} + git_push_supports_force_with_lease() { - git push -h 2>&1 | grep -qF 'force-with-lease' + git_push_help_mentions 'force-with-lease' +} + +# --force-if-includes (Git 2.30+) makes the safe retry below reject an unmerged +# remote advance instead of overwriting it. +git_push_supports_force_if_includes() { + git_push_help_mentions 'force-if-includes' } commit_and_push_translations_branch() { local dir="$1" branch="$2" libs_ref="$3" force="${4:-false}" - local push_rc remote_sha + local push_rc remote_sha fetch_rc attempt max_attempts=3 git -C "$dir" status --short if git -C "$dir" diff --cached --quiet; then echo " No staged submodule changes on $branch; skipping commit." >&2 @@ -220,14 +235,35 @@ commit_and_push_translations_branch() { phase_err "git push --force-with-lease is not supported by this Git installation" return 1 fi - if git -C "$dir" push --force-with-lease origin "$branch"; then - : - else - push_rc=$? - remote_sha=$(git -C "$dir" ls-remote --heads origin "$branch" | awk '{print $1}') - phase_err "force-with-lease push rejected for branch $branch (remote HEAD=${remote_sha:-unknown}); remote may have advanced concurrently — re-run after fetch or resolve manually." - return "$push_rc" + if ! git_push_supports_force_if_includes; then + phase_err "git push --force-if-includes is not supported; requires Git 2.30+" + return 1 fi + # A concurrent advance rejects the lease. Fetch so the lease reflects the + # advanced remote and retry, but keep --force-if-includes so the retry still + # rejects an advance we never integrated. A spurious rejection (remote + # unchanged from our base) retries to success; a real concurrent commit + # fails closed rather than being silently overwritten. + for (( attempt=1; attempt<=max_attempts; attempt++ )); do + if git -C "$dir" push --force-with-lease --force-if-includes origin "$branch"; then + return 0 + else + push_rc=$? + fi + if (( attempt < max_attempts )); then + # A failed fetch (auth/network) leaves the tracking ref stale, so a + # retry can't satisfy the lease. Fail with the real cause instead of + # letting the loop misreport it as a concurrent-advance rejection. + git -C "$dir" fetch origin "$branch" || { + fetch_rc=$? + phase_err "failed to fetch origin/$branch before retrying the push" + return "$fetch_rc" + } + fi + done + remote_sha=$(git -C "$dir" ls-remote --heads origin "$branch" | awk '{print $1}') + phase_err "force-with-lease push rejected for branch $branch (remote HEAD=${remote_sha:-unknown}); remote may have advanced concurrently — re-run after fetch or resolve manually." + return "$push_rc" else git -C "$dir" push origin "$branch" fi diff --git a/.github/workflows/sync-translation.yml b/.github/workflows/sync-translation.yml index bf66bc0..23b28e5 100644 --- a/.github/workflows/sync-translation.yml +++ b/.github/workflows/sync-translation.yml @@ -138,8 +138,9 @@ jobs: 2>/dev/null | sed 's/^[^ ]* //') if [[ $updated -eq 1 ]] && ! git diff --staged --quiet; then - git commit -m "Update libs submodules ($branch)" - git push --force-with-lease origin "HEAD:$branch" + # Commit + push through the shared helper so this path gets the same + # bounded retry and --force-if-includes safety as the finalize flow. + commit_and_push_translations_branch "$GITHUB_WORKSPACE" "$branch" "$branch" true echo " Pushed to origin/$branch" >&2 else echo " No pointer changes for $branch." >&2 diff --git a/tests/helpers/git_fixtures.bash b/tests/helpers/git_fixtures.bash index 525b474..94f20f8 100644 --- a/tests/helpers/git_fixtures.bash +++ b/tests/helpers/git_fixtures.bash @@ -86,27 +86,44 @@ push_commit_to_remote_branch() { rm -rf "$tmp" } -# Prepend a git wrapper to PATH that advances remote branch before push --force-with-lease. +# A git wrapper that injects a concurrent advance before each force-with-lease +# push, so the lease is rejected. The optional 4th arg caps how many pushes get a +# fresh advance (0 = every push, the default); pass 1 to inject a single real +# concurrent advance that the retry still refuses to overwrite. Pass 1 as the +# 5th arg to also fail every branch-specific `git fetch origin ` (the +# inter-attempt retry fetch). install_git_push_pre_hook() { - local bare="$1" branch="$2" message="${3:-concurrent advance}" + local bare="$1" branch="$2" message="${3:-concurrent advance}" max_injections="${4:-0}" fail_branch_fetch="${5:-0}" local wrapper_dir="$GIT_FIXTURE_ROOT/bin" REAL_GIT="$(command -v git)" export REAL_GIT export GIT_HOOK_BARE_REMOTE="$bare" GIT_HOOK_BRANCH="$branch" GIT_HOOK_MESSAGE="$message" + export GIT_HOOK_MAX_INJECTIONS="$max_injections" + export GIT_HOOK_INJECT_COUNTER="$GIT_FIXTURE_ROOT/inject-count" + export GIT_HOOK_FAIL_BRANCH_FETCH="$fail_branch_fetch" + : >"$GIT_HOOK_INJECT_COUNTER" mkdir -p "$wrapper_dir" cat >"$wrapper_dir/git" <<'EOF' #!/usr/bin/env bash +if [[ "$GIT_HOOK_FAIL_BRANCH_FETCH" == "1" && "${1:-}" == "-C" && "${3:-}" == "fetch" && -n "${5:-}" ]]; then + echo "simulated fetch failure" >&2 + exit 1 +fi if [[ "${1:-}" == "-C" && "${3:-}" == "push" && "$*" == *"--force-with-lease"* ]]; then - tmp="$(mktemp -d)" - "$REAL_GIT" clone "$GIT_HOOK_BARE_REMOTE" "$tmp" - "$REAL_GIT" -C "$tmp" config user.email "test@test.local" - "$REAL_GIT" -C "$tmp" config user.name "Test" - "$REAL_GIT" -C "$tmp" checkout "$GIT_HOOK_BRANCH" - echo "$GIT_HOOK_MESSAGE" >>"$tmp/README" - "$REAL_GIT" -C "$tmp" add README - "$REAL_GIT" -C "$tmp" commit -m "$GIT_HOOK_MESSAGE" - "$REAL_GIT" -C "$tmp" push origin "$GIT_HOOK_BRANCH" - rm -rf "$tmp" + injected="$(wc -l <"$GIT_HOOK_INJECT_COUNTER" 2>/dev/null || echo 0)" + if [[ "$GIT_HOOK_MAX_INJECTIONS" -eq 0 || "$injected" -lt "$GIT_HOOK_MAX_INJECTIONS" ]]; then + tmp="$(mktemp -d)" + "$REAL_GIT" clone "$GIT_HOOK_BARE_REMOTE" "$tmp" + "$REAL_GIT" -C "$tmp" config user.email "test@test.local" + "$REAL_GIT" -C "$tmp" config user.name "Test" + "$REAL_GIT" -C "$tmp" checkout "$GIT_HOOK_BRANCH" + echo "$GIT_HOOK_MESSAGE" >>"$tmp/README" + "$REAL_GIT" -C "$tmp" add README + "$REAL_GIT" -C "$tmp" commit -m "$GIT_HOOK_MESSAGE" + "$REAL_GIT" -C "$tmp" push origin "$GIT_HOOK_BRANCH" + rm -rf "$tmp" + echo 1 >>"$GIT_HOOK_INJECT_COUNTER" + fi fi exec "$REAL_GIT" "$@" EOF @@ -122,20 +139,22 @@ restore_git_push_pre_hook() { rm -rf "$GIT_WRAPPER_DIR" unset GIT_WRAPPER_DIR REAL_GIT unset GIT_HOOK_BARE_REMOTE GIT_HOOK_BRANCH GIT_HOOK_MESSAGE GIT_FETCH_COUNTER_FILE + unset GIT_HOOK_MAX_INJECTIONS GIT_HOOK_INJECT_COUNTER GIT_HOOK_FAIL_BRANCH_FETCH GIT_HIDE_PUSH_FLAG fi } -# Prepend a git wrapper that hides --force-with-lease from push -h (simulates pre-2.8 git). -install_git_without_force_with_lease() { +# Prepend a git wrapper that hides one flag from `git push -h` output (which +# exits 129) to simulate a git that predates that flag. +install_git_without_flag() { local wrapper_dir="$GIT_FIXTURE_ROOT/bin" REAL_GIT="$(command -v git)" - export REAL_GIT + export REAL_GIT GIT_HIDE_PUSH_FLAG="$1" mkdir -p "$wrapper_dir" cat >"$wrapper_dir/git" <<'EOF' #!/usr/bin/env bash if [[ "${1:-}" == "push" && "${2:-}" == "-h" ]]; then - "$REAL_GIT" push -h 2>&1 | grep -v 'force-with-lease' - exit 0 + "$REAL_GIT" push -h 2>&1 | grep -vF "$GIT_HIDE_PUSH_FLAG" + exit 129 # match real `git push -h` fi exec "$REAL_GIT" "$@" EOF @@ -144,6 +163,11 @@ EOF export PATH="$wrapper_dir:$PATH" } +# simulates pre-2.8 git (no force-with-lease) +install_git_without_force_with_lease() { install_git_without_flag 'force-with-lease'; } +# simulates a git from 2.8 to 2.29 (force-with-lease present, force-if-includes not) +install_git_without_force_if_includes() { install_git_without_flag 'force-if-includes'; } + # Prepend a git wrapper that counts fetch invocations and optionally injects concurrent push. install_git_fetch_counter() { local counter_file="$1" diff --git a/tests/test_lib.bats b/tests/test_lib.bats index 97d9799..03930cb 100644 --- a/tests/test_lib.bats +++ b/tests/test_lib.bats @@ -345,7 +345,7 @@ teardown() { git --git-dir="$BARE_REMOTE" show -s --format=%s "$remote_sha" | grep -q "concurrent advance" } -@test "finalize_translations_local: fail-fast on lease rejection (no retry)" { +@test "finalize_translations_local: does not overwrite a concurrent advance (force-if-includes)" { # shellcheck source=tests/helpers/git_fixtures.bash source "$BATS_TEST_DIRNAME/helpers/git_fixtures.bash" init_git_fixture_root @@ -358,22 +358,84 @@ teardown() { UPDATES=("algorithm") update_translations_submodule() { :; } + # One concurrent advance: the fetch updates the lease baseline, but + # --force-if-includes still rejects the retry because that advance was never + # integrated, so the run fails closed instead of overwriting it. + install_git_push_pre_hook "$BARE_REMOTE" "${LOCAL_BRANCH_PREFIX}en" "concurrent advance" 1 + + local stderr_file="$BATS_TMPDIR/finalize-no-overwrite-stderr" + set +e + finalize_translations_local "$trans_dir" "develop" "en" 2>"$stderr_file" + rc=$? + set -e + + [ "$rc" -ne 0 ] + grep -q "force-with-lease push rejected" "$stderr_file" + # the concurrent advance is still the remote tip, not overwritten by our push + remote_sha=$(git ls-remote --heads "$BARE_REMOTE" "${LOCAL_BRANCH_PREFIX}en" | awk '{print $1}') + git --git-dir="$BARE_REMOTE" show -s --format=%s "$remote_sha" | grep -q "concurrent advance" +} + +@test "finalize_translations_local: fails after force-with-lease retries are exhausted" { + # shellcheck source=tests/helpers/git_fixtures.bash + source "$BATS_TEST_DIRNAME/helpers/git_fixtures.bash" + init_git_fixture_root + create_bare_remote_with_clone "translations" + create_remote_branch "$BARE_REMOTE" "${LOCAL_BRANCH_PREFIX}en" "$MASTER_BRANCH" + trans_dir="$GIT_FIXTURE_ROOT/translations-work" + git clone "$BARE_REMOTE" "$trans_dir" + set_git_bot_config "$trans_dir" + + UPDATES=("algorithm") + update_translations_submodule() { :; } + + # Advance the remote before every push, so all three attempts are rejected. + # The counter also records fetches: 1 finalize pre-fetch + 2 between retries. local fetch_counter="$BATS_TMPDIR/fetch-count" : >"$fetch_counter" install_git_fetch_counter "$fetch_counter" "$BARE_REMOTE" "${LOCAL_BRANCH_PREFIX}en" "concurrent advance" - remote_sha_before=$(git ls-remote --heads "$BARE_REMOTE" "${LOCAL_BRANCH_PREFIX}en" | awk '{print $1}') + local stderr_file="$BATS_TMPDIR/finalize-exhausted-stderr" + set +e + finalize_translations_local "$trans_dir" "develop" "en" 2>"$stderr_file" + rc=$? + set -e + + [ "$rc" -ne 0 ] + [ "$(wc -l <"$fetch_counter")" -eq 3 ] + grep -q "force-with-lease push rejected" "$stderr_file" + remote_sha=$(git ls-remote --heads "$BARE_REMOTE" "${LOCAL_BRANCH_PREFIX}en" | awk '{print $1}') + grep -q "$remote_sha" "$stderr_file" + git --git-dir="$BARE_REMOTE" show -s --format=%s "$remote_sha" | grep -q "concurrent advance" +} +@test "finalize_translations_local: fails with the real cause when an inter-attempt fetch fails" { + # shellcheck source=tests/helpers/git_fixtures.bash + source "$BATS_TEST_DIRNAME/helpers/git_fixtures.bash" + init_git_fixture_root + create_bare_remote_with_clone "translations" + create_remote_branch "$BARE_REMOTE" "${LOCAL_BRANCH_PREFIX}en" "$MASTER_BRANCH" + trans_dir="$GIT_FIXTURE_ROOT/translations-work" + git clone "$BARE_REMOTE" "$trans_dir" + set_git_bot_config "$trans_dir" + + UPDATES=("algorithm") + update_translations_submodule() { :; } + + # First push is rejected (one concurrent advance), then the inter-attempt + # `git fetch origin ` fails. + install_git_push_pre_hook "$BARE_REMOTE" "${LOCAL_BRANCH_PREFIX}en" "concurrent advance" 1 1 + + local stderr_file="$BATS_TMPDIR/finalize-fetch-fail-stderr" set +e - finalize_translations_local "$trans_dir" "develop" "en" 2>/dev/null + finalize_translations_local "$trans_dir" "develop" "en" 2>"$stderr_file" rc=$? set -e [ "$rc" -ne 0 ] - [ "$(wc -l <"$fetch_counter")" -eq 1 ] - remote_sha_after=$(git ls-remote --heads "$BARE_REMOTE" "${LOCAL_BRANCH_PREFIX}en" | awk '{print $1}') - [ "$remote_sha_before" != "$remote_sha_after" ] - git --git-dir="$BARE_REMOTE" show -s --format=%s "$remote_sha_after" | grep -q "concurrent advance" + grep -q "failed to fetch origin/${LOCAL_BRANCH_PREFIX}en" "$stderr_file" + # not misreported as a concurrent-advance rejection + ! grep -q "force-with-lease push rejected" "$stderr_file" } @test "commit_and_push_translations_branch: clear error when force-with-lease unsupported" { @@ -390,17 +452,55 @@ teardown() { install_git_without_force_with_lease local stderr_file="$BATS_TMPDIR/push-unsupported-stderr" - set +e + # pipefail is on in the workflows; the probe must still work when `git push -h` + # exits 129 (the shim mirrors that). + set +e -o pipefail commit_and_push_translations_branch "$trans_dir" "${LOCAL_BRANCH_PREFIX}en" "develop" true \ 2>"$stderr_file" rc=$? - set -e + set -e +o pipefail [ "$rc" -ne 0 ] grep -q "force-with-lease is not supported" "$stderr_file" grep -q "not supported by this Git installation" "$stderr_file" } +@test "commit_and_push_translations_branch: clear error when force-if-includes unsupported" { + # shellcheck source=tests/helpers/git_fixtures.bash + source "$BATS_TEST_DIRNAME/helpers/git_fixtures.bash" + init_git_fixture_root + create_bare_remote_with_clone "translations" + create_remote_branch "$BARE_REMOTE" "${LOCAL_BRANCH_PREFIX}en" "$MASTER_BRANCH" + trans_dir="$GIT_FIXTURE_ROOT/translations-work" + git clone "$BARE_REMOTE" "$trans_dir" + set_git_bot_config "$trans_dir" + git -C "$trans_dir" checkout "${LOCAL_BRANCH_PREFIX}en" + + # force-with-lease present, force-if-includes hidden (pre-2.30 Git). + install_git_without_force_if_includes + + local stderr_file="$BATS_TMPDIR/push-no-includes-stderr" + set +e -o pipefail + commit_and_push_translations_branch "$trans_dir" "${LOCAL_BRANCH_PREFIX}en" "develop" true \ + 2>"$stderr_file" + rc=$? + set -e +o pipefail + + [ "$rc" -ne 0 ] + grep -q "force-if-includes is not supported" "$stderr_file" + grep -q "requires Git 2.30+" "$stderr_file" +} + +@test "git_push_supports_* probes are pipefail-safe with real git" { + # Modern git supports both flags, but `git push -h` exits 129; piped into grep + # that fails under pipefail even when the flag is present. The probes must + # still report supported. + set -o pipefail + git_push_supports_force_with_lease + git_push_supports_force_if_includes + set +o pipefail +} + @test "begin_phase and end_phase: emit group markers and track CURRENT_PHASE" { local out_file="$BATS_TMPDIR/phase.out"