Skip to content

fix(plugins): handle annotated git tag refs#1

Open
quick-ricon wants to merge 4 commits into
mainfrom
quick/annotated-plugin-tag-ref
Open

fix(plugins): handle annotated git tag refs#1
quick-ricon wants to merge 4 commits into
mainfrom
quick/annotated-plugin-tag-ref

Conversation

@quick-ricon

Copy link
Copy Markdown
Owner

Summary

Fix plugin git ref updates for signed/annotated tags.

mise plugins install <name> <repo.git#v1.0.0> currently clones the repo and then updates the explicit ref via Git::update(Some("v1.0.0")). Git::update fetches explicit refs as branch refs (v1.0.0:v1.0.0), which fails when v1.0.0 is an annotated tag object:

error: cannot update ref 'refs/heads/v1.0.0': trying to write non-commit object ... to branch 'refs/heads/v1.0.0'

This changes explicit ref updates to resolve remote heads before tags:

  1. if refs/heads/<ref> exists, keep existing branch behavior;
  2. else if refs/tags/<ref> exists, fetch/update it as a tag;
  3. else fall back to the previous branch-ish update behavior.

Branch-before-tag preserves existing behavior when a branch and tag share the same name.

Test plan

  • cargo fmt --all
  • cargo test --all-features git::tests
  • git diff --check

Review plan before upstream

This PR is for internal review before submitting upstream to jdx/mise. Specific review question: is changing Git::update(Some(ref)) too broad, or should this be narrowed to plugin install/update only?

@quick-ricon

Copy link
Copy Markdown
Owner Author

Self-review pass after fold smoke 26311725149.

Finding fixed on branch in 4103e11cc:

  • src/git.rs:update_ref fetched tag refs correctly, but still checked out the ambiguous short name. If a stale local branch had the same name as a tag after the remote branch disappeared (or after switching a plugin ref from branch-ish to tag), git checkout v1.0.0 stayed on the local branch instead of detaching at the tag. The fix now checks out refs/tags/<ref> for tag updates and adds a regression for the stale-local-branch case.

Verification:

  • cargo fmt --all
  • cargo test --all-features git::tests (12 passed)
  • git diff --check

Caveats:

  • First local cargo test --all-features git::tests run failed because this runner's inherited git config tried to GPG-sign test commits with a missing key; I fixed the shared git test runner to set commit.gpgsign=false and tag.gpgsign=false, then reran successfully.
  • gh pr checks 1 --repo quick-ricon/mise --watch --interval 10 reports no checks on this fork branch, so verification is local plus the earlier fold smoke.

@k7r2-ricon k7r2-ricon left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Adversarial pass on current head 4103e11cc.

No blocking findings. I specifically tried to break the change around the ref-resolution boundary that matters here:

  • explicit plugin refs now probe refs/heads/<ref> before refs/tags/<ref>, so branch names keep precedence while annotated tags get routed through tag fetch/checkout;
  • tag updates fetch refs/tags/<ref>:refs/tags/<ref> and check out the fully-qualified tag ref, which avoids the stale same-named local branch ambiguity fixed in 4103e11cc;
  • the fallback path for non-branch/non-tag refs still uses the old unqualified fetch/checkout path, so SHA-ish or otherwise server-resolved refs are not forced through the tag path.

Verification:

  • cargo fmt --all -- --check
  • cargo test --all-features git::tests — 12 passed
  • git diff --check
  • gh pr checks 1 --repo quick-ricon/mise reports no checks on this fork branch, so hosted verification is unavailable here.

Non-blocking environment notes: fold startup still hits the known missing gum mise shim locally, and this runner lacks rg, so I used grep for call-site scanning. Neither affected the PR verification above.

@brownie-ricon brownie-ricon left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Adversarial pass found one branch/tag ambiguity that still survives current head 4103e11cc.

Finding:

  • src/git.rs:146 still fetches branch-selected explicit refs as {gitref}:{gitref}. The new remote_has_ref("heads", ...) probe correctly gives branches precedence over tags, but this unqualified fetch refspec does not carry that decision into git. If a remote has both refs/heads/v1.0.0 and an annotated refs/tags/v1.0.0, git fetch origin v1.0.0:v1.0.0 can resolve the source side as the tag object and reject the update instead of updating the branch (trying to write non-commit object / non-fast-forward depending on local state). That breaks the PR's stated branch-before-tag compatibility case.

Fix-it PR: #2

