fix(operations.git): keep git.repo src in sync with config origin (#1763)#1764
fix(operations.git): keep git.repo src in sync with config origin (#1763)#1764pascal-cm wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
1. Idempotency regression (main concern). The set-url is emitted unconditionally for every existing repo on every run. pyinfra cannot know that re-setting the same URL is a no-op, so git.repo now always reports as changed/Success. This defeats the recently added skip-pull / "already up to date" no-op behavior. The smell is visible in the diff: up_to_date.json now has a non-empty commands list while still asserting noop_description (the operation both calls host.noop() and yields a command). The test harness checks the two independently, so it passes while the runtime behavior is contradictory.
The fix should be conditional. The existing GitConfig fact already exposes remote.origin.url (git config --local -l), which fits the fact-driven, idempotent style of the rest of the operation:
git_config = host.get_fact(GitConfig, repo=dest)
current_origin = (git_config.get("remote.origin.url") or [None])[0]
if current_origin != src:
git_commands.append(StringCommand("remote", "set-url", "origin", QuoteString(src)))2. Fails on repos without an origin remote. git remote set-url origin <url> errors with "No such remote 'origin'" when origin does not exist (a checkout pyinfra did not clone, or one using a different remote name). git.repo previously worked on such adopted repos for branch/pull, so this is a robustness regression. Gating on the GitConfig fact (only emit when remote.origin.url exists and differs) also resolves this.
3. Fact ordering vs skip-pull. Facts are gathered before commands execute, so GitRemoteBranchCommit queries the old origin URL. When src genuinely changes, the skip-pull comparison can match the old remote tip and wrongly skip the pull, so #1763 is only partially fixed on that path. A conditional set-url does not fully solve this; ideally, when origin changes, skip-pull should be forced off.
4. New fixture names look swapped and do not test the claimed cases.
clone_src_origin_changed_url.jsonusessrc="myrepo"(the default the other fixtures use).clone_src_origin_same_url.jsonusessrc="changedrepo".
The names are inverted relative to content, and neither exercises the "origin already equals src" path (the implementation never skips). With a conditional fix, add a fixture where GitConfig reports remote.origin.url=<src> asserting no set-url is produced, and one where it differs asserting it is.
The fix is correct in intent, but the unconditional implementation regresses idempotency and breaks on repos lacking an origin remote. A GitConfig-gated conditional is a small change that resolves both.
Fixes #1763
3.xat this time)scripts/dev-test.sh)scripts/dev-lint.sh)