feature/readonly restore regression#53
Conversation
flyingrobots
commented
Oct 21, 2025
flyingrobots
commented
- test: add remote restore regression coverage
- test: scaffold caller remote snapshot
- test: implement remote snapshot restore fallback
- docs: note remote restore skip flag for tests
Summary by CodeRabbitRelease Notes
WalkthroughAdds a comprehensive BATS test suite for remote-restore behavior, a README note for simulating read-only Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant BATS as BATS test
participant Helpers as shiplog helpers
participant Git as git (repo)
rect #E8F5E9
Note right of Helpers: Snapshot phase
BATS->>Helpers: shiplog_snapshot_caller_repo_state()
Helpers->>Git: query remotes & configs
Git-->>Helpers: remotes + config entries
Helpers-->>BATS: store SHIPLOG_ORIG_REMOTE_ORDER & SHIPLOG_ORIG_REMOTES_CONFIG
end
rect #FFF3E0
Note right of Helpers: Restore phase
BATS->>Helpers: shiplog_restore_caller_remotes()
Helpers->>Git: list current remotes
Git-->>Helpers: current remotes
alt unexpected remotes present
Helpers->>Git: git remote remove <name> (via shiplog_restore_exec)
end
loop restore saved remotes/configs
Helpers->>Git: git remote add <name> <url> (via shiplog_restore_exec)
Helpers->>Git: git config set <key> <value> (via shiplog_restore_exec)
Git-->>Helpers: success / error (readonly detection)
end
Helpers->>BATS: shiplog_reset_remote_snapshot_state()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
test/26_remote_restore.bats(1 hunks)test/README.md(1 hunks)test/helpers/common.bash(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
test/**/*.bats
📄 CodeRabbit inference engine (AGENTS.md)
Tests that manipulate Git refs must self-clean (e.g.,
git update-ref -d) and generate unique identifiers per test to avoid cross-run collisions
Files:
test/26_remote_restore.bats
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: flyingrobots/shiplog#0
File: AGENTS.md:0-0
Timestamp: 2025-10-15T00:33:11.402Z
Learning: Applies to test/**/*.bats : Tests that manipulate Git refs must self-clean (e.g., `git update-ref -d`) and generate unique identifiers per test to avoid cross-run collisions
📚 Learning: 2025-10-15T00:33:11.402Z
Learnt from: CR
PR: flyingrobots/shiplog#0
File: AGENTS.md:0-0
Timestamp: 2025-10-15T00:33:11.402Z
Learning: Applies to test/**/*.bats : Tests that manipulate Git refs must self-clean (e.g., `git update-ref -d`) and generate unique identifiers per test to avoid cross-run collisions
Applied to files:
test/README.md
🧬 Code graph analysis (1)
test/26_remote_restore.bats (1)
test/helpers/common.bash (4)
shiplog_reset_remote_snapshot_state(38-43)shiplog_git_caller(13-15)shiplog_snapshot_caller_repo_state(45-75)shiplog_restore_caller_remotes(77-203)
🪛 GitHub Actions: Bash Matrix CI
test/26_remote_restore.bats
[error] 22-22: not ok 83 restore removes unexpected remotes. 'git remote add "$remote" https://example.invalid/original.git' failed with status 128; fatal: detected dubious ownership in repository at '/work'.
[error] 37-37: not ok 84 restore rebuilds multi-url remote configuration. 'git remote add "$remote" https://example.invalid/primary.git' failed with status 128; fatal: detected dubious ownership in repository at '/work'.
🔇 Additional comments (6)
test/README.md (1)
31-31: Documentation looks acceptable.The explanation is clear enough. You're documenting a test-only workaround for the Docker-as-root limitation. Fine.
test/helpers/common.bash (3)
13-15: LGTM - critical wrapper for test git operations.This wrapper adds the safe.directory config that prevents dubious ownership errors. Good. Now make sure EVERY git call in your tests uses this wrapper instead of bare
gitcommands.
45-75: Snapshot logic looks correct.The remote enumeration and config capture logic is sound. Line 63's regex escaping is adequate for basic regex patterns used by
git config --get-regexp. The escaping handles the standard metacharacters for POSIX basic regex.
77-203: Stop worrying about this—the code is CORRECT.Here's what actually happens:
"Only pushurl" edge case? Impossible. Git won't let you. Try deleting the only fetch URL and git says "fatal: Will not delete all non-push URLs". Your code already guards against this at lines 141-150 by requiring
first_urlto be non-empty. Done.Multiple URLs ordering? Git PRESERVES IT. The test proves it: add url1, then set-url --add url2, set-url --add url3, and git config shows them in order. Your code at lines 161-171 correctly skips the primary URL on re-add. No breakage.
Unreachable first URL during restore? Not your problem. Git tries each URL sequentially during fetch. The first URL is just the primary for the
remote addcall—if it fails, git keeps trying. Your exit code handling is correct.The restoration logic is sound. Multiple URLs work. Pushurl handling works. The test harness override is documented. You've thought through the error cases.
Stop second-guessing yourself and ship it.
test/26_remote_restore.bats (2)
5-17: Setup/teardown follows self-cleaning guidelines.Good. You're using unique identifiers per test (BATS_TEST_NUMBER) and cleaning up remotes in both setup and teardown. The defensive
|| truehandling is appropriate since these operations might fail if state is already clean.Based on learnings
61-83: Test logic is acceptable but has limitations.This test validates the read-only skip path, but there's a catch: because Docker runs as root, the
chmod u-won line 75 doesn't actually prevent writes. So you're usingSHIPLOG_FORCE_REMOTE_RESTORE_SKIP=1to force the skip path, which is documented in README.md.This means you're testing the skip LOGIC but not actual read-only file permission handling. That's acceptable given the Docker-as-root constraint, but understand you're not exercising the real read-only detection regex patterns in
shiplog_restore_exec.However, lines 68-69, 74, and 79 STILL use bare
gitcommands. Even though the subshell sets a differentSHIPLOG_TEST_ROOT, you should useshiplog_git_callerfor consistency and to avoid confusion.Fix the bare git commands here too. Then consider adding a test that actually exercises the read-only detection patterns (maybe by mocking
shiplog_git_callerto return read-only error messages).
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
Here are the copyable unit test edits: Copyable Editstest/26_remote_restore.bats@@ -81,3 +81,591 @@
[ -z "$output" ]
)
}
+
+@test "snapshot captures empty remote list" {
+ # Remove all remotes first
+ for remote in $(git remote); do
+ git remote remove "$remote" 2>/dev/null || true
+ done
+
+ run shiplog_snapshot_caller_repo_state
+ [ "$status" -eq 0 ]
+ [ "$SHIPLOG_CALLER_REPO_CAPTURED" -eq 1 ]
+ [ "${#SHIPLOG_ORIG_REMOTE_ORDER[@]}" -eq 0 ]
+}
+
+@test "snapshot captures single remote with basic config" {
+ local remote="shimtest-${BATS_TEST_NUMBER}"
+ git remote add "$remote" https://example.invalid/single.git
+
+ run shiplog_snapshot_caller_repo_state
+ [ "$status" -eq 0 ]
+ [ "$SHIPLOG_CALLER_REPO_CAPTURED" -eq 1 ]
+ [ "${#SHIPLOG_ORIG_REMOTE_ORDER[@]}" -eq 1 ]
+ [ "${SHIPLOG_ORIG_REMOTE_ORDER[0]}" = "$remote" ]
+ [[ "${SHIPLOG_ORIG_REMOTES_CONFIG[$remote]}" == *"$remote.url"* ]]
+}
+
+@test "snapshot captures multiple remotes in order" {
+ local remote1="alpha-${BATS_TEST_NUMBER}"
+ local remote2="beta-${BATS_TEST_NUMBER}"
+ local remote3="gamma-${BATS_TEST_NUMBER}"
+
+ git remote add "$remote1" https://example.invalid/alpha.git
+ git remote add "$remote2" https://example.invalid/beta.git
+ git remote add "$remote3" https://example.invalid/gamma.git
+
+ run shiplog_snapshot_caller_repo_state
+ [ "$status" -eq 0 ]
+ [ "${#SHIPLOG_ORIG_REMOTE_ORDER[@]}" -eq 3 ]
+
+ # Verify all remotes are captured
+ local found_alpha=0 found_beta=0 found_gamma=0
+ for remote in "${SHIPLOG_ORIG_REMOTE_ORDER[@]}"; do
+ if [ "$remote" = "$remote1" ]; then found_alpha=1; fi
+ if [ "$remote" = "$remote2" ]; then found_beta=1; fi
+ if [ "$remote" = "$remote3" ]; then found_gamma=1; fi
+ done
+ [ "$found_alpha" -eq 1 ]
+ [ "$found_beta" -eq 1 ]
+ [ "$found_gamma" -eq 1 ]
+}
+
+@test "snapshot captures remote with special characters in name" {
+ local remote="test-remote.with_special-chars-${BATS_TEST_NUMBER}"
+ git remote add "$remote" https://example.invalid/special.git
+
+ run shiplog_snapshot_caller_repo_state
+ [ "$status" -eq 0 ]
+ [ "${SHIPLOG_ORIG_REMOTE_ORDER[0]}" = "$remote" ]
+ [[ "${SHIPLOG_ORIG_REMOTES_CONFIG[$remote]}" == *"$remote.url"* ]]
+}
+
+@test "snapshot fails gracefully when not in git repository" {
+ local temp_dir
+ temp_dir="$(mktemp -d)"
+ export SHIPLOG_TEST_ROOT="$temp_dir"
+
+ run shiplog_snapshot_caller_repo_state
+ [ "$status" -ne 0 ]
+ echo "$output" | grep -q "not a git repository"
+
+ rm -rf "$temp_dir"
+ export SHIPLOG_TEST_ROOT="$(pwd)"
+}
+
+@test "reset clears all snapshot state variables" {
+ local remote="shimtest-${BATS_TEST_NUMBER}"
+ git remote add "$remote" https://example.invalid/test.git
+ shiplog_snapshot_caller_repo_state
+
+ # Verify state is captured
+ [ "$SHIPLOG_CALLER_REPO_CAPTURED" -eq 1 ]
+ [ "${#SHIPLOG_ORIG_REMOTE_ORDER[@]}" -gt 0 ]
+
+ run shiplog_reset_remote_snapshot_state
+ [ "$status" -eq 0 ]
+ [ "$SHIPLOG_CALLER_REPO_CAPTURED" -eq 0 ]
+ [ "${#SHIPLOG_ORIG_REMOTE_ORDER[@]}" -eq 0 ]
+}
+
+@test "restore is no-op when no snapshot captured" {
+ local remote="shimtest-${BATS_TEST_NUMBER}"
+ git remote add "$remote" https://example.invalid/noop.git
+
+ # Don't capture snapshot
+ shiplog_reset_remote_snapshot_state
+
+ run shiplog_restore_caller_remotes
+ [ "$status" -eq 0 ]
+
+ # Remote should still exist since restore did nothing
+ run git remote
+ [ "$status" -eq 0 ]
+ echo "$output" | grep -q "^$remote$"
+}
+
+@test "restore handles empty snapshot correctly" {
+ local remote="shimtest-${BATS_TEST_NUMBER}"
+
+ # Capture empty state
+ shiplog_snapshot_caller_repo_state
+
+ # Add a remote after snapshot
+ git remote add "$remote" https://example.invalid/added.git
+
+ run shiplog_restore_caller_remotes
+ [ "$status" -eq 0 ]
+
+ # Remote should be removed
+ run git remote
+ [ "$status" -eq 0 ]
+ ! echo "$output" | grep -q "^$remote$"
+}
+
+@test "restore preserves remote order" {
+ local remote1="first-${BATS_TEST_NUMBER}"
+ local remote2="second-${BATS_TEST_NUMBER}"
+ local remote3="third-${BATS_TEST_NUMBER}"
+
+ git remote add "$remote1" https://example.invalid/first.git
+ git remote add "$remote2" https://example.invalid/second.git
+ git remote add "$remote3" https://example.invalid/third.git
+
+ shiplog_snapshot_caller_repo_state
+
+ # Remove all and add in different order
+ git remote remove "$remote1"
+ git remote remove "$remote2"
+ git remote remove "$remote3"
+ git remote add "$remote3" https://example.invalid/wrong.git
+ git remote add "$remote1" https://example.invalid/wrong.git
+ git remote add "$remote2" https://example.invalid/wrong.git
+
+ run shiplog_restore_caller_remotes
+ [ "$status" -eq 0 ]
+
+ # Verify all remotes restored with correct URLs
+ local url1 url2 url3
+ url1="$(git config --get remote.$remote1.url)"
+ url2="$(git config --get remote.$remote2.url)"
+ url3="$(git config --get remote.$remote3.url)"
+ [ "$url1" = "https://example.invalid/first.git" ]
+ [ "$url2" = "https://example.invalid/second.git" ]
+ [ "$url3" = "https://example.invalid/third.git" ]
+}
+
+@test "restore handles remote with multiple fetch specs" {
+ local remote="shimtest-${BATS_TEST_NUMBER}"
+ git remote add "$remote" https://example.invalid/multi.git
+ git config --add "remote.${remote}.fetch" "+refs/heads/main:refs/remotes/${remote}/main"
+ git config --add "remote.${remote}.fetch" "+refs/heads/dev:refs/remotes/${remote}/dev"
+ git config --add "remote.${remote}.fetch" "+refs/tags/*:refs/tags/*"
+
+ local before
+ before="$(git config --get-all remote.${remote}.fetch | sort)"
+
+ shiplog_snapshot_caller_repo_state
+
+ # Modify configuration
+ git remote remove "$remote"
+ git remote add "$remote" https://example.invalid/different.git
+
+ run shiplog_restore_caller_remotes
+ [ "$status" -eq 0 ]
+
+ local after
+ after="$(git config --get-all remote.${remote}.fetch | sort)"
+ [ "$before" = "$after" ]
+}
+
+@test "restore handles remote with multiple pushurls" {
+ local remote="shimtest-${BATS_TEST_NUMBER}"
+ git remote add "$remote" https://example.invalid/fetch.git
+ git remote set-url --push "$remote" ssh://push1.invalid/repo.git
+ git remote set-url --push --add "$remote" ssh://push2.invalid/repo.git
+ git remote set-url --push --add "$remote" ssh://push3.invalid/repo.git
+
+ local before
+ before="$(git config --get-all remote.${remote}.pushurl | sort)"
+
+ shiplog_snapshot_caller_repo_state
+
+ git remote remove "$remote"
+ git remote add "$remote" https://example.invalid/wrong.git
+
+ run shiplog_restore_caller_remotes
+ [ "$status" -eq 0 ]
+
+ local after
+ after="$(git config --get-all remote.${remote}.pushurl | sort)"
+ [ "$before" = "$after" ]
+}
+
+@test "restore handles remote with custom config keys" {
+ local remote="shimtest-${BATS_TEST_NUMBER}"
+ git remote add "$remote" https://example.invalid/custom.git
+ git config "remote.${remote}.tagOpt" "--no-tags"
+ git config "remote.${remote}.prune" "true"
+ git config "remote.${remote}.skipDefaultUpdate" "true"
+
+ local before
+ before="$(git config --get-regexp "^remote\.${remote}\." | sort)"
+
+ shiplog_snapshot_caller_repo_state
+
+ git remote remove "$remote"
+ git remote add "$remote" https://example.invalid/plain.git
+
+ run shiplog_restore_caller_remotes
+ [ "$status" -eq 0 ]
+
+ local after
+ after="$(git config --get-regexp "^remote\.${remote}\." | sort)"
+ [ "$before" = "$after" ]
+}
+
+@test "restore removes multiple unexpected remotes" {
+ local remote1="expected-${BATS_TEST_NUMBER}"
+ local remote2="unexpected1-${BATS_TEST_NUMBER}"
+ local remote3="unexpected2-${BATS_TEST_NUMBER}"
+
+ git remote add "$remote1" https://example.invalid/expected.git
+ shiplog_snapshot_caller_repo_state
+
+ git remote add "$remote2" https://example.invalid/unexpected1.git
+ git remote add "$remote3" https://example.invalid/unexpected2.git
+
+ run git remote
+ [ "$status" -eq 0 ]
+ echo "$output" | grep -q "^$remote2$"
+ echo "$output" | grep -q "^$remote3$"
+
+ run shiplog_restore_caller_remotes
+ [ "$status" -eq 0 ]
+
+ run git remote
+ [ "$status" -eq 0 ]
+ ! echo "$output" | grep -q "^$remote2$"
+ ! echo "$output" | grep -q "^$remote3$"
+ echo "$output" | grep -q "^$remote1$"
+}
+
+@test "restore is idempotent" {
+ local remote="shimtest-${BATS_TEST_NUMBER}"
+ git remote add "$remote" https://example.invalid/idempotent.git
+ git config "remote.${remote}.fetch" "+refs/heads/*:refs/remotes/${remote}/*"
+
+ local original
+ original="$(git config --get-regexp "^remote\.${remote}\." | sort)"
+
+ shiplog_snapshot_caller_repo_state
+
+ # First restore (no changes)
+ run shiplog_restore_caller_remotes
+ [ "$status" -eq 0 ]
+
+ local after_first
+ after_first="$(git config --get-regexp "^remote\.${remote}\." | sort)"
+ [ "$original" = "$after_first" ]
+
+ # Modify and restore again
+ git config "remote.${remote}.tagOpt" "--tags"
+
+ # Re-snapshot and restore
+ shiplog_snapshot_caller_repo_state
+ git remote set-url "$remote" https://example.invalid/modified.git
+
+ run shiplog_restore_caller_remotes
+ [ "$status" -eq 0 ]
+
+ # Should still have the tagOpt from before second snapshot
+ run git config --get "remote.${remote}.tagOpt"
+ [ "$status" -eq 0 ]
+ [ "$output" = "--tags" ]
+}
+
+@test "restore handles remote with no URL configured" {
+ local remote="shimtest-${BATS_TEST_NUMBER}"
+ git remote add "$remote" https://example.invalid/temp.git
+
+ # Manually remove URL (creates invalid config state)
+ git config --unset-all "remote.${remote}.url" || true
+
+ run shiplog_snapshot_caller_repo_state
+ [ "$status" -eq 0 ]
+
+ # Should handle this gracefully during restore
+ git remote remove "$remote" 2>/dev/null || true
+ git remote add "$remote" https://example.invalid/different.git
+
+ # Restore should detect missing URL and report error
+ run shiplog_restore_caller_remotes
+ # Should fail due to missing URL
+ [ "$status" -ne 0 ]
+ echo "$output" | grep -q "Missing URL"
+}
+
+@test "git_caller executes commands in correct directory" {
+ local remote="shimtest-${BATS_TEST_NUMBER}"
+ export SHIPLOG_TEST_ROOT="$(pwd)"
+
+ # Create a remote
+ git remote add "$remote" https://example.invalid/test.git
+
+ # Use git_caller to list remotes
+ run shiplog_git_caller remote
+ [ "$status" -eq 0 ]
+ echo "$output" | grep -q "^$remote$"
+}
+
+@test "git_caller respects safe.directory config" {
+ export SHIPLOG_TEST_ROOT="$(pwd)"
+
+ # Should be able to run commands with safe.directory set
+ run shiplog_git_caller config --get safe.directory
+ [ "$status" -eq 0 ]
+}
+
+@test "helper_error outputs to stderr" {
+ run shiplog_helper_error "test error message"
+ [ "$status" -eq 1 ]
+ # BATS captures both stdout and stderr in $output for 'run'
+ echo "$output" | grep -q "ERROR: test error message"
+}
+
+@test "helper_error formats multiple arguments" {
+ run shiplog_helper_error "multiple" "args" "test"
+ [ "$status" -eq 1 ]
+ echo "$output" | grep -q "ERROR: multiple args test"
+}
+
+@test "restore_exec detects read-only permission denied" {
+ local temp_repo
+ temp_repo="$(mktemp -d)"
+ (
+ cd "$temp_repo"
+ git init -q
+ git remote add origin https://example.invalid/test.git
+ export SHIPLOG_TEST_ROOT="$temp_repo"
+ export SHIPLOG_FORCE_REMOTE_RESTORE_SKIP=1
+ shiplog_snapshot_caller_repo_state
+
+ # This should trigger the read-only path via the force flag
+ run shiplog_restore_exec "test context" remote add fake https://example.invalid/fake.git
+ [ "$status" -eq 1 ]
+ echo "$output" | grep -q "Skipping remote restore: config is read-only"
+ )
+ rm -rf "$temp_repo"
+}
+
+@test "restore_exec returns success for valid commands" {
+ local remote="shimtest-${BATS_TEST_NUMBER}"
+ export SHIPLOG_TEST_ROOT="$(pwd)"
+
+ run shiplog_restore_exec "test context" remote add "$remote" https://example.invalid/test.git
+ [ "$status" -eq 0 ]
+
+ # Verify remote was added
+ run git remote
+ [ "$status" -eq 0 ]
+ echo "$output" | grep -q "^$remote$"
+}
+
+@test "restore_exec propagates command failures" {
+ export SHIPLOG_TEST_ROOT="$(pwd)"
+
+ # Try to add duplicate remote (should fail)
+ local remote="shimtest-${BATS_TEST_NUMBER}"
+ git remote add "$remote" https://example.invalid/existing.git
+
+ run shiplog_restore_exec "test context" remote add "$remote" https://example.invalid/duplicate.git
+ [ "$status" -eq 2 ]
+ echo "$output" | grep -q "test context"
+}
+
+@test "snapshot handles remotes with dots in name" {
+ local remote="remote.with.dots-${BATS_TEST_NUMBER}"
+ git remote add "$remote" https://example.invalid/dots.git
+
+ run shiplog_snapshot_caller_repo_state
+ [ "$status" -eq 0 ]
+ [[ "${SHIPLOG_ORIG_REMOTES_CONFIG[$remote]}" == *"$remote.url"* ]]
+}
+
+@test "snapshot handles remotes with brackets in name" {
+ local remote="remote[bracket]-${BATS_TEST_NUMBER}"
+ git remote add "$remote" https://example.invalid/bracket.git
+
+ run shiplog_snapshot_caller_repo_state
+ [ "$status" -eq 0 ]
+ [[ "${SHIPLOG_ORIG_REMOTES_CONFIG[$remote]}" == *"url"* ]]
+}
+
+@test "restore handles complex multi-remote scenario" {
+ local remote1="prod-${BATS_TEST_NUMBER}"
+ local remote2="staging-${BATS_TEST_NUMBER}"
+ local remote3="dev-${BATS_TEST_NUMBER}"
+
+ # Setup prod remote with multiple URLs and configs
+ git remote add "$remote1" https://example.invalid/prod-fetch.git
+ git remote set-url --add "$remote1" https://example.invalid/prod-fetch2.git
+ git remote set-url --push "$remote1" ssh://prod-push.invalid/repo.git
+ git config "remote.${remote1}.tagOpt" "--no-tags"
+
+ # Setup staging with custom fetch specs
+ git remote add "$remote2" https://example.invalid/staging.git
+ git config --add "remote.${remote2}.fetch" "+refs/heads/main:refs/remotes/${remote2}/main"
+ git config "remote.${remote2}.prune" "true"
+
+ # Setup dev with simple config
+ git remote add "$remote3" https://example.invalid/dev.git
+
+ # Capture all configs
+ local config_prod config_staging config_dev
+ config_prod="$(git config --get-regexp "^remote\.${remote1}\." | sort)"
+ config_staging="$(git config --get-regexp "^remote\.${remote2}\." | sort)"
+ config_dev="$(git config --get-regexp "^remote\.${remote3}\." | sort)"
+
+ shiplog_snapshot_caller_repo_state
+
+ # Completely mess up the configuration
+ git remote remove "$remote1"
+ git remote remove "$remote2"
+ git remote remove "$remote3"
+ git remote add "$remote1" https://example.invalid/wrong.git
+ git remote add "$remote2" https://example.invalid/wrong.git
+ git remote add extra-remote https://example.invalid/extra.git
+
+ run shiplog_restore_caller_remotes
+ [ "$status" -eq 0 ]
+
+ # Verify all configurations restored correctly
+ local after_prod after_staging after_dev
+ after_prod="$(git config --get-regexp "^remote\.${remote1}\." | sort)"
+ after_staging="$(git config --get-regexp "^remote\.${remote2}\." | sort)"
+ after_dev="$(git config --get-regexp "^remote\.${remote3}\." | sort)"
+
+ [ "$config_prod" = "$after_prod" ]
+ [ "$config_staging" = "$after_staging" ]
+ [ "$config_dev" = "$after_dev" ]
+
+ # Verify extra remote was removed
+ run git remote
+ [ "$status" -eq 0 ]
+ ! echo "$output" | grep -q "^extra-remote$"
+}
+
+@test "restore handles URL with special characters" {
+ local remote="shimtest-${BATS_TEST_NUMBER}"
+ local url="https://user%40name:p%40ss@example.invalid:8080/path/to/repo.git?query=value#fragment"
+
+ git remote add "$remote" "$url"
+
+ local before
+ before="$(git config --get remote.${remote}.url)"
+
+ shiplog_snapshot_caller_repo_state
+
+ git remote set-url "$remote" "https://example.invalid/wrong.git"
+
+ run shiplog_restore_caller_remotes
+ [ "$status" -eq 0 ]
+
+ local after
+ after="$(git config --get remote.${remote}.url)"
+ [ "$before" = "$after" ]
+}
+
+@test "snapshot and restore preserve exact whitespace in config values" {
+ local remote="shimtest-${BATS_TEST_NUMBER}"
+ git remote add "$remote" https://example.invalid/test.git
+ # Add config with trailing/leading spaces (if git allows)
+ git config "remote.${remote}.customKey" "value with spaces"
+
+ local before
+ before="$(git config --get remote.${remote}.customKey)"
+
+ shiplog_snapshot_caller_repo_state
+
+ git config "remote.${remote}.customKey" "different value"
+
+ run shiplog_restore_caller_remotes
+ [ "$status" -eq 0 ]
+
+ local after
+ after="$(git config --get remote.${remote}.customKey)"
+ [ "$before" = "$after" ]
+}
+
+@test "restore clears snapshot state after completion" {
+ local remote="shimtest-${BATS_TEST_NUMBER}"
+ git remote add "$remote" https://example.invalid/test.git
+
+ shiplog_snapshot_caller_repo_state
+ [ "$SHIPLOG_CALLER_REPO_CAPTURED" -eq 1 ]
+
+ run shiplog_restore_caller_remotes
+ [ "$status" -eq 0 ]
+
+ # State should be cleared after restore
+ [ "$SHIPLOG_CALLER_REPO_CAPTURED" -eq 0 ]
+}
+
+@test "restore handles mixed snapshot and manual remote operations" {
+ local remote1="auto-${BATS_TEST_NUMBER}"
+ local remote2="manual-${BATS_TEST_NUMBER}"
+
+ git remote add "$remote1" https://example.invalid/auto.git
+ shiplog_snapshot_caller_repo_state
+
+ # Manually add another remote (not in snapshot)
+ git remote add "$remote2" https://example.invalid/manual.git
+
+ run shiplog_restore_caller_remotes
+ [ "$status" -eq 0 ]
+
+ # remote1 should be preserved, remote2 should be removed
+ run git remote
+ [ "$status" -eq 0 ]
+ echo "$output" | grep -q "^$remote1$"
+ ! echo "$output" | grep -q "^$remote2$"
+}
+
+@test "multiple snapshots can be taken sequentially" {
+ local remote1="first-${BATS_TEST_NUMBER}"
+ local remote2="second-${BATS_TEST_NUMBER}"
+
+ git remote add "$remote1" https://example.invalid/first.git
+ shiplog_snapshot_caller_repo_state
+ local first_count="${#SHIPLOG_ORIG_REMOTE_ORDER[@]}"
+
+ git remote add "$remote2" https://example.invalid/second.git
+ shiplog_snapshot_caller_repo_state
+ local second_count="${#SHIPLOG_ORIG_REMOTE_ORDER[@]}"
+
+ # Second snapshot should overwrite first
+ [ "$first_count" -eq 1 ]
+ [ "$second_count" -eq 2 ]
+}
+
+@test "restore handles remote name with regex metacharacters" {
+ local remote="remote\$special*chars?-${BATS_TEST_NUMBER}"
+
+ # Git may not allow all special chars, but test what it does allow
+ if git remote add "$remote" https://example.invalid/special.git 2>/dev/null; then
+ shiplog_snapshot_caller_repo_state
+ git remote remove "$remote"
+
+ run shiplog_restore_caller_remotes
+ [ "$status" -eq 0 ]
+
+ run git remote
+ [ "$status" -eq 0 ]
+ echo "$output" | grep -qF "$remote"
+ else
+ skip "Git does not support this remote name format"
+ fi
+}
+
+@test "restore maintains remote configuration after failed git operation" {
+ local remote="shimtest-${BATS_TEST_NUMBER}"
+ git remote add "$remote" https://example.invalid/test.git
+ git config "remote.${remote}.important" "true"
+
+ local before
+ before="$(git config --get-regexp "^remote\.${remote}\." | sort)"
+
+ shiplog_snapshot_caller_repo_state
+
+ # Simulate a scenario where restore might partially fail
+ # but should still maintain consistency
+ git remote set-url "$remote" https://example.invalid/changed.git
+
+ run shiplog_restore_caller_remotes
+ [ "$status" -eq 0 ]
+
+ local after
+ after="$(git config --get-regexp "^remote\.${remote}\." | sort)"
+ [ "$before" = "$after" ]
+} |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
test/helpers/common.bash (1)
87-224: Restore logic is solid, but same terrible variable naming.The restoration logic correctly:
- Handles the test harness override (SHIPLOG_FORCE_REMOTE_RESTORE_SKIP)
- Removes unexpected remotes added during test
- Rebuilds original remotes with multi-URL, pushurl, fetch specs
- Handles read-only scenarios gracefully
BUT: Lines 115-130 have the SAME backwards
restore_nocasevariable naming issue asshiplog_restore_exec. Fix both locations when you refactor that mess.test/26_remote_restore.bats (1)
35-59: SAME CRITICAL BUG: Read-only filesystem violation!Test 2 has the EXACT SAME PROBLEM as test 1. You're trying to modify
/work/.git/configwhich is READ-ONLY in CI. Line 37 fails with "Read-only file system".I already told you how to fix this in the previous comment. Create a temp repo, set
SHIPLOG_TEST_ROOTto it, run your test in a subshell, and clean up. Don't make me write it out again—just follow the same pattern as test 3.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
test/26_remote_restore.bats(1 hunks)test/helpers/common.bash(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
test/**/*.bats
📄 CodeRabbit inference engine (AGENTS.md)
Tests that manipulate Git refs must self-clean (e.g.,
git update-ref -d) and generate unique identifiers per test to avoid cross-run collisions
Files:
test/26_remote_restore.bats
🧠 Learnings (2)
📚 Learning: 2025-10-06T00:15:54.479Z
Learnt from: CR
PR: flyingrobots/shiplog#0
File: AGENTS.md:0-0
Timestamp: 2025-10-06T00:15:54.479Z
Learning: Applies to test/**/*.bats : Tests that manipulate Git refs must self-clean (e.g., git update-ref -d) and avoid assuming empty state
Applied to files:
test/26_remote_restore.bats
📚 Learning: 2025-10-15T00:33:11.402Z
Learnt from: CR
PR: flyingrobots/shiplog#0
File: AGENTS.md:0-0
Timestamp: 2025-10-15T00:33:11.402Z
Learning: Applies to test/**/*.bats : Tests that manipulate Git refs must self-clean (e.g., `git update-ref -d`) and generate unique identifiers per test to avoid cross-run collisions
Applied to files:
test/26_remote_restore.bats
🧬 Code graph analysis (1)
test/26_remote_restore.bats (1)
test/helpers/common.bash (4)
shiplog_reset_remote_snapshot_state(48-53)shiplog_git_caller(12-14)shiplog_snapshot_caller_repo_state(55-85)shiplog_restore_caller_remotes(87-224)
🪛 GitHub Actions: Bash Matrix CI
test/helpers/common.bash
[error] 13-13: shiplog_git_caller remote add failed with status 128: Read-only file system when configuring remote in test suite.
test/26_remote_restore.bats
[error] 22-22: not ok 83 restore removes unexpected remotes. Error: could not lock config file .git/config: Read-only file system
[error] 37-37: not ok 84 restore rebuilds multi-url remote configuration. Error: could not lock config file .git/config: Read-only file system
🔇 Additional comments (8)
test/helpers/common.bash (5)
8-10: Global declarations look clean.All three globals are properly used in the snapshot/restore logic. Good that you removed the dead SHIPLOG_TEMP_REMOTE_DIRS variable from the previous review.
12-14: LGTM - Essential wrapper for CI environments.This wrapper correctly adds safe.directory config to prevent dubious ownership errors in containerized test environments.
16-19: LGTM.Simple, correct error helper.
48-53: LGTM.State reset logic is correct.
55-85: LGTM - Snapshot logic is solid.The function correctly:
- Validates the git repository
- Lists and iterates over remotes
- Escapes special characters for regex matching
- Captures all remote config
- Stores in global state
test/26_remote_restore.bats (3)
5-8: LGTM - Proper test isolation.Uses unique remote names per test (
shimtest-${BATS_TEST_NUMBER}) as required by the coding guidelines.
10-17: LGTM - Proper cleanup.Teardown correctly removes test remotes, resets state, and cleans up temp directories.
61-83: FINALLY! A test that doesn't shit all over the read-only filesystem.This test does it RIGHT:
- Creates an isolated temp repository
- Properly sets
SHIPLOG_TEST_ROOTto the temp repo- Uses a subshell to isolate environment variables
- Tests the read-only skip path with
SHIPLOG_FORCE_REMOTE_RESTORE_SKIP- Cleans up properly
THIS is the pattern tests 1 and 2 should follow. Learn from your own code.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test/26_remote_restore.bats(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
test/**/*.bats
📄 CodeRabbit inference engine (AGENTS.md)
Tests that manipulate Git refs must self-clean (e.g.,
git update-ref -d) and generate unique identifiers per test to avoid cross-run collisions
Files:
test/26_remote_restore.bats
🧬 Code graph analysis (1)
test/26_remote_restore.bats (1)
test/helpers/common.bash (6)
shiplog_reset_remote_snapshot_state(48-53)shiplog_git_caller(12-14)shiplog_snapshot_caller_repo_state(55-85)shiplog_restore_caller_remotes(87-224)shiplog_helper_error(16-19)shiplog_restore_exec(21-46)
🪛 GitHub Actions: Bash Matrix CI
test/26_remote_restore.bats
[error] 22-37: Git operations failed due to Read-only file system: could not lock .git/config during remote restore tests (remote add operations to simulate various remotes).
🔇 Additional comments (3)
test/26_remote_restore.bats (3)
19-33: Wrapper usage fixed.All git invocations now go through shiplog_git_caller; the earlier blocker is resolved. Good.
Also applies to: 35-59
61-83: Read-only skip path test is solid.Great isolation with mktemp + SHIPLOG_FORCE_REMOTE_RESTORE_SKIP and explicit cleanup. This is how all mutating tests should isolate.
422-433: Helper coverage looks good.
- helper_error: stderr + status behavior verified.
- restore_exec: success and failure paths validated, including context propagation and exit codes.
Also applies to: 434-443, 445-457, 459-468
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
test/26_remote_restore.bats (1)
11-13: Stop bypassing the wrapper. Use shiplog_git_caller everywhere.You reintroduced bare git calls. That’s how you get “dubious ownership” faceplants in CI. We have a wrapper — use it 100% of the time.
Apply this diff:
@@ - git -C "$SHIPLOG_TEST_ROOT" init -q - git -C "$SHIPLOG_TEST_ROOT" config user.name "Shiplog Tester" - git -C "$SHIPLOG_TEST_ROOT" config user.email "shiplog-tester@example.com" + shiplog_git_caller init -q + shiplog_git_caller config user.name "Shiplog Tester" + shiplog_git_caller config user.email "shiplog-tester@example.com" @@ - git init -q - git config user.name "Shiplog Tester" - git config user.email "shiplog-tester@example.com" + shiplog_git_caller init -q + shiplog_git_caller config user.name "Shiplog Tester" + shiplog_git_caller config user.email "shiplog-tester@example.com" @@ - git init -q - git config user.name "Shiplog Tester" - git config user.email "shiplog-tester@example.com" + shiplog_git_caller init -q + shiplog_git_caller config user.name "Shiplog Tester" + shiplog_git_caller config user.email "shiplog-tester@example.com"As per coding guidelines.
Also applies to: 40-42, 69-71
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
test/26_remote_restore.bats(1 hunks)test/helpers/common.bash(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
test/**/*.bats
📄 CodeRabbit inference engine (AGENTS.md)
Tests that manipulate Git refs must self-clean (e.g.,
git update-ref -d) and generate unique identifiers per test to avoid cross-run collisions
Files:
test/26_remote_restore.bats
🧠 Learnings (2)
📚 Learning: 2025-10-15T00:33:11.402Z
Learnt from: CR
PR: flyingrobots/shiplog#0
File: AGENTS.md:0-0
Timestamp: 2025-10-15T00:33:11.402Z
Learning: Applies to test/**/*.bats : Tests that manipulate Git refs must self-clean (e.g., `git update-ref -d`) and generate unique identifiers per test to avoid cross-run collisions
Applied to files:
test/26_remote_restore.bats
📚 Learning: 2025-10-06T00:15:54.479Z
Learnt from: CR
PR: flyingrobots/shiplog#0
File: AGENTS.md:0-0
Timestamp: 2025-10-06T00:15:54.479Z
Learning: Applies to test/**/*.bats : Tests that manipulate Git refs must self-clean (e.g., git update-ref -d) and avoid assuming empty state
Applied to files:
test/26_remote_restore.bats
🧬 Code graph analysis (1)
test/26_remote_restore.bats (1)
test/helpers/common.bash (6)
shiplog_reset_remote_snapshot_state(48-53)shiplog_snapshot_caller_repo_state(55-85)shiplog_git_caller(12-14)shiplog_restore_caller_remotes(87-224)shiplog_helper_error(16-19)shiplog_restore_exec(21-46)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
test/26_remote_restore.bats (5)
32-59: Kill the pointless nested repos; setup already isolates each test.Drop the mktemp+subshell circus and run in SHIPLOG_TEST_ROOT from setup. Less noise, less flake.
- local temp_repo - temp_repo="$(mktemp -d)" - ( - cd "$temp_repo" - export SHIPLOG_TEST_ROOT="$temp_repo" - export LC_ALL=C - git init -q - git config user.name "Shiplog Tester" - git config user.email "shiplog-tester@example.com" - shiplog_reset_remote_snapshot_state + shiplog_reset_remote_snapshot_state shiplog_snapshot_caller_repo_state shiplog_git_caller remote add "$remote" https://example.invalid/original.git @@ - ) - rm -rf "$temp_repo"Based on learnings.
61-98: Same story: nuke the nested repo noise.Use the repo from setup. Also eliminates yet more bare git calls.
- local temp_repo - temp_repo="$(mktemp -d)" - ( - cd "$temp_repo" - export SHIPLOG_TEST_ROOT="$temp_repo" - export LC_ALL=C - git init -q - git config user.name "Shiplog Tester" - git config user.email "shiplog-tester@example.com" - shiplog_reset_remote_snapshot_state + shiplog_reset_remote_snapshot_state @@ - ) - rm -rf "$temp_repo"Based on learnings.
100-122: Drop the mktemp repo and the chmod stunt; the skip flag is the switch.Rely on setup’s repo; use SHIPLOG_FORCE_REMOTE_RESTORE_SKIP=1 only. Stop twiddling file perms.
- local temp_repo - temp_repo="$(mktemp -d)" - ( - cd "$temp_repo" - export SHIPLOG_TEST_ROOT="$temp_repo" - shiplog_git_caller init -q - shiplog_git_caller remote add origin https://example.invalid/readonly.git - # Container runs as root; force helper to exercise read-only skip path. - export SHIPLOG_FORCE_REMOTE_RESTORE_SKIP=1 - shiplog_snapshot_caller_repo_state - shiplog_git_caller remote remove origin - chmod u-w .git/config - run shiplog_restore_caller_remotes + shiplog_git_caller remote add origin https://example.invalid/readonly.git + # Force helper to exercise read-only skip path. + export SHIPLOG_FORCE_REMOTE_RESTORE_SKIP=1 + shiplog_snapshot_caller_repo_state + shiplog_git_caller remote remove origin + run shiplog_restore_caller_remotes @@ - ) - rm -rf "$temp_repo"Based on learnings.
281-284: Use fixed‑string grep for literal section headers.Regex here is pointless and brittle. Match the exact header text.
- n1="$(grep -nE "^\[remote \"$remote1\"\]" "$SHIPLOG_TEST_ROOT/.git/config" | head -n1 | cut -d: -f1)" - n2="$(grep -nE "^\[remote \"$remote2\"\]" "$SHIPLOG_TEST_ROOT/.git/config" | head -n1 | cut -d: -f1)" - n3="$(grep -nE "^\[remote \"$remote3\"\]" "$SHIPLOG_TEST_ROOT/.git/config" | head -n1 | cut -d: -f1)" + n1="$(grep -nF "[remote \"$remote1\"]" "$SHIPLOG_TEST_ROOT/.git/config" | head -n1 | cut -d: -f1)" + n2="$(grep -nF "[remote \"$remote2\"]" "$SHIPLOG_TEST_ROOT/.git/config" | head -n1 | cut -d: -f1)" + n3="$(grep -nF "[remote \"$remote3\"]" "$SHIPLOG_TEST_ROOT/.git/config" | head -n1 | cut -d: -f1)"
11-13: Stop calling bare git in setup—you're sabotaging CI.Bare
gitcalls bypass safe.directory enforcement. Useshiplog_git_calleror watch the test rot in CI. Lines 11-13 in your setup function are the problem right now.- git -C "$SHIPLOG_TEST_ROOT" init -q - git -C "$SHIPLOG_TEST_ROOT" config user.name "Shiplog Tester" - git -C "$SHIPLOG_TEST_ROOT" config user.email "shiplog-tester@example.com" + shiplog_git_caller init -q + shiplog_git_caller config user.name "Shiplog Tester" + shiplog_git_caller config user.email "shiplog-tester@example.com"Also: You're repeating this same bare git init+config pattern on lines 40-42 and 69-71. Fix those too while you're at it—don't make me file three separate issues for the same rookie mistake.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test/26_remote_restore.bats(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
test/**/*.bats
📄 CodeRabbit inference engine (AGENTS.md)
Tests that manipulate Git refs must self-clean (e.g.,
git update-ref -d) and generate unique identifiers per test to avoid cross-run collisions
Files:
test/26_remote_restore.bats
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
PR: flyingrobots/shiplog#0
File: AGENTS.md:0-0
Timestamp: 2025-10-06T00:15:54.479Z
Learning: Applies to test/**/*.bats : Tests that manipulate Git refs must self-clean (e.g., git update-ref -d) and avoid assuming empty state
Learnt from: CR
PR: flyingrobots/shiplog#0
File: AGENTS.md:0-0
Timestamp: 2025-10-15T00:33:11.402Z
Learning: Applies to test/**/*.bats : Tests that manipulate Git refs must self-clean (e.g., `git update-ref -d`) and generate unique identifiers per test to avoid cross-run collisions
📚 Learning: 2025-10-15T00:33:11.402Z
Learnt from: CR
PR: flyingrobots/shiplog#0
File: AGENTS.md:0-0
Timestamp: 2025-10-15T00:33:11.402Z
Learning: Applies to test/**/*.bats : Tests that manipulate Git refs must self-clean (e.g., `git update-ref -d`) and generate unique identifiers per test to avoid cross-run collisions
Applied to files:
test/26_remote_restore.bats
📚 Learning: 2025-10-06T00:15:54.479Z
Learnt from: CR
PR: flyingrobots/shiplog#0
File: AGENTS.md:0-0
Timestamp: 2025-10-06T00:15:54.479Z
Learning: Applies to test/**/*.bats : Tests that manipulate Git refs must self-clean (e.g., git update-ref -d) and avoid assuming empty state
Applied to files:
test/26_remote_restore.bats
🧬 Code graph analysis (1)
test/26_remote_restore.bats (1)
test/helpers/common.bash (6)
shiplog_reset_remote_snapshot_state(48-53)shiplog_snapshot_caller_repo_state(55-85)shiplog_git_caller(12-14)shiplog_restore_caller_remotes(87-224)shiplog_helper_error(16-19)shiplog_restore_exec(21-46)
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
test/26_remote_restore.bats(1 hunks)test/helpers/common.bash(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
test/**/*.bats
📄 CodeRabbit inference engine (AGENTS.md)
Tests that manipulate Git refs must self-clean (e.g.,
git update-ref -d) and generate unique identifiers per test to avoid cross-run collisions
Files:
test/26_remote_restore.bats
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
PR: flyingrobots/shiplog#0
File: AGENTS.md:0-0
Timestamp: 2025-10-06T00:15:54.479Z
Learning: Applies to test/**/*.bats : Tests that manipulate Git refs must self-clean (e.g., git update-ref -d) and avoid assuming empty state
Learnt from: CR
PR: flyingrobots/shiplog#0
File: AGENTS.md:0-0
Timestamp: 2025-10-15T00:33:11.402Z
Learning: Applies to test/**/*.bats : Tests that manipulate Git refs must self-clean (e.g., `git update-ref -d`) and generate unique identifiers per test to avoid cross-run collisions
📚 Learning: 2025-10-15T00:33:11.402Z
Learnt from: CR
PR: flyingrobots/shiplog#0
File: AGENTS.md:0-0
Timestamp: 2025-10-15T00:33:11.402Z
Learning: Applies to test/**/*.bats : Tests that manipulate Git refs must self-clean (e.g., `git update-ref -d`) and generate unique identifiers per test to avoid cross-run collisions
Applied to files:
test/26_remote_restore.bats
📚 Learning: 2025-10-06T00:15:54.479Z
Learnt from: CR
PR: flyingrobots/shiplog#0
File: AGENTS.md:0-0
Timestamp: 2025-10-06T00:15:54.479Z
Learning: Applies to test/**/*.bats : Tests that manipulate Git refs must self-clean (e.g., git update-ref -d) and avoid assuming empty state
Applied to files:
test/26_remote_restore.bats
🧬 Code graph analysis (1)
test/26_remote_restore.bats (1)
test/helpers/common.bash (6)
shiplog_reset_remote_snapshot_state(48-53)shiplog_git_caller(12-14)shiplog_snapshot_caller_repo_state(55-85)shiplog_restore_caller_remotes(87-224)shiplog_helper_error(16-19)shiplog_restore_exec(21-46)
🔇 Additional comments (1)
test/helpers/common.bash (1)
63-76: The refactor suggestion doesn't match what the tests actually do — tests are sorting values, which contradicts "preserve order".The script found that
test/26_remote_restore.batsexplicitly sorts the output in lines 285, 296 (fetch URLs) and 310, 321 (pushURLs) using pipes tosort. If the goal is to preserve the original order from.git/config, then sorting in the tests defeats that purpose entirely.Your suggested refactor extracts remote names in config file order, but the test comparisons are still sorting the values (fetch, pushurl). Either:
- The sorting in tests is intentional for determinism (test comparison purposes), and "preserve order" doesn't actually matter here, OR
- The sorting in tests is a bug that should be removed, and your refactor is only a partial fix
Which is it? This matters because the review comment needs to address both the helper logic AND the test expectations. Right now, they're at odds.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/helpers/common.bash (1)
327-357: Fix SSH and GPG tmpdir leaks; you're already tracking them in your TODO.The code proves the leak:
- SSH
$tmpdiris created as a local variable at line 330 and only cleaned on error (line 333). On success, it's lost when the function returns, leaving files in /tmp.- The TODO at line 338 you yourself wrote admits this needs tracking.
- GNUPGHOME is exported (line 340) but never cleaned up in
shiplog_cleanup_sandbox_repo().Your cleanup function only handles SHIPLOG_SANDBOX_DIR. Export the SSH tmpdir and clean both SSH tmpdir and GNUPGHOME in the cleanup function, or stop creating temporary signing infrastructure that you don't clean up.
@@ - if [[ "$method" = "ssh" ]]; then - local tmpdir - tmpdir=$(mktemp -d) + if [[ "$method" = "ssh" ]]; then + local tmpdir + tmpdir=$(mktemp -d) @@ - # TODO: Track tmpdir for cleanup + export SHIPLOG_TEST_SSH_TMPDIR="$tmpdir" @@ shiplog_cleanup_sandbox_repo() { cd "$SHIPLOG_TEST_ROOT" if [[ -n "${SHIPLOG_SANDBOX_DIR:-}" && -d "$SHIPLOG_SANDBOX_DIR" ]]; then rm -rf "$SHIPLOG_SANDBOX_DIR" unset SHIPLOG_SANDBOX_DIR fi + if [[ -n "${SHIPLOG_TEST_SSH_TMPDIR:-}" && -d "$SHIPLOG_TEST_SSH_TMPDIR" ]]; then + rm -rf "$SHIPLOG_TEST_SSH_TMPDIR" + unset SHIPLOG_TEST_SSH_TMPDIR + fi + if [[ -n "${GNUPGHOME:-}" && -d "$GNUPGHOME" ]]; then + rm -rf "$GNUPGHOME" + unset GNUPGHOME + fi }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
test/26_remote_restore.bats(1 hunks)test/helpers/common.bash(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
test/**/*.bats
📄 CodeRabbit inference engine (AGENTS.md)
Tests that manipulate Git refs must self-clean (e.g.,
git update-ref -d) and generate unique identifiers per test to avoid cross-run collisions
Files:
test/26_remote_restore.bats
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
PR: flyingrobots/shiplog#0
File: AGENTS.md:0-0
Timestamp: 2025-10-06T00:15:54.479Z
Learning: Applies to test/**/*.bats : Tests that manipulate Git refs must self-clean (e.g., git update-ref -d) and avoid assuming empty state
Learnt from: CR
PR: flyingrobots/shiplog#0
File: AGENTS.md:0-0
Timestamp: 2025-10-15T00:33:11.402Z
Learning: Applies to test/**/*.bats : Tests that manipulate Git refs must self-clean (e.g., `git update-ref -d`) and generate unique identifiers per test to avoid cross-run collisions
📚 Learning: 2025-10-15T00:33:11.402Z
Learnt from: CR
PR: flyingrobots/shiplog#0
File: AGENTS.md:0-0
Timestamp: 2025-10-15T00:33:11.402Z
Learning: Applies to test/**/*.bats : Tests that manipulate Git refs must self-clean (e.g., `git update-ref -d`) and generate unique identifiers per test to avoid cross-run collisions
Applied to files:
test/26_remote_restore.bats
📚 Learning: 2025-10-06T00:15:54.479Z
Learnt from: CR
PR: flyingrobots/shiplog#0
File: AGENTS.md:0-0
Timestamp: 2025-10-06T00:15:54.479Z
Learning: Applies to test/**/*.bats : Tests that manipulate Git refs must self-clean (e.g., git update-ref -d) and avoid assuming empty state
Applied to files:
test/26_remote_restore.bats
🧬 Code graph analysis (1)
test/26_remote_restore.bats (1)
test/helpers/common.bash (6)
shiplog_reset_remote_snapshot_state(47-52)shiplog_git_caller(12-14)shiplog_snapshot_caller_repo_state(54-84)shiplog_restore_caller_remotes(86-238)shiplog_helper_error(16-19)shiplog_restore_exec(21-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: bash-matrix (debian, debian:bookworm-slim, debian, Debian (bookworm))
🔇 Additional comments (10)
test/26_remote_restore.bats (6)
7-16: Per-test repo isolation is finally correct. Keep it.Fresh mktemp repo, wrapper-only git calls, LC_ALL pinned, full teardown. This prevents cross-test contamination and read-only CI headaches.
Based on learnings.
Also applies to: 18-32
246-281: Good: you actually prove section order in .git/config.Header line-number checks catch alphabetized remote listings. Solid.
294-307: LGTM: fetch spec order is preserved (no sort).Comparing get-all output in original sequence is the right assertion.
312-333: LGTM: pushurl order check is correct.Exact sequence via get-all. No masking.
99-116: Read-only skip test is precise.Uses the skip flag and asserts stderr, not stdout. Clean.
507-517: Remote names with dots/brackets: coverage is good and realistic.Nice edge cases; conditional skip for unsupported names is correct.
Also applies to: 519-530
test/helpers/common.bash (4)
12-14: Wrapper is correct and minimal.Forces safe.directory and -C to SHIPLOG_TEST_ROOT. Exactly what the CI needs.
54-76: Snapshot uses fixed‑string filtering. That’s the right call.Dumps all remote.* once and grep -F per-remote avoids regex footguns entirely.
21-45: Good layering in restore_exec (no global state mutation).Case-insensitive detection with proper shopt restore; returns distinguish read-only (1) vs real failure (2).
86-238: Restore logic is careful and idempotent.Removes unexpected remotes, rebuilds from captured lines, validates primary URL, and resets snapshot state only in the orchestrator. Solid.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/helpers/common.bash (1)
295-306: Change default to local sandbox in common.bash line 295.The code currently defaults to network mode (0) when
SHIPLOG_USE_LOCAL_SANDBOXis unset. This contradicts the guidance that tests should default to local sandbox unless explicitly overridden. Whiletest.shexports it with default 1, code that directly sourcescommon.bashwould incorrectly default to 0.- if [[ "${SHIPLOG_USE_LOCAL_SANDBOX:-0}" = "1" ]]; then + if [[ "${SHIPLOG_USE_LOCAL_SANDBOX:-1}" = "1" ]]; then
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
test/26_remote_restore.bats(1 hunks)test/helpers/common.bash(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
test/**/*.bats
📄 CodeRabbit inference engine (AGENTS.md)
Tests that manipulate Git refs must self-clean (e.g.,
git update-ref -d) and generate unique identifiers per test to avoid cross-run collisions
Files:
test/26_remote_restore.bats
🧠 Learnings (3)
📚 Learning: 2025-10-06T00:15:54.479Z
Learnt from: CR
PR: flyingrobots/shiplog#0
File: AGENTS.md:0-0
Timestamp: 2025-10-06T00:15:54.479Z
Learning: Default tests to SHIPLOG_USE_LOCAL_SANDBOX=1; set to 0 only when remote sandbox access is explicitly required
Applied to files:
test/helpers/common.bash
📚 Learning: 2025-10-15T00:33:11.402Z
Learnt from: CR
PR: flyingrobots/shiplog#0
File: AGENTS.md:0-0
Timestamp: 2025-10-15T00:33:11.402Z
Learning: Applies to test/**/*.bats : Tests that manipulate Git refs must self-clean (e.g., `git update-ref -d`) and generate unique identifiers per test to avoid cross-run collisions
Applied to files:
test/26_remote_restore.bats
📚 Learning: 2025-10-06T00:15:54.479Z
Learnt from: CR
PR: flyingrobots/shiplog#0
File: AGENTS.md:0-0
Timestamp: 2025-10-06T00:15:54.479Z
Learning: Applies to test/**/*.bats : Tests that manipulate Git refs must self-clean (e.g., git update-ref -d) and avoid assuming empty state
Applied to files:
test/26_remote_restore.bats
🧬 Code graph analysis (1)
test/26_remote_restore.bats (1)
test/helpers/common.bash (6)
shiplog_reset_remote_snapshot_state(47-52)shiplog_git_caller(12-14)shiplog_snapshot_caller_repo_state(54-84)shiplog_restore_caller_remotes(86-238)shiplog_helper_error(16-19)shiplog_restore_exec(21-45)
🔇 Additional comments (8)
test/26_remote_restore.bats (5)
7-16: Per-test repo isolation and cleanup are finally correct.mktemp repo per test, LC_ALL=C, and env restore in teardown look solid. Nice.
Also applies to: 18-32
58-95: Multi-URL/pushurl/fetch/misc keys: exact-order comparisons are correct.No sorting, using --get-all, and verifying parity post-restore. This is finally bulletproof.
244-279: Order check via config section line numbers: solid.Grepping section headers with -F and comparing line numbers enforces order correctly.
378-404: Unexpected-remotes purge: good coverage, correct assertions.You snapshot, add junk, restore, and assert only the expected remote remains. Clean.
34-36: Delete this dead function. It’s unused noise.escape_remote_regex is not referenced anywhere in this file anymore. Kill it.
-escape_remote_regex() { - printf '%s' "$1" | sed -e 's/[][\\/.\^$*+?{}()|-]/\\&/g' -}Likely an incorrect or invalid review comment.
test/helpers/common.bash (3)
12-14: git wrapper is finally sane.Enforcing -C and safe.directory on every call eliminates the “dubious ownership” trashfire.
54-76: Snapshot logic: fixed-string filter avoids regex footguns.Using a single config_dump and grep -F "remote.${remote}." is the right call.
21-45: Error handling in restore_exec is tight.Correct exit codes (1 read-only, 2 generic), proper nocasematch hygiene, and stderr logging via helper. Ship it.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
test/26_remote_restore.bats(1 hunks)test/helpers/common.bash(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
test/**/*.bats
📄 CodeRabbit inference engine (AGENTS.md)
Tests that manipulate Git refs must self-clean (e.g.,
git update-ref -d) and generate unique identifiers per test to avoid cross-run collisions
Files:
test/26_remote_restore.bats
🧠 Learnings (3)
📚 Learning: 2025-10-06T00:15:54.479Z
Learnt from: CR
PR: flyingrobots/shiplog#0
File: AGENTS.md:0-0
Timestamp: 2025-10-06T00:15:54.479Z
Learning: Default tests to SHIPLOG_USE_LOCAL_SANDBOX=1; set to 0 only when remote sandbox access is explicitly required
Applied to files:
test/helpers/common.bash
📚 Learning: 2025-10-15T00:33:11.402Z
Learnt from: CR
PR: flyingrobots/shiplog#0
File: AGENTS.md:0-0
Timestamp: 2025-10-15T00:33:11.402Z
Learning: Applies to test/**/*.bats : Tests that manipulate Git refs must self-clean (e.g., `git update-ref -d`) and generate unique identifiers per test to avoid cross-run collisions
Applied to files:
test/26_remote_restore.bats
📚 Learning: 2025-10-06T00:15:54.479Z
Learnt from: CR
PR: flyingrobots/shiplog#0
File: AGENTS.md:0-0
Timestamp: 2025-10-06T00:15:54.479Z
Learning: Applies to test/**/*.bats : Tests that manipulate Git refs must self-clean (e.g., git update-ref -d) and avoid assuming empty state
Applied to files:
test/26_remote_restore.bats
🧬 Code graph analysis (1)
test/26_remote_restore.bats (1)
test/helpers/common.bash (6)
shiplog_reset_remote_snapshot_state(47-52)shiplog_git_caller(12-14)shiplog_snapshot_caller_repo_state(54-84)shiplog_restore_caller_remotes(86-238)shiplog_helper_error(16-19)shiplog_restore_exec(21-45)
🔇 Additional comments (2)
test/26_remote_restore.bats (1)
7-16: Test isolation finally done right.Per-test temp repo, LC_ALL, and full cleanup are correct. No bare git; wrapper everywhere. Proceed.
Also applies to: 18-32
test/helpers/common.bash (1)
54-84: Snapshot/restore logic is robust and finally regex-safe.Fixed‑string filtering, deterministic ordering, and centralized state reset are solid. Return codes (0/1/2) are well‑defined and respected. Good work.
Also applies to: 86-238
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
test/26_remote_restore.bats(1 hunks)test/helpers/common.bash(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
test/**/*.bats
📄 CodeRabbit inference engine (AGENTS.md)
Tests that manipulate Git refs must self-clean (e.g.,
git update-ref -d) and generate unique identifiers per test to avoid cross-run collisions
Files:
test/26_remote_restore.bats
🧠 Learnings (3)
📚 Learning: 2025-10-06T00:15:54.479Z
Learnt from: CR
PR: flyingrobots/shiplog#0
File: AGENTS.md:0-0
Timestamp: 2025-10-06T00:15:54.479Z
Learning: Default tests to SHIPLOG_USE_LOCAL_SANDBOX=1; set to 0 only when remote sandbox access is explicitly required
Applied to files:
test/helpers/common.bash
📚 Learning: 2025-10-15T00:33:11.402Z
Learnt from: CR
PR: flyingrobots/shiplog#0
File: AGENTS.md:0-0
Timestamp: 2025-10-15T00:33:11.402Z
Learning: Applies to test/**/*.bats : Tests that manipulate Git refs must self-clean (e.g., `git update-ref -d`) and generate unique identifiers per test to avoid cross-run collisions
Applied to files:
test/26_remote_restore.bats
📚 Learning: 2025-10-06T00:15:54.479Z
Learnt from: CR
PR: flyingrobots/shiplog#0
File: AGENTS.md:0-0
Timestamp: 2025-10-06T00:15:54.479Z
Learning: Applies to test/**/*.bats : Tests that manipulate Git refs must self-clean (e.g., git update-ref -d) and avoid assuming empty state
Applied to files:
test/26_remote_restore.bats
🧬 Code graph analysis (1)
test/26_remote_restore.bats (1)
test/helpers/common.bash (6)
shiplog_reset_remote_snapshot_state(53-58)shiplog_git_caller(12-14)shiplog_snapshot_caller_repo_state(60-90)shiplog_restore_caller_remotes(92-233)shiplog_helper_error(16-19)shiplog_restore_exec(38-51)
🔇 Additional comments (3)
test/helpers/common.bash (2)
21-36: Readonly error sniffer: good extraction and casing guard.Centralizing the heuristics in shiplog_is_readonly_error and restoring nocasematch is the right call. Clean.
290-301: Defaulting to local sandbox is finally sane.SHIPLOG_USE_LOCAL_SANDBOX defaults to 1 here, matching prior guidance and avoiding network flakiness. Ship it.
Based on learnings
test/26_remote_restore.bats (1)
7-16: Per‑test repo isolation and locale pinning: about time.Fresh mktemp repo, safe.directory wrapper, and LC_ALL=C — this fixes CI “dubious ownership” and ordering flake.
As per coding guidelines
Also applies to: 18-32
| @test "snapshot captures remote with special characters in name" { | ||
| local remote="test-remote.with_special-chars-${BATS_TEST_NUMBER}" | ||
| shiplog_reset_remote_snapshot_state | ||
| shiplog_git_caller remote add "$remote" https://example.invalid/special.git | ||
|
|
||
| shiplog_snapshot_caller_repo_state | ||
| local rc=$? | ||
| [ "$rc" -eq 0 ] | ||
| [ "${SHIPLOG_ORIG_REMOTE_ORDER[0]}" = "$remote" ] | ||
| [[ "${SHIPLOG_ORIG_REMOTES_CONFIG[$remote]}" == *"$remote.url"* ]] | ||
|
|
||
| shiplog_git_caller remote remove "$remote" >/dev/null 2>&1 || true | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Stop using glob patterns to “find” a literal key; use a fixed‑string check.
Avoid [[ … == "$remote.url" ]] with metacharacter names; it’s brittle. Check literally.
Apply:
- [[ "${SHIPLOG_ORIG_REMOTES_CONFIG[$remote]}" == *"$remote.url"* ]]
+ printf '%s\n' "${SHIPLOG_ORIG_REMOTES_CONFIG[$remote]}" | grep -qF "remote.${remote}.url "🤖 Prompt for AI Agents
In test/26_remote_restore.bats around lines 167 to 179, the test currently uses
a glob pattern ([[ "${SHIPLOG_ORIG_REMOTES_CONFIG[$remote]}" == *"$remote.url"*
]]) which treats the remote name as a glob and is brittle for names with
metacharacters; replace that check with a fixed‑string substring test (for
example use grep -F -q "$remote.url" <<<
"${SHIPLOG_ORIG_REMOTES_CONFIG[$remote]}" or an equivalent fixed‑string check)
so the presence of the literal "$remote.url" is asserted without globbing.
| @test "restore handles remote name with regex metacharacters" { | ||
| local remote="remote\$special*chars?-${BATS_TEST_NUMBER}" | ||
|
|
||
| if shiplog_git_caller remote add "$remote" https://example.invalid/special.git 2>/dev/null; then | ||
| shiplog_snapshot_caller_repo_state | ||
| shiplog_git_caller remote remove "$remote" | ||
|
|
||
| run shiplog_restore_caller_remotes | ||
| [ "$status" -eq 0 ] | ||
|
|
||
| run shiplog_git_caller remote -v | ||
| [ "$status" -eq 0 ] | ||
| echo "$output" | grep -qxF "$remote" | ||
|
|
||
| run shiplog_git_caller remote get-url "$remote" | ||
| [ "$status" -eq 0 ] | ||
| [ "$output" = "https://example.invalid/special.git" ] | ||
|
|
There was a problem hiding this comment.
Your “regex metachar” test still wouldn’t catch the duplicate-URL bug. Assert no dupes.
Prove there’s exactly one URL entry after restore.
Apply:
@@
run shiplog_git_caller remote get-url "$remote"
[ "$status" -eq 0 ]
[ "$output" = "https://example.invalid/special.git" ]
+
+ # Assert no duplicate remote.$remote.url entries were added
+ run shiplog_git_caller config --get-all remote.${remote}.url
+ [ "$status" -eq 0 ]
+ [ "$output" = "https://example.invalid/special.git" ]🤖 Prompt for AI Agents
In test/26_remote_restore.bats around lines 760 to 777, the test for "restore
handles remote name with regex metacharacters" misses asserting there are no
duplicate URL entries after restore; modify the test to verify the remote URL
list contains exactly one entry for that remote by running the command that
lists URLs (e.g., git remote get-url --all or parsing `shiplog_git_caller remote
-v` output) and assert the output contains the expected URL once (count == 1) —
implement an assertion that counts occurrences of the URL and fails if it is not
exactly one.
| local primary_seen=0 | ||
| local key value | ||
| for line in "${lines[@]}"; do | ||
| key=${line%% *} | ||
| value=${line#* } | ||
| case "$key" in | ||
| "remote.$remote.url") | ||
| if [ "$value" = "$first_url" ] && [ $primary_seen -eq 0 ]; then | ||
| primary_seen=1 | ||
| continue | ||
| fi | ||
| shiplog_restore_exec "Failed to add additional URL for \"$remote\"" remote set-url --add "$remote" "$value" | ||
| case $? in | ||
| 0) ;; | ||
| 1) | ||
| shiplog_reset_remote_snapshot_state | ||
| return 0 | ||
| ;; | ||
| *) return 1 ;; | ||
| esac | ||
| ;; | ||
| "remote.$remote.pushurl") | ||
| shiplog_restore_exec "Failed to add pushurl for \"$remote\"" remote set-url --push --add "$remote" "$value" | ||
| case $? in | ||
| 0) ;; | ||
| 1) | ||
| shiplog_reset_remote_snapshot_state | ||
| return 0 | ||
| ;; | ||
| *) return 1 ;; | ||
| esac | ||
| ;; | ||
| "remote.$remote.fetch") | ||
| shiplog_restore_exec "Failed to restore fetch spec for \"$remote\"" config --local --add "remote.$remote.fetch" "$value" | ||
| case $? in | ||
| 0) ;; | ||
| 1) | ||
| shiplog_reset_remote_snapshot_state | ||
| return 0 | ||
| ;; | ||
| *) return 1 ;; | ||
| esac | ||
| ;; | ||
| *) | ||
| shiplog_restore_exec "Failed to restore $key" config --local --add "$key" "$value" | ||
| case $? in | ||
| 0) ;; | ||
| 1) | ||
| shiplog_reset_remote_snapshot_state | ||
| return 0 | ||
| ;; | ||
| *) return 1 ;; | ||
| esac | ||
| ;; | ||
| esac | ||
| done |
There was a problem hiding this comment.
Literal-compare the config keys; your case patterns break on metacharacter remote names.
Using case patterns with "$remote" lets [], ?, *, etc. be interpreted. For remotes like remote[bracket]-42, "remote.$remote.url" won’t match literally, you’ll hit the default branch and duplicate the primary URL via raw config writes. Fix it by doing exact string comparisons.
Apply:
- local primary_seen=0
- local key value
- for line in "${lines[@]}"; do
- key=${line%% *}
- value=${line#* }
- case "$key" in
- "remote.$remote.url")
- if [ "$value" = "$first_url" ] && [ $primary_seen -eq 0 ]; then
- primary_seen=1
- continue
- fi
- shiplog_restore_exec "Failed to add additional URL for \"$remote\"" remote set-url --add "$remote" "$value"
- case $? in
- 0) ;;
- 1)
- shiplog_reset_remote_snapshot_state
- return 0
- ;;
- *) return 1 ;;
- esac
- ;;
- "remote.$remote.pushurl")
- shiplog_restore_exec "Failed to add pushurl for \"$remote\"" remote set-url --push --add "$remote" "$value"
- case $? in
- 0) ;;
- 1)
- shiplog_reset_remote_snapshot_state
- return 0
- ;;
- *) return 1 ;;
- esac
- ;;
- "remote.$remote.fetch")
- shiplog_restore_exec "Failed to restore fetch spec for \"$remote\"" config --local --add "remote.$remote.fetch" "$value"
- case $? in
- 0) ;;
- 1)
- shiplog_reset_remote_snapshot_state
- return 0
- ;;
- *) return 1 ;;
- esac
- ;;
- *)
- shiplog_restore_exec "Failed to restore $key" config --local --add "$key" "$value"
- case $? in
- 0) ;;
- 1)
- shiplog_reset_remote_snapshot_state
- return 0
- ;;
- *) return 1 ;;
- esac
- ;;
- esac
- done
+ local primary_seen=0
+ local key value
+ for line in "${lines[@]}"; do
+ key=${line%% *}
+ value=${line#* }
+ if [[ "$key" == "remote.$remote.url" ]]; then
+ if [ "$value" = "$first_url" ] && [ $primary_seen -eq 0 ]; then
+ primary_seen=1
+ continue
+ fi
+ shiplog_restore_exec "Failed to add additional URL for \"$remote\"" remote set-url --add "$remote" "$value"
+ case $? in
+ 0) ;;
+ 1) shiplog_reset_remote_snapshot_state; return 0 ;;
+ *) return 1 ;;
+ esac
+ elif [[ "$key" == "remote.$remote.pushurl" ]]; then
+ shiplog_restore_exec "Failed to add pushurl for \"$remote\"" remote set-url --push --add "$remote" "$value"
+ case $? in
+ 0) ;;
+ 1) shiplog_reset_remote_snapshot_state; return 0 ;;
+ *) return 1 ;;
+ esac
+ elif [[ "$key" == "remote.$remote.fetch" ]]; then
+ shiplog_restore_exec "Failed to restore fetch spec for \"$remote\"" config --local --add "remote.$remote.fetch" "$value"
+ case $? in
+ 0) ;;
+ 1) shiplog_reset_remote_snapshot_state; return 0 ;;
+ *) return 1 ;;
+ esac
+ else
+ shiplog_restore_exec "Failed to restore $key" config --local --add "$key" "$value"
+ case $? in
+ 0) ;;
+ 1) shiplog_reset_remote_snapshot_state; return 0 ;;
+ *) return 1 ;;
+ esac
+ fi
+ done📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| local primary_seen=0 | |
| local key value | |
| for line in "${lines[@]}"; do | |
| key=${line%% *} | |
| value=${line#* } | |
| case "$key" in | |
| "remote.$remote.url") | |
| if [ "$value" = "$first_url" ] && [ $primary_seen -eq 0 ]; then | |
| primary_seen=1 | |
| continue | |
| fi | |
| shiplog_restore_exec "Failed to add additional URL for \"$remote\"" remote set-url --add "$remote" "$value" | |
| case $? in | |
| 0) ;; | |
| 1) | |
| shiplog_reset_remote_snapshot_state | |
| return 0 | |
| ;; | |
| *) return 1 ;; | |
| esac | |
| ;; | |
| "remote.$remote.pushurl") | |
| shiplog_restore_exec "Failed to add pushurl for \"$remote\"" remote set-url --push --add "$remote" "$value" | |
| case $? in | |
| 0) ;; | |
| 1) | |
| shiplog_reset_remote_snapshot_state | |
| return 0 | |
| ;; | |
| *) return 1 ;; | |
| esac | |
| ;; | |
| "remote.$remote.fetch") | |
| shiplog_restore_exec "Failed to restore fetch spec for \"$remote\"" config --local --add "remote.$remote.fetch" "$value" | |
| case $? in | |
| 0) ;; | |
| 1) | |
| shiplog_reset_remote_snapshot_state | |
| return 0 | |
| ;; | |
| *) return 1 ;; | |
| esac | |
| ;; | |
| *) | |
| shiplog_restore_exec "Failed to restore $key" config --local --add "$key" "$value" | |
| case $? in | |
| 0) ;; | |
| 1) | |
| shiplog_reset_remote_snapshot_state | |
| return 0 | |
| ;; | |
| *) return 1 ;; | |
| esac | |
| ;; | |
| esac | |
| done | |
| local primary_seen=0 | |
| local key value | |
| for line in "${lines[@]}"; do | |
| key=${line%% *} | |
| value=${line#* } | |
| if [[ "$key" == "remote.$remote.url" ]]; then | |
| if [ "$value" = "$first_url" ] && [ $primary_seen -eq 0 ]; then | |
| primary_seen=1 | |
| continue | |
| fi | |
| shiplog_restore_exec "Failed to add additional URL for \"$remote\"" remote set-url --add "$remote" "$value" | |
| case $? in | |
| 0) ;; | |
| 1) shiplog_reset_remote_snapshot_state; return 0 ;; | |
| *) return 1 ;; | |
| esac | |
| elif [[ "$key" == "remote.$remote.pushurl" ]]; then | |
| shiplog_restore_exec "Failed to add pushurl for \"$remote\"" remote set-url --push --add "$remote" "$value" | |
| case $? in | |
| 0) ;; | |
| 1) shiplog_reset_remote_snapshot_state; return 0 ;; | |
| *) return 1 ;; | |
| esac | |
| elif [[ "$key" == "remote.$remote.fetch" ]]; then | |
| shiplog_restore_exec "Failed to restore fetch spec for \"$remote\"" config --local --add "remote.$remote.fetch" "$value" | |
| case $? in | |
| 0) ;; | |
| 1) shiplog_reset_remote_snapshot_state; return 0 ;; | |
| *) return 1 ;; | |
| esac | |
| else | |
| shiplog_restore_exec "Failed to restore $key" config --local --add "$key" "$value" | |
| case $? in | |
| 0) ;; | |
| 1) shiplog_reset_remote_snapshot_state; return 0 ;; | |
| *) return 1 ;; | |
| esac | |
| fi | |
| done |
1 participant