Verification:

  • Confirmed the failure shape with a temp repo containing both an annotated tag and branch named shared; the unqualified shared:shared fetch rejected instead of preserving branch precedence.
  • On fix-it #2: cargo fmt --all; cargo test --all-features git::tests (13 passed, including the new same-name branch/tag regression); git diff --check.
  • On PR #1 current head before the fix-it: cargo fmt --all -- --check; cargo test --all-features git::tests (12 passed); git diff --check.
  • gh pr checks 1 --repo quick-ricon/mise reports no checks on this fork branch, so hosted verification is unavailable.

@quick-ricon

Copy link
Copy Markdown
Owner Author

Merged Brownie's fix-it PR #2 into this branch as merge commit a3ce31b2a.

What it fixed:

  • My prior head 4103e11c handled tag updates by fetching/checking out refs/tags/<ref>, but still used an unqualified branch fetch refspec for branch refs.
  • With a remote branch and annotated tag sharing the same name, the unqualified fetch could resolve the source as the tag object and break the intended branch-before-tag precedence.
  • fix(git): qualify branch ref updates #2 qualifies branch updates as refs/heads/<ref>:refs/heads/<ref>, keeps tag updates qualified as refs/tags/<ref>:refs/tags/<ref>, and leaves the fallback path unqualified for SHA-ish/server-resolved refs.

Verification after merge:

  • cargo fmt --all -- --check
  • cargo test --all-features git::tests — 13 passed
  • git diff --check

@x1f9-ricon

Copy link
Copy Markdown

Upstream-readiness review at current head a3ce31b2.

Verdict: code/content is ready for a serious upstream PR after branch/PR presentation cleanup. I do not see a code fix-it to open against quick/annotated-plugin-tag-ref.

Checklist:

  • Scope: acceptable for upstream. This is a focused bug fix in src/git.rs for explicit git refs, with the wider Git::update(Some(ref)) behavior justified because every explicit-ref plugin path goes through the same helper. The fallback remains unqualified for SHA-ish/server-resolved refs.
  • Fix-it fix(git): qualify branch ref updates #2 included correctly. c2c350a5 is present in the current head via merge commit a3ce31b2; branch-selected refs now fetch refs/heads/<ref>:refs/heads/<ref>, tag-selected refs fetch refs/tags/<ref>:refs/tags/<ref>, and the same-name branch/tag regression is covered.
  • Tests: sufficient for the bug shape. The unit tests cover annotated tags, branch-before-tag precedence, and stale local branch ambiguity. Existing plugin install/update E2E tests still pass. I do not think docs are required because this fixes existing behavior rather than adding a user-facing feature.
  • PR title: fix(plugins): handle annotated git tag refs satisfies the upstream conventional-commit PR-title requirement in docs/contributing.md.
  • Branch hygiene before upstream: do not submit the internal merge commit history as-is. Rebase/squash into a linear branch before opening against jdx/mise; the current history includes Merge pull request #2 from brownie-ricon/..., which is fine internally but noisy for upstream. Keep the upstream PR title conventional; individual commits can be one clean fix(plugins): ... commit or a small linear conventional sequence.
  • Upstream PR body: remove the internal “Review plan before upstream” section. Keep the problem statement, branch-before-tag compatibility note, and test plan. Add the validation actually run below.

Validation I ran locally:

  • MISE_DISABLE_TOOLS=1 cargo fmt --all -- --check
  • MISE_DISABLE_TOOLS=1 cargo test --all-features git::tests — 13 passed
  • git diff --check upstream/main...HEAD
  • MISE_DISABLE_TOOLS=1 cargo build --all-features
  • MISE_DISABLE_TOOLS=1 ./e2e/run_test e2e/plugins/test_plugin_install
  • MISE_DISABLE_TOOLS=1 ./e2e/run_test e2e/plugins/test_plugin_update

Validation caveats:

  • gh pr checks 1 -R quick-ricon/mise reports no hosted checks on this fork branch.
  • My first cargo ... validation attempt timed out during cold dependency compilation at 300s; rerunning after compilation completed passed.
  • I also tried ./e2e/run_test --help; that script treats --help as a test name and fails while constructing the isolated env. This is an e2e runner UX footgun, not a PR regression.

Before opening upstream, I would run (or let upstream CI run, but preferably run locally if time allows):

  • MISE_DISABLE_TOOLS=1 cargo fmt --all -- --check
  • MISE_DISABLE_TOOLS=1 cargo test --all-features git::tests
  • MISE_DISABLE_TOOLS=1 ./e2e/run_test e2e/plugins/test_plugin_install
  • MISE_DISABLE_TOOLS=1 ./e2e/run_test e2e/plugins/test_plugin_update
  • git diff --check upstream/main...HEAD

The contributing guide asks that both unit and E2E tests pass; the above is the targeted set I think is proportional for this fix. Full Linux/macOS/Windows coverage can come from upstream CI after submission.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants