Skip to content

fix: resolve SCP-like submodule URLs via transport.NewEndpoint#514

Draft
mafredri wants to merge 1 commit into
74-git-submodule-supportfrom
mafredri/485-submodule-scp-endpoint
Draft

fix: resolve SCP-like submodule URLs via transport.NewEndpoint#514
mafredri wants to merge 1 commit into
74-git-submodule-supportfrom
mafredri/485-submodule-scp-endpoint

Conversation

@mafredri
Copy link
Copy Markdown
Member

Implements two suggestions from mafredri's review on #485 that have not been picked up yet:

  • Replace url.Parse + the SCP-like guard in ResolveSubmoduleURL with go-git's transport.NewEndpoint. SCP-like parent URLs (git@host:path) now resolve correctly, including the git@host:port:path form. The documented Support SCP-like parent URLs with relative submodules #492 limitation goes away.
  • Cover the new behavior with scpRelativeSibling, scpRelativeChild, scpMultiLevelUp, scpWithPort, and httpsMultiLevelUp cases. badParent and the scpParentWithRelativeSubmodule "expectErr" cases are removed because transport.NewEndpoint("://bad") parses as file:// and the SCP case no longer errors.

Threads addressed:

Verified locally:

  • go test ./git/...: passes (all 9 TestResolveSubmoduleURL subtests pass).
  • go build ./...: clean.
  • golangci-lint run ./git/...: clean.
Implementation plan
1. Replace url.Parse + SCP-guard with transport.NewEndpoint in
   git/git.go::ResolveSubmoduleURL. Use parentEP.String() to
   reconstruct. Drop the limitation comment (no longer applies).
2. Update git/git_test.go::TestResolveSubmoduleURL: drop badParent
   and scpParentWithRelativeSubmodule expectErr cases; add
   scpRelativeSibling, scpRelativeChild, scpMultiLevelUp,
   scpWithPort, httpsMultiLevelUp.
3. Keep scpLikeURLRegex (still used by RedactURL and extractHost).
4. No changes to auth forwarding, SameHost, RedactURL, or
   extractHost.

Out of scope (intentional):

  • AMREM-12 cross-host auth integration test. SameHost is unit-tested already. A higher-level test needs separate host identities for the two httptest.Servers and that is a real piece of work; @bjornrobertsson is already asking for guidance in-thread.
  • AMREM-7 native git.Submodules().Update(). Discussed and parked due to known go-git v5 relative-path bugs.

Generated by Coder Agents on behalf of @mafredri to land mafredri's own review suggestions on #485.

🤖 This PR was created with the help of Coder Agents, and will be reviewed by a human. 🏂🏻

Use go-git's transport.NewEndpoint to parse parent URLs in
ResolveSubmoduleURL. It accepts SCP-like syntax (git@host:path)
natively, including the host:port:path form, so the special-case
guard for SCP URLs and the documented #492 limitation can be
removed.

Test changes:
- Drop badParent (transport.NewEndpoint accepts ://bad as file proto).
- Replace scpParentWithRelativeSubmodule expectErr case with passing
  scpRelativeSibling, plus scpRelativeChild, scpMultiLevelUp,
  scpWithPort, and httpsMultiLevelUp.

Implements suggestions from mafredri review threads
PRRT_kwDOJPSNMM5vANV5 and PRRC_kwDOJPSNMM6oMqCV.
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.

2 participants