Skip to content
Open
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
81b6fd7
Implement submodule initialization in CloneRepo
bjornrobertsson Dec 9, 2025
5de553d
Add GitCloneSubmodules option for cloning
bjornrobertsson Dec 9, 2025
adbdccf
Fix typo in environment variables documentation entry
bjornrobertsson Dec 9, 2025
18384d5
Fix submodule handling in git
bjornrobertsson Dec 9, 2025
4d3e529
Fix submodule handling in README
bjornrobertsson Dec 9, 2025
e183aff
Update README.md
bjornrobertsson Dec 9, 2025
2d1df11
Add ResolveSubmoduleURLForTest function
bjornrobertsson Dec 9, 2025
5407a5b
Add tests for ResolveSubmoduleURL and CloneOptions
bjornrobertsson Dec 9, 2025
463fd91
Add tests for new environment variables in CLI
bjornrobertsson Dec 9, 2025
04f798b
make gen
bjornrobertsson Dec 11, 2025
db5e40d
Rename ResolveSubmoduleURLForTest to ResolveSubmoduleURL
bjornrobertsson Jan 22, 2026
223d3e4
Add integration test for git submodules
bjornrobertsson Jan 22, 2026
5c59408
Change GitCloneSubmodules to accept depth (true/false/integer)
bjornrobertsson Jan 23, 2026
8134964
Add tests for submodule depth values
bjornrobertsson Jan 23, 2026
442477e
Remove submodule section from README (belongs in release notes)
bjornrobertsson Jan 23, 2026
fb31cac
Fix typo: dev.zaure.com -> dev.azure.com
bjornrobertsson Jan 23, 2026
68ace4b
Fix formatting (gofmt alignment)
bjornrobertsson Jan 23, 2026
149ae9a
Fix git_test.go: use SubmoduleDepth int instead of Submodules bool
bjornrobertsson Jan 23, 2026
3f81838
Update golden file for CLI output test
bjornrobertsson Jan 23, 2026
ea25253
Merge branch 'main' into 74-git-submodule-support
bjornrobertsson Feb 3, 2026
cb11759
Fix: assign repo from CloneContext (was discarded after merge)
bjornrobertsson Feb 3, 2026
76138b0
Merge branch 'main' into 74-git-submodule-support
johnstcn Feb 6, 2026
79d1fc5
Merge branch 'main' into 74-git-submodule-support
johnstcn Feb 6, 2026
92783ef
Add RedactURL to prevent credential leakage in logs
bjornrobertsson Feb 17, 2026
92f5137
Security: only forward auth to submodules on same host
bjornrobertsson Feb 17, 2026
6f87394
Improve submodule test: create proper gitlink and verify clone
bjornrobertsson Feb 17, 2026
ad64adf
Document SCP-like URL limitation for relative submodules
bjornrobertsson Feb 18, 2026
334ade4
Update issue link to #492
bjornrobertsson Feb 18, 2026
e371318
Fix RedactURL: avoid URL-encoding of *** characters
bjornrobertsson Feb 18, 2026
afcce7f
Fix RedactURL: check SCP-like URLs before url.Parse
bjornrobertsson Feb 18, 2026
b82aee0
Fix RedactURL: handle IPv6 URLs correctly
bjornrobertsson Feb 18, 2026
d6ec3d5
Merge branch 'main' into 74-git-submodule-support
bjornrobertsson Apr 23, 2026
f711af5
fix: resolve SCP-like submodule URLs via transport.NewEndpoint (#514)
mafredri May 21, 2026
4d93f13
fix(git): address review feedback on submodule support (patch for #48…
mafredri May 21, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -118,3 +118,10 @@ On macOS or Windows systems, we recommend using a VM or the provided `.devcontai
- `test`: Runs tests.
- `test-registry`: Stands up a local registry for caching images used in tests.
- `docs/env-variables.md`: Updated the [environment variables documentation](./docs/env-variables.md).

**Submodule Handling Fix**

An issue concerning git's submodule handling has been resolved through iterative refinements. This fix ensures robust submodule cloning and URL resolution without relying on the calls to the git binary (current fallback).
```
ENVBUILDER_GIT_CLONE_SUBMODULES=true
Comment thread
mafredri marked this conversation as resolved.
Outdated
```
1 change: 1 addition & 0 deletions docs/env-variables.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
| `--git-clone-depth` | `ENVBUILDER_GIT_CLONE_DEPTH` | | The depth to use when cloning the Git repository. |
| `--git-clone-single-branch` | `ENVBUILDER_GIT_CLONE_SINGLE_BRANCH` | | Clone only a single branch of the Git repository. |
| `--git-clone-thinpack` | `ENVBUILDER_GIT_CLONE_THINPACK` | `true` | Git clone with thin pack compatibility enabled, ensuring that even when thin pack compatibility is activated,it will not be turned on for the domain dev.zaure.com. |
| `--git-clone-submodules` | `ENVBUILDER_GIT_CLONE_SUBMODULES` | | Recursively clone Git submodules after cloning the repository. |
Comment thread
mafredri marked this conversation as resolved.
Outdated
| `--git-username` | `ENVBUILDER_GIT_USERNAME` | | The username to use for Git authentication. This is optional. |
| `--git-password` | `ENVBUILDER_GIT_PASSWORD` | | The password to use for Git authentication. This is optional. |
| `--git-ssh-private-key-path` | `ENVBUILDER_GIT_SSH_PRIVATE_KEY_PATH` | | Path to an SSH private key to be used for Git authentication. If this is set, then GIT_SSH_PRIVATE_KEY_BASE64 cannot be set. |
Expand Down
284 changes: 283 additions & 1 deletion git/git.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,17 @@ import (
"fmt"
"io"
"net"
"net/url"
"os"
"path"
"strings"

"github.com/coder/envbuilder/options"

giturls "github.com/chainguard-dev/git-urls"
"github.com/go-git/go-billy/v5"
"github.com/go-git/go-git/v5"
"github.com/go-git/go-git/v5/config"
"github.com/go-git/go-git/v5/plumbing"
"github.com/go-git/go-git/v5/plumbing/cache"
"github.com/go-git/go-git/v5/plumbing/protocol/packp/capability"
Expand All @@ -41,6 +44,7 @@ type CloneRepoOptions struct {
Depth int
CABundle []byte
ProxyOptions transport.ProxyOptions
Submodules bool
}

// CloneRepo will clone the repository at the given URL into the given path.
Expand Down Expand Up @@ -119,7 +123,7 @@ func CloneRepo(ctx context.Context, logf func(string, ...any), opts CloneRepoOpt
return false, nil
}

_, err = git.CloneContext(ctx, gitStorage, fs, &git.CloneOptions{
repo, err = git.CloneContext(ctx, gitStorage, fs, &git.CloneOptions{
URL: parsed.String(),
Auth: opts.RepoAuth,
Progress: opts.Progress,
Expand All @@ -136,6 +140,15 @@ func CloneRepo(ctx context.Context, logf func(string, ...any), opts CloneRepoOpt
if err != nil {
return false, fmt.Errorf("clone %q: %w", opts.RepoURL, err)
}

Comment thread
mafredri marked this conversation as resolved.
// Initialize submodules if requested
if opts.Submodules {
err = initSubmodules(ctx, logf, repo, opts)
if err != nil {
return true, fmt.Errorf("init submodules: %w", err)
}
}
Comment thread
mafredri marked this conversation as resolved.
Outdated
Comment thread
mafredri marked this conversation as resolved.
Outdated

return true, nil
}

Expand Down Expand Up @@ -361,6 +374,7 @@ func CloneOptionsFromOptions(logf func(string, ...any), options options.Options)
ThinPack: options.GitCloneThinPack,
Depth: int(options.GitCloneDepth),
CABundle: caBundle,
Submodules: options.GitCloneSubmodules,
}

cloneOpts.RepoAuth = SetupRepoAuth(logf, &options)
Expand Down Expand Up @@ -418,3 +432,271 @@ func ProgressWriter(write func(line string, args ...any)) io.WriteCloser {
done: done,
}
}

// resolveSubmoduleURL resolves a potentially relative submodule URL against the parent repository URL
Comment thread
mafredri marked this conversation as resolved.
Outdated
// ResolveSubmoduleURLForTest is exported for testing resolveSubmoduleURL logic
func ResolveSubmoduleURLForTest(parentURL, submoduleURL string) (string, error) {
Comment thread
mafredri marked this conversation as resolved.
Outdated
// If the submodule URL is absolute (contains ://) or doesn't start with ./ or ../, return it as-is
if strings.Contains(submoduleURL, "://") || (!strings.HasPrefix(submoduleURL, "../") && !strings.HasPrefix(submoduleURL, "./")) {
return submoduleURL, nil
}

// Parse the parent URL
parentParsed, err := url.Parse(parentURL)
Comment thread
mafredri marked this conversation as resolved.
Outdated
if err != nil {
return "", fmt.Errorf("parse parent URL: %w", err)
}

// For relative URLs, we need to resolve them against the parent's path
// The parent path represents a repository (like a file in filesystem terms)
// So ../something means "sibling repository"
parentPath := strings.TrimSuffix(parentParsed.Path, "/")

// Split the submodule URL into components
// and manually walk up the directory tree for each ../
currentPath := parentPath
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P3 [ENVB-9] ResolveSubmoduleURL ../ resolution differs from git's native algorithm by one path level.

The code starts resolution at the full parent path (/org/main.git) and uses path.Dir to strip the file component on the first ... Git's relative_url strips the last component first (getting the directory), then processes each ../ as an additional directory-up.

Code:  ../deps/lib.git from /org/main.git → /org/deps/lib.git
Git:   ../deps/lib.git from /org/main.git → /deps/lib.git

The discrepancy primarily affects HTTPS-format parent URLs with relative submodule references. For SCP-format remotes, go-git's endpoint normalization coincidentally compensates. Since .gitmodules authored by git submodule add against HTTPS remotes rarely produces relative URLs that work across path depths, the practical impact is limited but real. (Kite)

🤖

relativeParts := strings.Split(submoduleURL, "/")

for _, part := range relativeParts {
if part == ".." {
// Go up one directory
currentPath = path.Dir(currentPath)
} else if part == "." {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

P2 [DEREM-2] ResolveSubmoduleURL mishandles ./-relative URLs.

The . case does continue (no-op), leaving currentPath at the full parent path including the repo "filename." For ./extras/tool.git relative to https://example.com/org/main.git, this produces https://example.com/org/main.git/extras/tool.git.

Git's behavior: both . and .. are relative to the directory containing the repo, not the repo path itself. The correct result is https://example.com/org/extras/tool.git. The .. case works by accident because path.Dir strips the filename AND goes up one level in a single operation.

Fix: before the loop, set currentPath = path.Dir(parentPath) to establish directory context. Then .. means "go up from directory" and . means "stay."

The test TestResolveSubmoduleURL/relativeChild asserts the wrong expected value, so it passes vacuously. (Netero)

🤖

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Verified against native git that ./extras/tool.git relative to https://example.com/org/main.git resolves to https://example.com/org/main.git/extras/tool.git, matching the current code:

$ # parent remote: 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 uses the same rule (vendor/.../submodule.go: rootEndpoint.Path = path.Join(rootEndpoint.Path, moduleEndpoint.Path)). Both . and .. are resolved against the parent URL via path.Join, not against the parent's directory: .. works because path.Join("/org/main.git", "../deps/lib.git") cleans to /org/deps/lib.git. The existing relativeChild test expectation is correct.

Closing as not a bug; happy to revisit if you have a reproducer showing a different behavior from upstream git.

🤖 Posted using /amend-review skill via Coder Agents.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 [ENVB-5] ResolveSubmoduleURL handles ./-relative paths incorrectly.

The . case at line 561 is a continue (no-op), leaving currentPath at the full parent path including the repo filename. Subsequent path components are appended to the filename.

Trace for ./extras/tool.git against https://example.com/org/main.git:

currentPath = "/org/main.git"
"."       → continue (stays "/org/main.git")
"extras"  → "/org/main.git/extras"
"tool.git"→ "/org/main.git/extras/tool.git"

Result: https://example.com/org/main.git/extras/tool.git (embeds repo name as spurious directory; 404).

Both RFC 3986 and git's native relative_url resolve this to https://example.com/org/extras/tool.git. Fix: change continue to currentPath = path.Dir(currentPath), which strips the file component.

The test expectations for relativeChild, scpRelativeChild, and httpsParentWithUserPassStripped assert the wrong results. (Kite)

🤖

// Stay in current directory
continue
} else if part != "" {
// Add this component to the path
currentPath = currentPath + "/" + part
}
}

// Clean the final path
resolvedPath := path.Clean(currentPath)

// Construct the absolute URL
resolvedParsed := &url.URL{
Scheme: parentParsed.Scheme,
User: parentParsed.User,
Host: parentParsed.Host,
Path: resolvedPath,
}

return resolvedParsed.String(), nil
}

// initSubmodules recursively initializes and updates all submodules in the repository.
func initSubmodules(ctx context.Context, logf func(string, ...any), repo *git.Repository, opts CloneRepoOptions) error {
Comment thread
mafredri marked this conversation as resolved.
Outdated
logf("🔗 Initializing git submodules...")

w, err := repo.Worktree()
if err != nil {
return fmt.Errorf("get worktree: %w", err)
}

subs, err := w.Submodules()
if err != nil {
return fmt.Errorf("get submodules: %w", err)
}

if len(subs) == 0 {
logf("No submodules found")
return nil
}

logf("Found %d submodule(s)", len(subs))

// Get the parent repository URL for resolving relative submodule URLs
cfg, err := repo.Config()
if err != nil {
return fmt.Errorf("get repo config: %w", err)
}

parentURL := opts.RepoURL
if origin, hasOrigin := cfg.Remotes["origin"]; hasOrigin && len(origin.URLs) > 0 {
parentURL = origin.URLs[0]
}
logf("Parent repository URL: %s", parentURL)
Comment thread
mafredri marked this conversation as resolved.
Outdated

for _, sub := range subs {
Comment thread
mafredri marked this conversation as resolved.
subConfig := sub.Config()
logf("📦 Initializing submodule: %s", subConfig.Name)
logf(" Submodule path: %s", subConfig.Path)
logf(" Submodule URL (from .gitmodules): %s", subConfig.URL)

// Get the expected commit hash
subStatus, err := sub.Status()
if err != nil {
return fmt.Errorf("get submodule status for %q: %w", subConfig.Name, err)
}
logf(" Expected commit: %s", subStatus.Expected)

// Resolve the submodule URL
resolvedURL, err := ResolveSubmoduleURLForTest(parentURL, subConfig.URL)
if err != nil {
return fmt.Errorf("resolve submodule URL for %q: %w", subConfig.Name, err)
}
logf(" Resolved URL: %s", resolvedURL)

// Clone the submodule manually
err = cloneSubmodule(ctx, logf, w, subConfig, subStatus.Expected, resolvedURL, opts)
if err != nil {
return fmt.Errorf("clone submodule %q: %w", subConfig.Name, err)
}

logf("✓ Submodule initialized: %s", subConfig.Name)

// Recursively handle nested submodules
subRepo, err := sub.Repository()
Comment thread
mafredri marked this conversation as resolved.
Outdated
if err != nil {
logf(" ⚠ Could not open submodule repository %s: %v", subConfig.Name, err)
continue
}

// Check for nested submodules
subWorktree, err := subRepo.Worktree()
if err == nil {
Comment thread
mafredri marked this conversation as resolved.
Outdated
nestedSubs, err := subWorktree.Submodules()
if err == nil && len(nestedSubs) > 0 {
logf(" Found %d nested submodule(s) in %s", len(nestedSubs), subConfig.Name)
// Create new opts with the submodule's URL as the parent
nestedOpts := opts
Comment thread
mafredri marked this conversation as resolved.
Outdated
nestedOpts.RepoURL = resolvedURL
err = initSubmodules(ctx, logf, subRepo, nestedOpts)
if err != nil {
return fmt.Errorf("init nested submodules in %q: %w", subConfig.Name, err)
}
}
}
}

logf("✓ All submodules initialized successfully")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P3 [ENVB-11] When openSubmoduleRepo or subWorktree.Submodules() fails at lines 651-658, the error is logged as a warning. Then line 669 unconditionally logs "✓ All submodules initialized successfully". The operator sees contradictory signals:

⚠ Could not open submodule repository X for nested traversal: ...
✓ All submodules initialized successfully

Track whether any warnings were emitted and adjust the final message. (Chopper)

🤖

return nil
}

// cloneSubmodule manually clones a submodule repository
func cloneSubmodule(ctx context.Context, logf func(string, ...any), parentWorktree *git.Worktree, subConfig *config.Submodule, expectedHash plumbing.Hash, resolvedURL string, opts CloneRepoOptions) error {
// Get the submodule directory within the parent worktree
submodulePath := subConfig.Path

// Create the submodule directory
subFS, err := parentWorktree.Filesystem.Chroot(submodulePath)
if err != nil {
return fmt.Errorf("chroot to submodule path: %w", err)
}

// Check if already cloned
_, err = subFS.Stat(".git")
if err == nil {
logf(" Submodule already cloned, checking out expected commit...")
// Open the existing repository
subRepo, err := git.Open(
Comment thread
mafredri marked this conversation as resolved.
filesystem.NewStorage(subFS, cache.NewObjectLRU(cache.DefaultMaxSize)),
subFS,
)
if err != nil {
return fmt.Errorf("open existing submodule: %w", err)
}

subWorktree, err := subRepo.Worktree()
if err != nil {
return fmt.Errorf("get submodule worktree: %w", err)
}

// Checkout the expected commit
err = subWorktree.Checkout(&git.CheckoutOptions{
Hash: expectedHash,
})
if err != nil {
return fmt.Errorf("checkout expected commit: %w", err)
Comment thread
mafredri marked this conversation as resolved.
Outdated
}
return nil
}

// Clone the submodule
logf(" Cloning submodule from: %s", resolvedURL)

// Create .git directory for the submodule
err = subFS.MkdirAll(".git", 0o755)
if err != nil {
return fmt.Errorf("create .git directory: %w", err)
}

subGitDir, err := subFS.Chroot(".git")
if err != nil {
return fmt.Errorf("chroot to .git: %w", err)
}

gitStorage := filesystem.NewStorage(subGitDir, cache.NewObjectLRU(cache.DefaultMaxSize*10))

// Clone the submodule repository
// Use SingleBranch=false to fetch all branches so we can find the commit
subRepo, err := git.CloneContext(ctx, gitStorage, subFS, &git.CloneOptions{
Comment thread
mafredri marked this conversation as resolved.
URL: resolvedURL,
Auth: opts.RepoAuth,
Comment thread
mafredri marked this conversation as resolved.
Outdated
Progress: opts.Progress,
InsecureSkipTLS: opts.Insecure,
CABundle: opts.CABundle,
ProxyOptions: opts.ProxyOptions,
SingleBranch: false, // Fetch all branches
NoCheckout: true, // Don't checkout yet, we'll do it manually
})
if err != nil && !errors.Is(err, git.ErrRepositoryAlreadyExists) {
Comment thread
mafredri marked this conversation as resolved.
Outdated
return fmt.Errorf("clone submodule repository: %w", err)
}

// Verify the commit exists
logf(" Verifying commit exists: %s", expectedHash)
_, err = subRepo.CommitObject(expectedHash)
if err != nil {
// Commit not found, try fetching with the specific hash
logf(" Commit not found, attempting to fetch it directly...")
err = subRepo.FetchContext(ctx, &git.FetchOptions{
RemoteName: "origin",
RefSpecs: []config.RefSpec{
config.RefSpec("+" + expectedHash.String() + ":" + expectedHash.String()),
},
Auth: opts.RepoAuth,
Progress: opts.Progress,
InsecureSkipTLS: opts.Insecure,
CABundle: opts.CABundle,
ProxyOptions: opts.ProxyOptions,
})
if err != nil && err != git.NoErrAlreadyUpToDate {
Comment thread
mafredri marked this conversation as resolved.
Outdated
// If that fails, try fetching all refs
logf(" Direct fetch failed, fetching all refs...")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P3 [ENVB-10] fetchErr is dropped from the log. The operator sees "Direct fetch failed, fetching all refs..." but never the reason (auth rejection, network timeout, uploadpack.allowReachableSHA1InWant disabled). If the fallback also fails, the direct-fetch root cause is invisible.

Fix: logf(" Direct fetch failed (%v), fetching all refs...", fetchErr) (Leorio)

🤖

err = subRepo.FetchContext(ctx, &git.FetchOptions{
RemoteName: "origin",
Auth: opts.RepoAuth,
Progress: opts.Progress,
InsecureSkipTLS: opts.Insecure,
CABundle: opts.CABundle,
ProxyOptions: opts.ProxyOptions,
})
if err != nil && err != git.NoErrAlreadyUpToDate {
return fmt.Errorf("fetch commit %s: %w", expectedHash, err)
}
}

// Verify again
_, err = subRepo.CommitObject(expectedHash)
if err != nil {
return fmt.Errorf("commit %s still not found after fetch: %w", expectedHash, err)
}
}

// Checkout the specific commit expected by the parent repository
logf(" Checking out commit: %s", expectedHash)
subWorktree, err := subRepo.Worktree()
if err != nil {
return fmt.Errorf("get submodule worktree: %w", err)
}

err = subWorktree.Checkout(&git.CheckoutOptions{
Hash: expectedHash,
})
if err != nil {
return fmt.Errorf("checkout expected commit %s: %w", expectedHash, err)
}

return nil
}
Loading
Loading