fix(git): fix submodule init on workspace restart#518
Merged
Conversation
- Fix swapped git.Open arguments at lines 112 and 136. The storer was
backed by the worktree root and the worktree arg was the .git/
filesystem. This made every workspace restart fail to detect the
existing repo, fall through to CloneContext, and hit
ErrRepositoryAlreadyExists. (AMREM-14)
- Clean up orphaned .git directory when CloneContext fails in
cloneSubmodule. Without this, one failed clone permanently poisons
the submodule directory: the next Stat(".git") takes the
already-cloned fast path and tries to Open a broken repo. (AMREM-16)
- Use openSubmoduleRepo in the already-cloned branch of
cloneSubmodule, removing the duplicate Chroot/NewStorage/Open
sequence. (AMREM-25)
- Rewrite extractHost to delegate to transport.NewEndpoint instead of
reimplementing SCP URL parsing with a separate regex. (AMREM-24)
- Include fetchErr in the "Direct fetch failed" log message so
operators see why the targeted fetch was rejected. (AMREM-22)
- Track warnings in initSubmodules and adjust the final log line so
it does not claim all submodules initialized successfully when
warnings were emitted. (AMREM-23)
- Correct the SameHost doc comment: port is ignored as a
simplification, not to match git credential helpers (which do
include port). (AMREM-27)
- Change "positive integer" to "non-negative integer (0 disables)"
in SubmoduleDepth docs and error messages, matching the actual
guard (n < 0). (AMREM-28)
- Cap SubmoduleDepth at 100 to bound runaway recursion. (AMREM-31)
- Document cross-protocol auth forwarding in submoduleAuthFor as
intentional. go-git rejects incompatible auth at the transport
layer. (AMREM-15)
- Fix test logf to use fmt.Fprintf so format args are expanded and
the warning check is not vacuously true. (AMREM-20)
- Fix TestCloneRepo/AlreadyCloned to produce a repo in the layout
CloneRepo expects (.git/ subdirectory) by cloning first, rather
than using gittest.NewRepo which writes objects at the root.
Skip the subtest when the test case expects the initial clone to
fail. (AMREM-14 test fix)
mafredri
requested review from
bjornrobertsson and
johnstcn
and removed request for
bjornrobertsson
johnstcn
reviewed
May 22, 2026
johnstcn
reviewed
May 22, 2026
johnstcn
approved these changes
May 22, 2026
If billyutil.RemoveAll fails to remove the orphaned .git directory, log a warning so the operator knows the submodule directory may be in a broken state.
mafredri
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Patch PR for #485 (round 2) that addresses the second review pass. Targets the PR's branch.
Changes
P1:
git.Openarguments swapped (AMREM-14)Both calls at lines 112 and 136 passed
(fsStorage, gitDir)when the correct order is(storer-backed-by-.git, worktree-fs). This made every workspace restart fail to detect the existing repo, enter the clone path, and hitErrRepositoryAlreadyExists. TheAlreadyClonedtest is updated to actually clone first (producing the right on-disk layout) instead of usinggittest.NewRepowhich writes objects at the root.P2: Orphaned
.gitblocks retries (AMREM-16)If
CloneContextfails (network timeout, auth error), the.gitdirectory created byMkdirAllpersisted. On the next start,Stat(".git")took the already-cloned fast path and tried toOpena corrupt repo. Now the.gitdirectory is removed on clone failure.P3 fixes
extractHostrewritten to usetransport.NewEndpoint, removing the duplicate SCP regex parser (AMREM-24).cloneSubmodulenow callsopenSubmoduleRepoinstead of duplicating the Chroot/NewStorage/Open sequence (AMREM-25).fetchErrincluded in the "Direct fetch failed" log message (AMREM-22).initSubmodulestracks warnings and adjusts the final log line accordingly (AMREM-23).SameHostdoc corrected: port is ignored as a simplification, not to match git credential helpers (AMREM-27).SubmoduleDepthcapped at 100 (AMREM-31).submoduleAuthFor(AMREM-15).logffixed to usefmt.Fprintfso format args are expanded (AMREM-20).Docs: PR description updated (AMREM-18)
The behavior section now accurately describes the retry-on-every-start semantics.
Contested
AMREM-17 and AMREM-21 re-raise the
./-relative URL resolution claim from DEREM-2 (R1), which was already contested with a native git reproduction showing the current behavior is correct. No change.