Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
16 changes: 13 additions & 3 deletions pkg/agent/workspace.go
Original file line number Diff line number Diff line change
Expand Up @@ -313,12 +313,17 @@ func ensureGit(ctx context.Context, agentConfig *provider2.ProviderAgentConfig)
if command.Exists("git") {
return nil
}
if local, _ := agentConfig.Local.Bool(); local {
if isLocalAgent(agentConfig) {
return errors.New("git not installed: install git and add it to PATH")
}
return git.InstallBinary(ctx)
}

func isLocalAgent(agentConfig *provider2.ProviderAgentConfig) bool {
local, _ := agentConfig.Local.Bool()
return local
}

func setupGitSSH(
options provider2.CLIOptions,
agentConfig *provider2.ProviderAgentConfig,
Expand Down Expand Up @@ -418,7 +423,8 @@ func cloneViaGit(ctx context.Context, p CloneWorkspaceParams, extraEnv []string)
repo := git.At(p.WorkspaceDir,
git.WithStrictHostKeyChecking(p.Options.StrictHostKeyChecking),
git.WithEnv(extraEnv))
if err := repo.CloneFromInfo(ctx, gitInfo, p.Helper, getGitOptions(p.Options)...); err != nil {
gitOpts := getGitOptions(p.Options, p.AgentConfig)
if err := repo.CloneFromInfo(ctx, gitInfo, p.Helper, gitOpts...); err != nil {
return failedClone(p.WorkspaceDir, "clone repository", err)
}
return nil
Expand Down Expand Up @@ -452,7 +458,10 @@ func applyDevsyIgnore(workspaceDir string) error {
return nil
}

func getGitOptions(options provider2.CLIOptions) []git.Option {
func getGitOptions(
options provider2.CLIOptions,
agentConfig *provider2.ProviderAgentConfig,
) []git.Option {
var gitOpts []git.Option
if options.GitCloneStrategy != "" {
gitOpts = append(gitOpts, git.WithCloneStrategy(options.GitCloneStrategy))
Expand All @@ -468,6 +477,7 @@ func getGitOptions(options provider2.CLIOptions) []git.Option {
} else {
gitOpts = append(gitOpts, git.WithLFSMode(options.GitLFSMode))
}
gitOpts = append(gitOpts, git.WithAllowLFSInstall(!isLocalAgent(agentConfig)))
if options.GitCloneRecursiveSubmodules {
gitOpts = append(gitOpts, git.WithRecursiveSubmodules())
}
Expand Down
23 changes: 23 additions & 0 deletions pkg/agent/workspace_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,33 @@ package agent

import (
"testing"

provider2 "github.com/devsy-org/devsy/pkg/provider"
"github.com/devsy-org/devsy/pkg/types"
)

const explicitAgentDir = "/some/dir"

func TestIsLocalAgent(t *testing.T) {
cases := []struct {
name string
local types.StrBool
want bool
}{
{"local true", "true", true},
{"local false", "false", false},
{"unset defaults to remote", "", false},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
got := isLocalAgent(&provider2.ProviderAgentConfig{Local: tc.local})
if got != tc.want {
t.Errorf("isLocalAgent(%q) = %v, want %v", tc.local, got, tc.want)
}
})
}
}

func withContainerDetector(t *testing.T, fn func() bool) {
t.Helper()
prev := containerDetector
Expand Down
6 changes: 6 additions & 0 deletions pkg/git/clone.go
Original file line number Diff line number Diff line change
Expand Up @@ -107,12 +107,18 @@ func WithSkipLFS() Option {
return WithLFSMode(LFSSkip)
}

// WithAllowLFSInstall permits SetupLFS to install the git-lfs binary itself.
func WithAllowLFSInstall(allow bool) Option {
return func(c *cloneConfig) { c.allowLFSInstall = allow }
}

// cloneConfig is the resolved set of clone options.
type cloneConfig struct {
strategy CloneStrategy
branch string
credentialHelper string
recurseSubmodules bool
allowLFSInstall bool
lfsMode LFSMode
}

Expand Down
34 changes: 25 additions & 9 deletions pkg/git/lfs.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,23 +41,20 @@ func cloneEnvForLFS() []string {
return nil
}

// lfsInstaller is overridable in tests.
var lfsInstaller = InstallLFS

// SetupLFS configures Git LFS in the repository.
func (r *Repo) SetupLFS(ctx context.Context, mode LFSMode) {
func (r *Repo) SetupLFS(ctx context.Context, mode LFSMode, allowInstall bool) {
if mode == LFSSkip {
return
}
if !repoUsesLFS(r.path) {
return
}

if !command.Exists(binGitLFS) {
if err := InstallLFS(ctx); err != nil {
log.Warnf(
"repository uses git-lfs but it could not be installed, LFS files will be pointer stubs: %v",
err,
)
return
}
if !command.Exists(binGitLFS) && !ensureLFSBinary(ctx, allowInstall) {
return
Comment on lines +56 to +57

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Remove the remaining automatic Git LFS installation path.

The PR objective says missing git-lfs must fall back to pointer stubs without installation, but non-local agents still call InstallLFS through lfsInstaller.

  • pkg/git/lfs.go#L56-L57: log and return when git-lfs is absent; remove lfsInstaller and ensureLFSBinary.
  • pkg/git/clone.go#L110-L121: remove WithAllowLFSInstall and allowLFSInstall.
  • pkg/git/repo.go#L176-L176: remove propagation of the installation flag.
  • pkg/agent/workspace.go#L480-L480: stop enabling LFS installation for remote agents.
  • pkg/git/lfs_test.go#L120-L138: replace the “installation allowed” test with coverage that the installer is never invoked when the binary is missing.
📍 Affects 5 files
  • pkg/git/lfs.go#L56-L57 (this comment)
  • pkg/git/clone.go#L110-L121
  • pkg/git/repo.go#L176-L176
  • pkg/agent/workspace.go#L480-L480
  • pkg/git/lfs_test.go#L120-L138
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/git/lfs.go` around lines 56 - 57, Remove the automatic Git LFS
installation path: in pkg/git/lfs.go:56-57, log and return when binGitLFS is
absent, and remove lfsInstaller and ensureLFSBinary; in
pkg/git/clone.go:110-121, remove WithAllowLFSInstall and allowLFSInstall; in
pkg/git/repo.go:176, stop propagating the installation flag; in
pkg/agent/workspace.go:480, stop enabling LFS installation for remote agents;
and in pkg/git/lfs_test.go:120-138, replace the installation-allowed test with
coverage confirming the installer is never invoked when git-lfs is missing.

}

if err := r.lfs(ctx, "install", "--local"); err != nil {
Expand All @@ -74,6 +71,25 @@ func (r *Repo) SetupLFS(ctx context.Context, mode LFSMode) {
}
}

// ensureLFSBinary installs git-lfs when allowInstall is set, reporting
// whether the binary is now available.
func ensureLFSBinary(ctx context.Context, allowInstall bool) bool {
if !allowInstall {
log.Info(
"repository uses git-lfs but the binary is not installed, LFS files will be pointer stubs",
)
return false
}
if err := lfsInstaller(ctx); err != nil {
log.Warnf(
"repository uses git-lfs but it could not be installed, LFS files will be pointer stubs: %v",
err,
)
return false
}
return true
}

// lfs runs a `git lfs <args...>` subcommand in the repository, returning any
// captured output alongside the error for diagnostics.
func (r *Repo) lfs(ctx context.Context, args ...string) error {
Expand Down
61 changes: 60 additions & 1 deletion pkg/git/lfs_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@ package git

import (
"context"
"errors"
"os"
"os/exec"
"path/filepath"
"strings"
"testing"
Expand Down Expand Up @@ -89,7 +91,7 @@ func TestSetupLFSModeCommands(t *testing.T) {
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
dir, fake := newLFSRepo(t)
At(dir, WithRunner(fake)).SetupLFS(context.Background(), tc.mode)
At(dir, WithRunner(fake)).SetupLFS(context.Background(), tc.mode, false)

var got []string
for _, args := range lfsSubcommands(fake) {
Expand All @@ -100,6 +102,63 @@ func TestSetupLFSModeCommands(t *testing.T) {
}
}

func TestSetupLFSSkipsWhenBinaryMissingAndInstallNotAllowed(t *testing.T) {
hideGitLFSFromPath(t)
stubLFSInstaller(t, func(context.Context) error {
t.Fatal("lfsInstaller must not be called when allowInstall is false")
return nil
})

dir, fake := newLFSRepo(t)
At(dir, WithRunner(fake)).SetupLFS(context.Background(), LFSFull, false)

if got := lfsSubcommands(fake); got != nil {
t.Errorf("lfs subcommands = %v, want none", got)
}
}

func TestSetupLFSInstallsWhenBinaryMissingAndInstallAllowed(t *testing.T) {
hideGitLFSFromPath(t)

var called bool
stubLFSInstaller(t, func(context.Context) error {
called = true
return errors.New("install unavailable in this environment")
})

dir, fake := newLFSRepo(t)
At(dir, WithRunner(fake)).SetupLFS(context.Background(), LFSFull, true)

if !called {
t.Error("lfsInstaller was not called despite allowInstall being true")
}
if got := lfsSubcommands(fake); got != nil {
t.Errorf("lfs subcommands = %v, want none (install failed)", got)
}
}

func stubLFSInstaller(t *testing.T, fn func(context.Context) error) {
t.Helper()
original := lfsInstaller
lfsInstaller = fn
t.Cleanup(func() { lfsInstaller = original })
}

func hideGitLFSFromPath(t *testing.T) {
t.Helper()

gitPath, err := exec.LookPath("git")
if err != nil {
t.Skip("git not found on PATH")
}

binDir := t.TempDir()
if err := os.Symlink(gitPath, filepath.Join(binDir, "git")); err != nil {
t.Fatal(err)
}
t.Setenv("PATH", binDir)
}

func TestRepoUsesLFS(t *testing.T) {
cases := []struct {
name string
Expand Down
2 changes: 1 addition & 1 deletion pkg/git/repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ func (r *Repo) CloneFromInfo(

// Bare clones have no worktree to hydrate.
if c.strategy != BareCloneStrategy {
r.SetupLFS(ctx, c.lfsMode)
r.SetupLFS(ctx, c.lfsMode, c.allowLFSInstall)
}
return nil
}
Expand Down
Loading