Skip to content

fix(operations.git): keep git.repo src in sync with config origin (#1763)#1764

Draft
pascal-cm wants to merge 1 commit into
pyinfra-dev:3.xfrom
pascal-cm:3.x
Draft

fix(operations.git): keep git.repo src in sync with config origin (#1763)#1764
pascal-cm wants to merge 1 commit into
pyinfra-dev:3.xfrom
pascal-cm:3.x

Conversation

@pascal-cm

Copy link
Copy Markdown
Contributor

Fixes #1763

  • Pull request is based on the default branch (3.x at this time)
  • Pull request includes tests for any new/updated operations/facts
  • Pull request includes documentation for any new/updated operations/facts
  • Tests pass (see scripts/dev-test.sh)
  • Type checking & code style passes (see scripts/dev-lint.sh)
  • Pull request title follows the conventional commits format

Comment thread src/pyinfra/operations/git.py Outdated
@pascal-cm pascal-cm marked this pull request as draft May 19, 2026 10:43
@pascal-cm pascal-cm marked this pull request as ready for review May 19, 2026 10:53
@wowi42 wowi42 added operations Issues with operations. new feature labels May 19, 2026

@wowi42 wowi42 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.json uses src="myrepo" (the default the other fixtures use).
  • clone_src_origin_same_url.json uses src="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.

@pascal-cm pascal-cm marked this pull request as draft May 19, 2026 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

new feature operations Issues with operations.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

git.repo does not honor change of src

3 participants