diff --git a/runtime/drivers/admin/repo.go b/runtime/drivers/admin/repo.go index 2b0d14e571d7..e53d370730ae 100644 --- a/runtime/drivers/admin/repo.go +++ b/runtime/drivers/admin/repo.go @@ -16,12 +16,12 @@ import ( "time" "github.com/bmatcuk/doublestar/v4" - "github.com/go-git/go-git/v5" - "github.com/rilldata/rill/cli/pkg/gitutil" + cligitutil "github.com/rilldata/rill/cli/pkg/gitutil" adminv1 "github.com/rilldata/rill/proto/gen/rill/admin/v1" "github.com/rilldata/rill/runtime/drivers" "github.com/rilldata/rill/runtime/pkg/ctxsync" "github.com/rilldata/rill/runtime/pkg/filewatcher" + "github.com/rilldata/rill/runtime/pkg/gitutil" "go.uber.org/zap" "golang.org/x/sync/singleflight" "gopkg.in/yaml.v3" @@ -453,15 +453,11 @@ func (r *repo) Status(ctx context.Context, remoteBranch string) (*drivers.RepoSt return &drivers.RepoStatus{}, nil } - repo, err := git.PlainOpen(r.git.repoDir) + currentBranch, err := currentBranch(r.git.repoDir) if err != nil { return nil, err } - head, err := repo.Head() - if err != nil { - return nil, err - } - branches := []string{head.Name().Short()} + branches := []string{currentBranch} // If a remote branch was explicitly requested and it differs from the current branch, fetch it too // so that ahead/behind counts in RunGitStatus have an up-to-date remote tracking ref to compare against. @@ -469,13 +465,13 @@ func (r *repo) Status(ctx context.Context, remoteBranch string) (*drivers.RepoSt branches = append(branches, remoteBranch) } - err = r.git.fetchBranch(ctx, repo, branches...) + err = gitutil.FetchBranches(ctx, r.git.repoDir, branches...) if err != nil { return nil, fmt.Errorf("failed to fetch branches %q: %w", branches, err) } // run git status - st, err := gitutil.RunGitStatus(r.git.repoDir, r.git.subpath, "origin", remoteBranch) + st, err := cligitutil.RunGitStatus(r.git.repoDir, r.git.subpath, "origin", remoteBranch) if err != nil { return nil, fmt.Errorf("failed to get Git status: %w", err) } @@ -505,11 +501,7 @@ func (r *repo) Commit(ctx context.Context, message string) (string, error) { if !r.git.editable() { return "", fmt.Errorf("repo is not editable") } - repo, err := git.PlainOpen(r.git.repoDir) - if err != nil { - return "", err - } - return r.git.commitAll(repo, message) + return gitutil.CommitAll(ctx, r.git.repoDir, r.git.subpath, message, "Rill", "noreply@rilldata.com") } // Pull implements drivers.RepoStore. @@ -564,7 +556,7 @@ func (r *repo) CommitHash(ctx context.Context) (string, error) { if r.archive != nil { return r.archive.archiveID, nil } - return r.git.commitHash() + return r.git.commitHash(ctx) } // CommitTimestamp implements drivers.RepoStore. @@ -578,7 +570,7 @@ func (r *repo) CommitTimestamp(ctx context.Context) (time.Time, error) { if r.archive != nil { return r.archive.archiveCreatedOn, nil } - return r.git.commitTimestamp() + return r.git.commitTimestamp(ctx) } // close deletes the temporary directories used by the repo. @@ -837,6 +829,11 @@ func (r *repo) checkHandshake(ctx context.Context, force bool) error { if err != nil { return fmt.Errorf("failed to get git data dir: %w", err) } + // Resolve to an absolute path so that `git -C ` does not depend on the process working directory. + repoDir, err = filepath.Abs(repoDir) + if err != nil { + return fmt.Errorf("failed to resolve git data dir: %w", err) + } r.git = &gitRepo{ h: r.h, repoDir: repoDir, diff --git a/runtime/drivers/admin/repo_git.go b/runtime/drivers/admin/repo_git.go index ab1557791795..9d8eed92e1f2 100644 --- a/runtime/drivers/admin/repo_git.go +++ b/runtime/drivers/admin/repo_git.go @@ -5,15 +5,10 @@ import ( "errors" "fmt" "os" - "os/exec" "path" "strings" "time" - "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/object" "github.com/rilldata/rill/runtime/drivers" "github.com/rilldata/rill/runtime/pkg/gitutil" "github.com/rilldata/rill/runtime/pkg/observability" @@ -68,30 +63,25 @@ func (r *gitRepo) pullInner(ctx context.Context, userTriggered, force bool) erro } // Check if repoDir exists and is a valid Git repository - repo, err := git.PlainOpen(r.repoDir) - if err != nil { + if !isRepoRoot(r.repoDir) { // Repository doesn't exist or is invalid, remove and clone fresh if err := os.RemoveAll(r.repoDir); err != nil { return err } - - cloneOptions := &git.CloneOptions{ - URL: r.remoteURL, - SingleBranch: r.primaryBranch == r.defaultBranch, - ReferenceName: plumbing.ReferenceName("refs/heads/" + r.primaryBranch), // primary branch must exist, default branch may not exist yet in editable mode. - } - - repo, err = git.PlainCloneContext(ctx, r.repoDir, false, cloneOptions) + // for non editable make the clone faster by only cloning the primary branch with no history + // for editable deployments, we need the full history and all branches to support editing and merging + err := gitutil.Clone(ctx, r.repoDir, r.remoteURL, r.primaryBranch, !r.editableDepl, !r.editableDepl) if err != nil { return err } + if r.editableDepl { // set git config in the repo dir to ensure git commits/git merge etc pass on cloud - err = ensureGitConfig(r.repoDir, "user.name", "Rill") + err = setGitConfig(r.repoDir, "user.name", "Rill") if err != nil { return err } - err = ensureGitConfig(r.repoDir, "user.email", "noreply@rilldata.com") + err = setGitConfig(r.repoDir, "user.email", "noreply@rilldata.com") if err != nil { return err } @@ -100,39 +90,35 @@ func (r *gitRepo) pullInner(ctx context.Context, userTriggered, force bool) erro // Repository exists, pull latest changes // Ensure the remote URL is correct - _ = repo.DeleteRemote("origin") - remote, err := repo.CreateRemote(&config.RemoteConfig{ - Name: "origin", - URLs: []string{r.remoteURL}, - }) + err := setRemoteURL(r.repoDir, r.remoteURL) if err != nil { - return fmt.Errorf("failed to set remote URL: %w", err) + return err + } + + if !r.editableDepl { + // For non-editable repos, we only care about the primary branch, so set the fetch refspec to only fetch the primary branch. + err := setFetchBranch(r.repoDir, r.primaryBranch) + if err != nil { + return err + } } // Fetch the remote changes. - // We fetch each branch individually because a NoMatchingRefSpecError for one branch (e.g., an edit branch - // that doesn't exist on the remote yet) would cause a combined fetch to skip all branches. - branches := []string{r.defaultBranch} - if r.primaryBranch != r.defaultBranch { - branches = append(branches, r.primaryBranch) + branchesToFetch := []string{r.primaryBranch} + if r.editableDepl && r.primaryBranch != r.defaultBranch { + branchesToFetch = append(branchesToFetch, r.defaultBranch) } - for _, branch := range branches { - refSpec := config.RefSpec(fmt.Sprintf("refs/heads/%s:refs/remotes/origin/%s", branch, branch)) - err = remote.Fetch(&git.FetchOptions{ - RefSpecs: []config.RefSpec{refSpec}, - Force: true, - }) - if err != nil && !(errors.Is(err, git.NoErrAlreadyUpToDate) || git.NoMatchingRefSpecError{}.Is(err)) { - return fmt.Errorf("failed to fetch from remote: %w", err) - } + err = gitutil.FetchBranches(ctx, r.repoDir, branchesToFetch...) + if err != nil { + return err } } // Checkout the default branch var createDefault bool - err = gitCheckout(r.repoDir, r.defaultBranch, force, false, "") + err := gitCheckout(r.repoDir, r.defaultBranch, force, false, "") if err != nil { - if !errors.Is(err, errRefNotFound) { + if !errors.Is(err, gitutil.ErrRefNotFound) { return fmt.Errorf("failed to checkout branch %q: %w", r.defaultBranch, err) } @@ -140,12 +126,12 @@ func (r *gitRepo) pullInner(ctx context.Context, userTriggered, force bool) erro // a. when branch is created remotely after the last pull // b. primary branch was edited // c. in editable mode when the default branch may not exist at all. - remoteHash, err := repo.Reference(plumbing.ReferenceName("refs/remotes/origin/"+r.defaultBranch), true) + remoteHash, err := gitutil.Hash(ctx, r.repoDir, "refs/remotes/origin/"+r.defaultBranch) if err != nil { - if errors.Is(err, plumbing.ErrReferenceNotFound) && r.editable() { + if errors.Is(err, gitutil.ErrRefNotFound) && r.editable() { // In editable mode, the default branch may not exist yet on remote. We will create it based on the primary branch. r.h.logger.Info("Default branch does not exist on remote, will create it based on primary branch", zap.String("defaultBranch", r.defaultBranch), zap.String("primaryBranch", r.primaryBranch)) - remoteHash, err = repo.Reference(plumbing.ReferenceName("refs/remotes/origin/"+r.primaryBranch), true) + remoteHash, err = gitutil.Hash(ctx, r.repoDir, "refs/remotes/origin/"+r.primaryBranch) if err != nil { return fmt.Errorf("failed to get reference for primary branch %q: %w", r.primaryBranch, err) } @@ -157,7 +143,7 @@ func (r *gitRepo) pullInner(ctx context.Context, userTriggered, force bool) erro } // Create the default branch at the resolved remote hash - err = gitCheckout(r.repoDir, r.defaultBranch, true, true, remoteHash.Hash().String()) + err = gitCheckout(r.repoDir, r.defaultBranch, true, true, remoteHash) if err != nil { return fmt.Errorf("failed to create and checkout default branch %q: %w", r.defaultBranch, err) } @@ -175,7 +161,7 @@ func (r *gitRepo) pullInner(ctx context.Context, userTriggered, force bool) erro // Hard reset to remote branch err = resetToRemoteTrackingBranch(r.repoDir, r.defaultBranch) if err != nil { - if !(errors.Is(err, errRefNotFound) && r.editable()) { // In editable mode, the default branch may not exist yet on remote. + if !(errors.Is(err, gitutil.ErrRefNotFound) && r.editable()) { // In editable mode, the default branch may not exist yet on remote. return fmt.Errorf("failed to reset to remote tracking branch %q: %w", r.defaultBranch, err) } } @@ -247,14 +233,10 @@ func (r *gitRepo) commitToDefaultBranch(ctx context.Context, message string, for } r.h.logger.Info("commitToDefaultBranch", observability.ZapCtx(ctx)) - repo, err := git.PlainOpen(r.repoDir) - if err != nil { - return err - } - _, err = r.commitAll(repo, message) + _, err := gitutil.CommitAll(ctx, r.repoDir, r.subpath, message, "Rill", "noreply@rilldata.com") if err != nil { - if !errors.Is(err, git.ErrEmptyCommit) { + if !errors.Is(err, gitutil.ErrEmptyCommit) { return fmt.Errorf("failed to commit changes to edit branch: %w", err) } // continue to push existing commits, if any @@ -307,17 +289,13 @@ func (r *gitRepo) mergeToBranch(ctx context.Context, branch string, force bool) } r.h.logger.Info("mergeToBranch", zap.String("branch", branch), zap.Bool("force", force), observability.ZapCtx(ctx)) - repo, err := git.PlainOpen(r.repoDir) - if err != nil { - return fmt.Errorf("failed to open repository: %w", err) - } - _, err = r.commitAll(repo, "Auto commit before merging to "+branch) - if err != nil && !errors.Is(err, git.ErrEmptyCommit) { + _, err := gitutil.CommitAll(ctx, r.repoDir, r.subpath, "Auto commit before merging to "+branch, "Rill", "noreply@rilldata.com") + if err != nil && !errors.Is(err, gitutil.ErrEmptyCommit) { return fmt.Errorf("failed to commit changes: %w", err) } // Fetch the branch to ensure we are up-to-date - err = r.fetchBranch(ctx, repo, branch) + err = gitutil.FetchBranches(ctx, r.repoDir, branch) if err != nil { return err } @@ -374,134 +352,47 @@ func (r *gitRepo) mergeToBranch(ctx context.Context, branch string, force bool) } // commitHash returns the current commit hash of the repository. -func (r *gitRepo) commitHash() (string, error) { - repo, err := git.PlainOpen(r.repoDir) - if err != nil { - return "", err - } - - ref, err := repo.Head() - if err != nil { - return "", err - } - - if ref.Hash().IsZero() { +// It returns an empty string (without error) when HEAD points to an unborn branch. +func (r *gitRepo) commitHash(ctx context.Context) (string, error) { + hash, err := gitutil.Hash(ctx, r.repoDir, "HEAD") + if errors.Is(err, gitutil.ErrRefNotFound) { return "", nil } - - return ref.Hash().String(), nil + return hash, err } // commitTimestamp returns the timestamp of the latest commit on the current branch. -func (r *gitRepo) commitTimestamp() (time.Time, error) { - repo, err := git.PlainOpen(r.repoDir) - if err != nil { - return time.Time{}, err - } - - ref, err := repo.Head() - if err != nil { - return time.Time{}, err - } - - commit, err := repo.CommitObject(ref.Hash()) +func (r *gitRepo) commitTimestamp(ctx context.Context) (time.Time, error) { + out, err := gitutil.Run(ctx, r.repoDir, "log", "-1", "--format=%aI", "HEAD") if err != nil { return time.Time{}, err } - - return commit.Author.When, nil -} - -func (r *gitRepo) commitAll(repo *git.Repository, message string) (string, error) { - worktree, err := repo.Worktree() - if err != nil { - return "", err - } - - err = worktree.AddWithOptions(&git.AddOptions{ - All: true, // Add all changes - }) - if err != nil { - return "", err - } - - hash, err := worktree.Commit(message, &git.CommitOptions{ - All: true, // Commit all changes - Author: &object.Signature{ - Name: "Rill", - Email: "noreply@rilldata.com", - When: time.Now(), - }, - }) - if err != nil { - return "", err - } - return hash.String(), nil + return time.Parse(time.RFC3339, out) } func (r *gitRepo) fetchCurrentBranch(ctx context.Context) error { - repo, err := git.PlainOpen(r.repoDir) - if err != nil { - return err - } - head, err := repo.Head() + branch, err := currentBranch(r.repoDir) if err != nil { return err } - return r.fetchBranch(ctx, repo, head.Name().Short()) + return gitutil.FetchBranches(ctx, r.repoDir, branch) } // pushBranch pushes the specified branches to the remote repository. -// If the remote branch is already up-to-date, it does not return an error. -// It re-opens the repository so that any objects written by prior shell-based git -// commands (e.g. merge, checkout) are visible to go-git's push. func (r *gitRepo) pushBranch(ctx context.Context, branches ...string) error { if len(branches) == 0 { return errors.New("at least one branch must be specified to push") } - repo, err := git.PlainOpen(r.repoDir) - if err != nil { - return err - } - refSpecs := make([]config.RefSpec, 0, len(branches)) - for _, branch := range branches { - refSpecs = append(refSpecs, config.RefSpec(fmt.Sprintf("refs/heads/%s:refs/heads/%s", branch, branch))) - } - err = repo.PushContext(ctx, &git.PushOptions{ - RemoteName: "origin", - RemoteURL: r.remoteURL, - RefSpecs: refSpecs, - }) - if err != nil && !errors.Is(err, git.NoErrAlreadyUpToDate) { - return err - } - return nil -} - -// fetchBranch fetches the specified branch(s) from the remote repository. -// It does not return an error if the local branch is already up-to-date with the remote branch. -func (r *gitRepo) fetchBranch(ctx context.Context, repo *git.Repository, branch ...string) error { - refSpecs := make([]config.RefSpec, 0, len(branch)) - for _, b := range branch { - refSpecs = append(refSpecs, config.RefSpec(fmt.Sprintf("refs/heads/%s:refs/remotes/origin/%s", b, b))) - } - err := repo.FetchContext(ctx, &git.FetchOptions{ - RemoteURL: r.remoteURL, - RefSpecs: refSpecs, - }) - if err != nil && !errors.Is(err, git.NoErrAlreadyUpToDate) { - return err - } - return nil + args := append([]string{"push", "origin"}, branches...) + _, err := gitutil.Run(ctx, r.repoDir, args...) + return err } -var errRefNotFound = errors.New("reference not found") - // gitCheckout checks out a branch using the git command. // If create is true, it creates the branch (using -B) at the given startPoint. // go-git wipes out git-ignored changes during checkout so must use the git command. func gitCheckout(repoDir, branch string, force, create bool, startPoint string) error { - args := []string{"-C", repoDir, "checkout"} + args := []string{"checkout"} if force { args = append(args, "--force") } @@ -513,18 +404,12 @@ func gitCheckout(repoDir, branch string, force, create bool, startPoint string) } else { args = append(args, branch) } - cmd := exec.Command("git", args...) - _, err := cmd.Output() + _, err := gitutil.Run(context.Background(), repoDir, args...) if err != nil { - var execErr *exec.ExitError - if !errors.As(err, &execErr) { - return err - } - stderr := string(execErr.Stderr) - if strings.Contains(stderr, "did not match") { - return errRefNotFound + if strings.Contains(err.Error(), "did not match") { + return gitutil.ErrRefNotFound } - return fmt.Errorf("git checkout failed: %s", stderr) + return err } return nil } @@ -533,35 +418,40 @@ func gitCheckout(repoDir, branch string, force, create bool, startPoint string) // This is used to reset the local branch to the state of the remote branch so it is expected that the latest changes have been fetched. // go-git wipes out git-ignored changes so must use the git command. func resetToRemoteTrackingBranch(repoDir, branch string) error { - cmd := exec.Command("git", "-C", repoDir, "reset", "--hard", "origin/"+branch) - _, err := cmd.Output() + _, err := gitutil.Run(context.Background(), repoDir, "reset", "--hard", "origin/"+branch) if err != nil { - var execErr *exec.ExitError - if !errors.As(err, &execErr) { - return err + if strings.Contains(err.Error(), "unknown revision") { + return gitutil.ErrRefNotFound } - if strings.Contains(string(execErr.Stderr), "unknown revision") { - return errRefNotFound - } - return fmt.Errorf("git reset failed: %s", string(execErr.Stderr)) + return err } return nil } -// ensureGitConfig ensures that the git config key is set. -// if not set then it sets the key to the given value locally in the repo -func ensureGitConfig(repoDir, key, value string) error { - _, err := exec.Command("git", "-C", repoDir, "config", "--get", key).Output() - if err == nil { - return nil - } +// setGitConfig sets the git config key locally in the repo. +func setGitConfig(repoDir, key, value string) error { + _, err := gitutil.Run(context.Background(), repoDir, "config", "--local", key, value) + return err +} - // Exit code 1 means "key not set" — that's the case we want to handle. - var exitErr *exec.ExitError - if !errors.As(err, &exitErr) || exitErr.ExitCode() != 1 { - return err +func isRepoRoot(path string) bool { + out, err := gitutil.Run(context.Background(), path, "rev-parse", "--show-cdup") + if err != nil { + return false } + return out == "" +} + +func setRemoteURL(path, remoteURL string) error { + _, err := gitutil.Run(context.Background(), path, "remote", "set-url", "origin", remoteURL) + return err +} + +func setFetchBranch(path, branch string) error { + _, err := gitutil.Run(context.Background(), path, "remote", "set-branches", "origin", branch) + return err +} - // set only locally - return exec.Command("git", "-C", repoDir, "config", "--local", key, value).Run() +func currentBranch(path string) (string, error) { + return gitutil.Run(context.Background(), path, "rev-parse", "--abbrev-ref", "HEAD") } diff --git a/runtime/drivers/admin/repo_git_test.go b/runtime/drivers/admin/repo_git_test.go index 82e1439ba0bb..886cb6c31542 100644 --- a/runtime/drivers/admin/repo_git_test.go +++ b/runtime/drivers/admin/repo_git_test.go @@ -12,6 +12,7 @@ import ( "github.com/go-git/go-git/v5" "github.com/go-git/go-git/v5/plumbing" "github.com/go-git/go-git/v5/plumbing/object" + "github.com/rilldata/rill/runtime/pkg/gitutil" "github.com/stretchr/testify/require" "go.uber.org/zap" ) @@ -901,20 +902,36 @@ func TestGitRepo_mergeToBranch(t *testing.T) { } } -func TestEnsureGitConfig(t *testing.T) { +func TestGitCheckout_refNotFound(t *testing.T) { + repoDir := t.TempDir() + require.NoError(t, execGitCommand(exec.Command("git", "init", repoDir))) + require.NoError(t, execGitCommand(exec.Command("git", "-C", repoDir, "checkout", "-b", "main"))) + setupGitConfig(t, repoDir) + // Need an initial commit so HEAD resolves; otherwise checkout would fail for a different reason. + require.NoError(t, os.WriteFile(filepath.Join(repoDir, "seed.txt"), []byte("seed"), 0644)) + require.NoError(t, execGitCommand(exec.Command("git", "-C", repoDir, "add", "seed.txt"))) + require.NoError(t, execGitCommand(exec.Command("git", "-C", repoDir, "commit", "-m", "seed"))) + + err := gitCheckout(repoDir, "does-not-exist", false, false, "") + require.ErrorIs(t, err, gitutil.ErrRefNotFound, "missing ref must surface as ErrRefNotFound so pullInner can create the branch") +} + +func TestSetGitConfig(t *testing.T) { withCleanGitEnv(t) - repo := t.TempDir() - require.NoError(t, execGitCommand(exec.Command("git", "-C", repo, "init"))) - require.NoError(t, ensureGitConfig(repo, "user.name", "Test User")) - out, err := exec.Command("git", "-C", repo, "config", "--local", "--get", "user.name").CombinedOutput() + repoDir := t.TempDir() + require.NoError(t, execGitCommand(exec.Command("git", "init", repoDir))) + + require.NoError(t, setGitConfig(repoDir, "user.name", "Test User")) + out, err := exec.Command("git", "-C", repoDir, "config", "--local", "--get", "user.name").CombinedOutput() require.NoError(t, err) require.Equal(t, "Test User\n", string(out)) - require.NoError(t, ensureGitConfig(repo, "user.name", "Rest User")) - out, err = exec.Command("git", "-C", repo, "config", "--local", "--get", "user.name").CombinedOutput() + // setGitConfig overwrites unconditionally — earlier behavior (ensureGitConfig) only set if missing. + require.NoError(t, setGitConfig(repoDir, "user.name", "Rest User")) + out, err = exec.Command("git", "-C", repoDir, "config", "--local", "--get", "user.name").CombinedOutput() require.NoError(t, err) - require.Equal(t, "Test User\n", string(out)) // still test user since set locally + require.Equal(t, "Rest User\n", string(out)) } func newEditableGitRepo(localDir, remoteURL, defaultBranch, primaryBranch, subpath string) *gitRepo { diff --git a/runtime/pkg/gitutil/gitcmdwrapper.go b/runtime/pkg/gitutil/gitcmdwrapper.go index 74922b3ae18c..0a1e10bc8f0d 100644 --- a/runtime/pkg/gitutil/gitcmdwrapper.go +++ b/runtime/pkg/gitutil/gitcmdwrapper.go @@ -1,32 +1,53 @@ package gitutil import ( + "bytes" + "context" "errors" "fmt" + "os" "os/exec" + "path/filepath" + "strings" ) +var ErrRefNotFound = errors.New("git reference not found") + +// ErrEmptyCommit is returned by CommitAll when there are no changes to commit. +var ErrEmptyCommit = errors.New("nothing to commit") + +func Clone(ctx context.Context, path, remote, checkoutBranch string, singleBranch, shallow bool) error { + args := []string{"clone", remote, path} + if singleBranch { + args = append(args, "--single-branch") + } + if shallow { + args = append(args, "--depth", "1") + } + if checkoutBranch != "" { + args = append(args, "--branch", checkoutBranch) + } + + _, err := Run(ctx, "", args...) + return err +} + // MergeWithStrategy merge a branch into the current branch using the specified strategy. func MergeWithStrategy(path, branch, strategy string) error { var args []string switch strategy { case "theirs": - args = []string{"-C", path, "merge", "-X", "theirs", branch} + args = []string{"merge", "-X", "theirs", branch} case "ours": - args = []string{"-C", path, "merge", "-X", "ours", branch} + args = []string{"merge", "-X", "ours", branch} case "": - args = []string{"-C", path, "merge", branch} + args = []string{"merge", branch} default: return fmt.Errorf("internal error: unsupported merge strategy: %s", strategy) } - cmd := exec.Command("git", args...) - _, err := cmd.Output() + _, err := Run(context.Background(), path, args...) if err != nil { - var execErr *exec.ExitError - if errors.As(err, &execErr) { - return fmt.Errorf("git merge failed: %s", string(execErr.Stderr)) - } return err } return nil @@ -34,28 +55,128 @@ func MergeWithStrategy(path, branch, strategy string) error { // MergeWithBailOnConflict attempts to merge a branch into the current branch and aborts if there are conflicts. // Returns true if merge was successful, false if there were conflicts (but abort succeeded). -// Returns an error only if both merge and abort fail. +// Returns an error if the merge failed for a reason other than conflicts, or if both merge and abort fail. func MergeWithBailOnConflict(path, branch string) (bool, error) { - // First try the merge - cmd := exec.Command("git", "-C", path, "merge", "--no-ff", branch) - _, err := cmd.Output() - if err == nil { - // Merge succeeded + _, mergeErr := Run(context.Background(), path, "merge", "--no-ff", branch) + if mergeErr == nil { return true, nil } - // Merge failed, try to abort - abortCmd := exec.Command("git", "-C", path, "merge", "--abort") - abortErr := abortCmd.Run() - if abortErr != nil { - // Both merge and abort failed - var execErr *exec.ExitError - if errors.As(err, &execErr) { - return false, fmt.Errorf("git merge failed and abort failed: %s", string(execErr.Stderr)) - } - return false, fmt.Errorf("git merge failed and abort failed: %w", err) + // Detect "merge in progress" via MERGE_HEAD: presence means git stopped on conflicts and is waiting for resolution. + // Other merge failures (e.g., invalid ref, untracked-file overwrite that git auto-aborts) leave no MERGE_HEAD, + // and should surface as real errors rather than be silently treated as conflicts. + merging, err := mergeInProgress(path) + if err != nil { + return false, fmt.Errorf("merge failed: %w; could not check merge state: %w", mergeErr, err) + } + if !merging { + return false, mergeErr } - // Merge failed but abort succeeded + if _, abortErr := Run(context.Background(), path, "merge", "--abort"); abortErr != nil { + return false, fmt.Errorf("merge failed with error: %w, and abort also failed with error: %w", mergeErr, abortErr) + } return false, nil } + +// mergeInProgress reports whether the repository at path is in the middle of a merge. +func mergeInProgress(path string) (bool, error) { + mergeHeadPath, err := Run(context.Background(), path, "rev-parse", "--git-path", "MERGE_HEAD") + if err != nil { + return false, err + } + if !filepath.IsAbs(mergeHeadPath) { + mergeHeadPath = filepath.Join(path, mergeHeadPath) + } + if _, err := os.Stat(mergeHeadPath); err != nil { + if errors.Is(err, os.ErrNotExist) { + return false, nil + } + return false, err + } + return true, nil +} + +// CommitAll stages all changes in the working tree and creates a commit with the given message. +// If pathspec is non-empty, staging and the empty-commit check are scoped to that pathspec. +// The commit's author and committer are set to the provided name and email. +// Returns the new commit hash, or ErrEmptyCommit if there are no changes to commit. +func CommitAll(ctx context.Context, path, pathspec, message, authorName, authorEmail string) (string, error) { + addArgs := []string{"add", "--all"} + statusArgs := []string{"status", "--porcelain"} + if pathspec != "" { + addArgs = append(addArgs, "--", pathspec) + statusArgs = append(statusArgs, "--", pathspec) + } + + if _, err := Run(ctx, path, addArgs...); err != nil { + return "", err + } + + status, err := Run(ctx, path, statusArgs...) + if err != nil { + return "", err + } + if status == "" { + return "", ErrEmptyCommit + } + + args := []string{ + "-c", "user.name=" + authorName, + "-c", "user.email=" + authorEmail, + "commit", "-m", message, + } + if _, err := Run(ctx, path, args...); err != nil { + return "", err + } + + return Hash(ctx, path, "HEAD") +} + +// FetchBranches fetches the specified branches from the remote repository. +// If a branch doesn't exist on the remote, it will be skipped without returning an error. +func FetchBranches(ctx context.Context, path string, branches ...string) error { + for _, branch := range branches { + // fetch separately to avoid NoMatchingRefSpecError when one of the branches doesn't exist on remote + _, err := Run(ctx, path, "fetch", "origin", branch) + if err != nil { + if strings.Contains(err.Error(), "find remote ref") { + continue + } + return err + } + } + return nil +} + +// Hash returns the commit hash for the given ref. Returns ErrRefNotFound if the ref does not resolve. +func Hash(ctx context.Context, path, ref string) (string, error) { + out, err := Run(ctx, path, "rev-parse", "--verify", ref) + if err != nil { + if strings.Contains(err.Error(), "Needed a single revision") { + return "", ErrRefNotFound + } + return "", err + } + return out, nil +} + +// Run executes a git command with the specified arguments in the given path and returns its output or an error. +// If path is empty, the command runs without -C (use for commands like `clone` that take an explicit destination). +// Use it to run one-off git commands that don't fit into the other helper functions in this package. +func Run(ctx context.Context, path string, args ...string) (string, error) { + fullArgs := args + if path != "" { + fullArgs = append([]string{"-C", path}, args...) + } + var stdout, stderr bytes.Buffer + cmd := exec.CommandContext(ctx, "git", fullArgs...) + // Force English error messages so stderr substring checks are stable, and disable interactive credential prompts. + cmd.Env = append(os.Environ(), "LC_ALL=C", "GIT_TERMINAL_PROMPT=0") + cmd.Stdout = &stdout + cmd.Stderr = &stderr + if err := cmd.Run(); err != nil { + return "", fmt.Errorf("git %s: %s(%w)", strings.Join(args, " "), strings.TrimSpace(stderr.String()), err) + } + return strings.TrimSpace(stdout.String()), nil +} diff --git a/runtime/pkg/gitutil/gitcmdwrapper_test.go b/runtime/pkg/gitutil/gitcmdwrapper_test.go index 8eefa3f6476b..31abb7166eaf 100644 --- a/runtime/pkg/gitutil/gitcmdwrapper_test.go +++ b/runtime/pkg/gitutil/gitcmdwrapper_test.go @@ -1,6 +1,7 @@ package gitutil import ( + "context" "errors" "fmt" "os" @@ -219,7 +220,158 @@ func TestMergeWithBailOnConflict(t *testing.T) { output, err = cmd.Output() require.NoError(t, err, "failed to get git status ", string(output)) require.Empty(t, string(output), "working directory should be clean after aborted merge") + + // MERGE_HEAD must be gone after the abort. + require.NoFileExists(t, filepath.Join(tempDir, ".git", "MERGE_HEAD"), "MERGE_HEAD should not exist after abort") + }) + + t.Run("merge failure unrelated to conflicts surfaces as an error", func(t *testing.T) { + tempDir := setupTestRepository(t) + + success, err := MergeWithBailOnConflict(tempDir, "does-not-exist") + require.Error(t, err, "non-conflict failures must not be silently swallowed") + require.False(t, success) + }) +} + +func TestCommitAll(t *testing.T) { + t.Run("returns ErrEmptyCommit when there are no changes", func(t *testing.T) { + tempDir := setupTestRepository(t) + + _, err := CommitAll(context.Background(), tempDir, "", "noop", "Rill", "noreply@rilldata.com") + require.ErrorIs(t, err, ErrEmptyCommit) + }) + + t.Run("returns ErrEmptyCommit when changes exist but fall outside the pathspec", func(t *testing.T) { + tempDir := setupTestRepository(t) + + // Seed the pathspec directory with a committed file so the pathspec resolves to a real path. + require.NoError(t, os.MkdirAll(filepath.Join(tempDir, "sub"), 0755)) + require.NoError(t, os.WriteFile(filepath.Join(tempDir, "sub", "seed.txt"), []byte("seed"), 0644)) + require.NoError(t, execGit(tempDir, "add", "sub/seed.txt")) + require.NoError(t, execGit(tempDir, "commit", "-m", "seed")) + + // Introduce a change *outside* the pathspec. + require.NoError(t, os.WriteFile(filepath.Join(tempDir, "outside.txt"), []byte("outside"), 0644)) + + _, err := CommitAll(context.Background(), tempDir, "sub", "noop", "Rill", "noreply@rilldata.com") + require.ErrorIs(t, err, ErrEmptyCommit) + + // The outside file must not have been committed. + out, err := Run(context.Background(), tempDir, "status", "--porcelain") + require.NoError(t, err) + require.Contains(t, out, "outside.txt") + }) + + t.Run("only commits files matching the pathspec", func(t *testing.T) { + tempDir := setupTestRepository(t) + + require.NoError(t, os.MkdirAll(filepath.Join(tempDir, "sub"), 0755)) + require.NoError(t, os.WriteFile(filepath.Join(tempDir, "sub", "inside.txt"), []byte("inside"), 0644)) + require.NoError(t, os.WriteFile(filepath.Join(tempDir, "outside.txt"), []byte("outside"), 0644)) + + hash, err := CommitAll(context.Background(), tempDir, "sub", "scoped commit", "Rill", "noreply@rilldata.com") + require.NoError(t, err) + require.NotEmpty(t, hash) + + // The outside file should still be untracked (not committed). + out, err := Run(context.Background(), tempDir, "status", "--porcelain") + require.NoError(t, err) + require.Contains(t, out, "outside.txt") + require.NotContains(t, out, "inside.txt") + + // The commit should include only the inside file. + filesOut, err := Run(context.Background(), tempDir, "show", "--name-only", "--pretty=format:", "HEAD") + require.NoError(t, err) + require.Contains(t, filesOut, "sub/inside.txt") + require.NotContains(t, filesOut, "outside.txt") + }) + + t.Run("uses the provided author name and email", func(t *testing.T) { + tempDir := setupTestRepository(t) + + require.NoError(t, os.WriteFile(filepath.Join(tempDir, "new.txt"), []byte("hello"), 0644)) + + _, err := CommitAll(context.Background(), tempDir, "", "msg", "Rill Bot", "bot@rilldata.com") + require.NoError(t, err) + + name, err := Run(context.Background(), tempDir, "log", "-1", "--format=%an") + require.NoError(t, err) + require.Equal(t, "Rill Bot", name) + + email, err := Run(context.Background(), tempDir, "log", "-1", "--format=%ae") + require.NoError(t, err) + require.Equal(t, "bot@rilldata.com", email) + }) +} + +func TestFetchBranches(t *testing.T) { + t.Run("silently skips branches that do not exist on the remote", func(t *testing.T) { + remote, local := setupRemoteAndClone(t) + + // Add a new branch on the remote with a commit. + require.NoError(t, execGit(remote, "checkout", "-b", "feature")) + createCommit(t, remote, "f.txt", "f", "feature commit") + require.NoError(t, execGit(remote, "checkout", "main")) + + err := FetchBranches(context.Background(), local, "feature", "does-not-exist") + require.NoError(t, err, "missing branch must not produce an error") + + // The existing branch must have been fetched. + hash, err := Hash(context.Background(), local, "refs/remotes/origin/feature") + require.NoError(t, err) + require.NotEmpty(t, hash) + }) +} + +func TestHash(t *testing.T) { + t.Run("returns ErrRefNotFound for a missing ref", func(t *testing.T) { + tempDir := setupTestRepository(t) + + _, err := Hash(context.Background(), tempDir, "refs/heads/does-not-exist") + require.ErrorIs(t, err, ErrRefNotFound) }) + + t.Run("returns ErrRefNotFound for HEAD in an unborn repository", func(t *testing.T) { + tempDir := t.TempDir() + require.NoError(t, exec.Command("git", "init", tempDir).Run()) + require.NoError(t, exec.Command("git", "-C", tempDir, "checkout", "-b", "main").Run()) + setupGitConfig(t, tempDir) + + _, err := Hash(context.Background(), tempDir, "HEAD") + require.ErrorIs(t, err, ErrRefNotFound) + }) + + t.Run("returns the commit hash for HEAD", func(t *testing.T) { + tempDir := setupTestRepository(t) + + hash, err := Hash(context.Background(), tempDir, "HEAD") + require.NoError(t, err) + require.Len(t, hash, 40) + }) +} + +// setupRemoteAndClone creates a remote repository with a single commit on `main` +// and clones it into a separate local directory. It returns the remote and local paths. +func setupRemoteAndClone(t *testing.T) (string, string) { + remote := setupTestRepository(t) + local := t.TempDir() + // t.TempDir() pre-creates the directory; remove it so `git clone` can populate it. + require.NoError(t, os.RemoveAll(local)) + + err := Clone(context.Background(), local, remote, "main", false, false) + require.NoError(t, err) + setupGitConfig(t, local) + return remote, local +} + +func execGit(repoPath string, args ...string) error { + cmd := exec.Command("git", append([]string{"-C", repoPath}, args...)...) + out, err := cmd.CombinedOutput() + if err != nil { + return fmt.Errorf("git %v failed: %s: %w", args, string(out), err) + } + return nil } func setupTestRepository(t *testing.T) string {