From 860818728ae477fffeb665de7219df7d66f72aad Mon Sep 17 00:00:00 2001 From: oppiliappan Date: Wed, 28 Jan 2026 11:28:51 +0000 Subject: [PATCH] knotserver/git: rework merge check to also use `git am` we were using `git apply` in merge check and `git am` for the actual merge, but in reality, there are slight behavior changes among the two. this change switches out `git apply` in `mergeCheck` to use `git am` when dealing with mailbox style patches. Signed-off-by: oppiliappan --- knotserver/git/merge.go | 60 ++++++++++++++++++++++++++++++------------------------------ knotserver/xrpc/merge_check.go | 8 +++++++- 2 file(s) changed, 37 insertion(s)(+), 31 deletion(s)(-) diff --git a/knotserver/git/merge.go b/knotserver/git/merge.go --- a/knotserver/git/merge.go +++ b/knotserver/git/merge.go @@ -107,13 +107,13 @@ } return fmt.Sprintf("merge failed: %s", e.Message) } -func (g *GitRepo) createTempFileWithPatch(patchData string) (string, error) { +func createTemp(data string) (string, error) { tmpFile, err := os.CreateTemp("", "git-patch-*.patch") if err != nil { return "", fmt.Errorf("failed to create temporary patch file: %w", err) } - if _, err := tmpFile.Write([]byte(patchData)); err != nil { + if _, err := tmpFile.Write([]byte(data)); err != nil { tmpFile.Close() os.Remove(tmpFile.Name()) return "", fmt.Errorf("failed to write patch data to temporary file: %w", err) @@ -127,7 +127,7 @@ return tmpFile.Name(), nil } -func (g *GitRepo) cloneRepository(targetBranch string) (string, error) { +func (g *GitRepo) cloneTemp(targetBranch string) (string, error) { tmpDir, err := os.MkdirTemp("", "git-clone-") if err != nil { return "", fmt.Errorf("failed to create temporary directory: %w", err) @@ -147,24 +147,6 @@ return tmpDir, nil } -func (g *GitRepo) checkPatch(tmpDir, patchFile string) error { - var stderr bytes.Buffer - - cmd := exec.Command("git", "-C", tmpDir, "apply", "--check", "-v", patchFile) - cmd.Stderr = &stderr - - if err := cmd.Run(); err != nil { - conflicts := parseGitApplyErrors(stderr.String()) - return &ErrMerge{ - Message: "patch cannot be applied cleanly", - Conflicts: conflicts, - HasConflict: len(conflicts) > 0, - OtherError: err, - } - } - return nil -} - func (g *GitRepo) applyPatch(patchData, patchFile string, opts MergeOptions) error { var stderr bytes.Buffer var cmd *exec.Cmd @@ -173,6 +155,7 @@ // configure default git user before merge exec.Command("git", "-C", g.path, "config", "user.name", opts.CommitterName).Run() exec.Command("git", "-C", g.path, "config", "user.email", opts.CommitterEmail).Run() exec.Command("git", "-C", g.path, "config", "advice.mergeConflict", "false").Run() + exec.Command("git", "-C", g.path, "config", "advice.amWorkDir", "false").Run() // if patch is a format-patch, apply using 'git am' if opts.FormatPatch { @@ -213,7 +196,13 @@ cmd.Stderr = &stderr if err := cmd.Run(); err != nil { - return fmt.Errorf("patch application failed: %s", stderr.String()) + conflicts := parseGitApplyErrors(stderr.String()) + return &ErrMerge{ + Message: "patch cannot be applied cleanly", + Conflicts: conflicts, + HasConflict: len(conflicts) > 0, + OtherError: err, + } } return nil @@ -241,7 +230,7 @@ return nil } func (g *GitRepo) applySingleMailbox(singlePatch types.FormatPatch) (plumbing.Hash, error) { - tmpPatch, err := g.createTempFileWithPatch(singlePatch.Raw) + tmpPatch, err := createTemp(singlePatch.Raw) if err != nil { return plumbing.ZeroHash, fmt.Errorf("failed to create temporary patch file for singluar mailbox patch: %w", err) } @@ -257,7 +246,13 @@ } log.Println("head before apply", head.Hash().String()) if err := cmd.Run(); err != nil { - return plumbing.ZeroHash, fmt.Errorf("patch application failed: %s", stderr.String()) + conflicts := parseGitApplyErrors(stderr.String()) + return plumbing.ZeroHash, &ErrMerge{ + Message: "patch cannot be applied cleanly", + Conflicts: conflicts, + HasConflict: len(conflicts) > 0, + OtherError: err, + } } if err := g.Refresh(); err != nil { @@ -324,12 +319,12 @@ // new hash of commit return newHash, nil } -func (g *GitRepo) MergeCheck(patchData string, targetBranch string) error { +func (g *GitRepo) MergeCheckWithOptions(patchData string, targetBranch string, mo MergeOptions) error { if val, ok := mergeCheckCache.Get(g, patchData, targetBranch); ok { return val } - patchFile, err := g.createTempFileWithPatch(patchData) + patchFile, err := createTemp(patchData) if err != nil { return &ErrMerge{ Message: err.Error(), @@ -338,7 +333,7 @@ } } defer os.Remove(patchFile) - tmpDir, err := g.cloneRepository(targetBranch) + tmpDir, err := g.cloneTemp(targetBranch) if err != nil { return &ErrMerge{ Message: err.Error(), @@ -347,13 +342,18 @@ } } defer os.RemoveAll(tmpDir) - result := g.checkPatch(tmpDir, patchFile) + tmpRepo, err := PlainOpen(tmpDir) + if err != nil { + return err + } + + result := tmpRepo.applyPatch(patchData, patchFile, mo) mergeCheckCache.Set(g, patchData, targetBranch, result) return result } func (g *GitRepo) MergeWithOptions(patchData string, targetBranch string, opts MergeOptions) error { - patchFile, err := g.createTempFileWithPatch(patchData) + patchFile, err := createTemp(patchData) if err != nil { return &ErrMerge{ Message: err.Error(), @@ -362,7 +362,7 @@ } } defer os.Remove(patchFile) - tmpDir, err := g.cloneRepository(targetBranch) + tmpDir, err := g.cloneTemp(targetBranch) if err != nil { return &ErrMerge{ Message: err.Error(), diff --git a/knotserver/xrpc/merge_check.go b/knotserver/xrpc/merge_check.go --- a/knotserver/xrpc/merge_check.go +++ b/knotserver/xrpc/merge_check.go @@ -9,6 +9,7 @@ securejoin "github.com/cyphar/filepath-securejoin" "tangled.org/core/api/tangled" "tangled.org/core/knotserver/git" + "tangled.org/core/patchutil" xrpcerr "tangled.org/core/xrpc/errors" ) @@ -51,7 +52,12 @@ fail(xrpcerr.GenericError(fmt.Errorf("failed to open repository: %w", err))) return } - err = gr.MergeCheck(data.Patch, data.Branch) + mo := git.MergeOptions{} + mo.CommitterName = x.Config.Git.UserName + mo.CommitterEmail = x.Config.Git.UserEmail + mo.FormatPatch = patchutil.IsFormatPatch(data.Patch) + + err = gr.MergeCheckWithOptions(data.Patch, data.Branch, mo) response := tangled.RepoMergeCheck_Output{ Is_conflicted: false, -- tangled.sh