Skip to content

fix(git): address review feedback on submodule support (patch for #485)#516

Merged
bjornrobertsson merged 3 commits into
74-git-submodule-supportfrom
74-git-submodule-support-amend-review
May 21, 2026
Merged

fix(git): address review feedback on submodule support (patch for #485)#516
bjornrobertsson merged 3 commits into
74-git-submodule-supportfrom
74-git-submodule-support-amend-review

Conversation

@mafredri
Copy link
Copy Markdown
Member

Patch PR for #485 that addresses the Coder Agents review feedback on the submodule support. Targets the PR's branch rather than main so the changes ride along when #485 merges.

Changes by area

Submodule credential handling

  • ResolveSubmoduleURL strips userinfo from the parent endpoint before returning. Without this, a parent URL like https://token@github.com/org/repo re-emits its credentials into every resolved submodule URL through Endpoint.String. The password is always stripped; the user portion is preserved for ssh:// endpoints, where it is the SSH login name rather than a credential.
  • initSubmodules now tracks the parent URL and auth that actually reached the current recursion level, and recursive calls use the auth forwarded at this level as the new parent auth. The previous recursion copied opts wholesale, so an auth-withheld step still left opts.RepoAuth populated for the next level. That allowed two-hop credential exfiltration when the nested submodule matched the malicious host.
  • A new submoduleAuthFor helper centralizes the SameHost gating decision.

Submodule lifecycle

  • CloneRepo runs initSubmodules even when the parent repo already exists on disk. A transient submodule init failure used to permanently leave the workspace without submodule content because subsequent runs short-circuited on the existing parent.
  • The "already cloned" path now chroots to .git before constructing the storage, matching the fresh-clone path. The previous layout passed the worktree root as git storage and would fail with a misleading "repository does not exist" error on any partial-prior-run recovery.
  • Nested-submodule recursion no longer relies on sub.Repository(), which requires the submodule to be registered in the parent's .git/config via sub.Init(). The custom clone path bypasses sub.Init, so sub.Repository() always returned ErrSubmoduleNotInitialized and the nested recursion silently no-op'd. A new openSubmoduleRepo helper opens the on-disk repo directly.
  • The latent nil-pointer path on ErrRepositoryAlreadyExists is gone because the fresh-clone branch is only entered when .git did not exist, removing the prior bare !=err guard.
  • The "already cloned" checkout error now includes the expected commit hash, matching the fresh-clone error.

Other

  • Submodule clones propagate opts.Depth. The expected-commit fetch-by-hash fallback already deepens when needed.
  • The submodule loop checks ctx.Done() at the top of each iteration so cancellation lands promptly between submodules.
  • Fetch error checks use errors.Is(err, git.NoErrAlreadyUpToDate) instead of a bare !=, consistent with the rest of the file.
  • SameHost documents that the port is ignored on purpose, matching git's credential-helper convention.
  • Errors when opening or listing a nested submodule worktree are logged before continuing instead of being silently swallowed.

Naming and parsing

  • Options.GitCloneSubmodules renamed to Options.GitCloneSubmoduleDepth so the int reads as a recursion depth rather than a count of submodules. The env var and flag names are unchanged.
  • SubmoduleDepth.Set parses the integer from the trimmed/lowered string, so values like " 5 " parse correctly.

Tests

  • TestResolveSubmoduleURL covers credential-stripping for HTTPS, the ssh:// user-preservation, the git:// scheme, and the previously unused expectErr field (with an unparseable parent URL).
  • TestEnvOptionParsing moves the submodule-depth cases out of the bool subtest into a submodule depth group and adds a whitespace case.
  • New internal-package tests for submoduleAuthFor and the credential-chain invariant.
  • gittest.CreateGitServerWithSubmodule inlined into its single caller.

One finding contested

DEREM-2 claims ResolveSubmoduleURL mishandles ./-relative URLs and that the test TestResolveSubmoduleURL/relativeChild asserts the wrong value. I tested this against native git:

$ # parent URL set to https://example.com/org/main.git
$ cat .gitmodules
[submodule "extras"]
        path = extras
        url = ./extras/tool.git

$ git submodule init && git config submodule.extras.url
https://example.com/org/main.git/extras/tool.git

go-git resolves the same way (submodule.go:152: path.Join(rootEndpoint.Path, moduleEndpoint.Path)). The current code matches both. The reviewer's claim that ./ is relative to "the directory containing the repo" rather than "the repo path" does not match git's behavior. No change.

Findings addressed
ID Severity Status
DEREM-1 P1 Fixed
DEREM-2 P2 Contested (reviewer claim contradicts git's behavior)
DEREM-3 P3 Fixed
DEREM-4 Nit Fixed
DEREM-5 P2 Fixed
DEREM-6 P2 Fixed
DEREM-7 P2 Fixed
DEREM-8 P2 Fixed
DEREM-9 P2 Fixed
DEREM-10 P2 Partially addressed (auth gating covered; full retry-chain coverage deferred)
DEREM-11 P3 Fixed
DEREM-12 P3 Documented as intentional
DEREM-13 P3 Fixed
DEREM-14 P3 Fixed (Go field rename only; env var unchanged)
DEREM-15 P3 Fixed
DEREM-16 P3 Fixed
DEREM-17 P3 Fixed
DEREM-18 Nit Fixed
DEREM-19 Nit Fixed
AMREM-2 open Covered for ssh:// and git:// schemes
AMREM-6 open Helper inlined

This PR was prepared by Coder Agents on behalf of @mafredri.

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

mafredri added 3 commits May 21, 2026 14:13
…Depth.Set

- Rename Options.GitCloneSubmodules to Options.GitCloneSubmoduleDepth to
  align with the internal SubmoduleDepth type and make the int's meaning
  (recursion depth, not count) self-evident. Env var and flag names are
  unchanged so existing user configuration keeps working.
- Use the trimmed/lowered value when parsing the integer in
  SubmoduleDepth.Set so that values like " 5 " parse correctly.
- Move the GIT_CLONE_SUBMODULES tests out of the 'bool' subtest group
  into a sibling 'submodule depth' group; add a whitespace case.

Addresses DEREM-14, DEREM-18, DEREM-19 on #485.
This commit reworks the submodule path in git/git.go to address a
cluster of issues found in review. The changes are correlated so they
land together.

Credential handling
-------------------
- ResolveSubmoduleURL now strips userinfo from the parent endpoint
  before returning. Without this, a parent URL like
  https://token@github.com/org/repo would re-emit its credentials into
  every resolved submodule URL (via go-git's Endpoint.String). The
  password is always stripped; the user portion is preserved only for
  ssh:// endpoints where it is the SSH login name rather than a
  credential. (DEREM-7)
- initSubmodules now tracks the parent URL and auth that reached the
  current recursion level, and nested recursion uses the auth that was
  forwarded at this level (which may be nil) as the new parent auth.
  Previously the recursive call copied opts wholesale, so an auth-
  withheld step still left opts.RepoAuth populated for the next level,
  allowing two-hop credential exfiltration when the nested submodule
  matched the malicious host. (DEREM-1)
- A new submoduleAuthFor helper centralizes the SameHost gating
  decision so the same rule applies to every clone and fetch.

Submodule lifecycle
-------------------
- CloneRepo now invokes initSubmodules even when the parent repo
  already exists on disk. A transient submodule init failure used to
  permanently leave the workspace without submodule content because
  subsequent runs short-circuited on the existing parent. (DEREM-6)
- The 'already cloned' path now chroots to '.git' before constructing
  the filesystem storage, matching the fresh-clone path. The previous
  layout passed the worktree root as git storage and would fail with
  a misleading 'repository does not exist' error on any partial-prior-
  run recovery. (DEREM-5)
- The 'already cloned' checkout error now includes the expected commit
  hash, matching the fresh-clone error. (DEREM-15)
- Nested-submodule recursion no longer relies on sub.Repository(),
  which requires the submodule to be registered in the parent's
  .git/config via sub.Init(). The custom clone path bypasses sub.Init,
  so sub.Repository() always returned ErrSubmoduleNotInitialized and
  nested recursion silently no-op'd. The new helper openSubmoduleRepo
  opens the on-disk repo we just wrote. (DEREM-8)
- The latent nil-pointer path on ErrRepositoryAlreadyExists is gone
  because the fresh-clone branch is only entered when '.git' did not
  exist, removing the prior bare !=err guard. (DEREM-11)

Other polish
-------------
- Submodule clones now propagate opts.Depth. The expected-commit
  fetch-by-hash fallback already deepens when needed. (DEREM-9)
- The submodule loop now checks ctx.Done() at the top of each
  iteration so cancellation lands promptly between submodules.
  (DEREM-13)
- Fetch error checks use errors.Is(err, git.NoErrAlreadyUpToDate)
  instead of a bare !=, consistent with the rest of the file. (DEREM-3)
- Errors when opening or listing the nested submodule worktree are now
  logged before continuing instead of being silently swallowed.
  (DEREM-16)
- Removed two unnecessary 'tc := tc' captures in the test file. Go
  1.24 makes per-iteration loop variable scoping the default. (DEREM-4)
- TestResolveSubmoduleURL now exercises the credential-stripping path,
  ssh:// and git:// parent URLs, and a parent URL that fails parsing,
  covering both the previously unused expectErr field and the schemes
  requested in the earlier review. (DEREM-17, AMREM-2)

Addresses DEREM-1, DEREM-3, DEREM-4, DEREM-5, DEREM-6, DEREM-7,
DEREM-8, DEREM-9, DEREM-11, DEREM-13, DEREM-15, DEREM-16, DEREM-17, and
AMREM-2 on #485.
Add an internal-package test for submoduleAuthFor and the credential-chain
invariant: once auth is withheld at one level of submodule recursion,
deeper levels must keep it nil even when their hosts match.

Inline gittest.CreateGitServerWithSubmodule into its single caller in
the integration test, and drop the helper. The integration test now
builds its parent and submodule servers directly, which keeps the
setup visible in the test it serves.
@mafredri mafredri requested review from bjornrobertsson and johnstcn and removed request for bjornrobertsson May 21, 2026 14:47
@bjornrobertsson bjornrobertsson marked this pull request as ready for review May 21, 2026 15:34
@bjornrobertsson bjornrobertsson merged commit 4d93f13 into 74-git-submodule-support May 21, 2026
@bjornrobertsson bjornrobertsson deleted the 74-git-submodule-support-amend-review branch May 21, 2026 15:35
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