Refactor and tidy-up the merge/update branch code (#22568)

The merge and update branch code was previously a little tangled and had
some very long functions. The functions were not very clear in their
reasoning and there were deficiencies in their logging and at least one
bug in the handling of LFS for update by rebase.

This PR substantially refactors this code and splits things out to into
separate functions. It also attempts to tidy up the calls by wrapping
things in "context"s. There are also attempts to improve logging when
there are errors.

Signed-off-by: Andrew Thornton <art27@cantab.net>

---------

Signed-off-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: techknowlogick <techknowlogick@gitea.io>
Co-authored-by: delvh <dev.lh@web.de>

modules/git/pipeline/revlist.go

@@ -42,7 +42,10 @@ func RevListObjects(ctx context.Context, revListWriter *io.PipeWriter, wg *sync.
 	defer revListWriter.Close()
 	stderr := new(bytes.Buffer)
 	var errbuf strings.Builder
-	cmd := git.NewCommand(ctx, "rev-list", "--objects").AddDynamicArguments(headSHA).AddArguments("--not").AddDynamicArguments(baseSHA)
+	cmd := git.NewCommand(ctx, "rev-list", "--objects").AddDynamicArguments(headSHA)
+	if baseSHA != "" {
+		cmd = cmd.AddArguments("--not").AddDynamicArguments(baseSHA)
+	}
 	if err := cmd.Run(&git.RunOpts{
 		Dir:    tmpBasePath,
 		Stdout: revListWriter,

services/pull/lfs.go

@@ -116,7 +116,7 @@ func createLFSMetaObjectsFromCatFileBatch(catFileBatchReader *io.PipeReader, wg
 		}
 
 		// Then we need to check that this pointer is in the db
-		if _, err := git_model.GetLFSMetaObjectByOid(db.DefaultContext, pr.HeadRepo.ID, pointer.Oid); err != nil {
+		if _, err := git_model.GetLFSMetaObjectByOid(db.DefaultContext, pr.HeadRepoID, pointer.Oid); err != nil {
 			if err == git_model.ErrLFSObjectNotExist {
 				log.Warn("During merge of: %d in %-v, there is a pointer to LFS Oid: %s which although present in the LFS store is not associated with the head repo %-v", pr.Index, pr.BaseRepo, pointer.Oid, pr.HeadRepo)
 				continue

services/pull/merge_merge.go

@@ -0,0 +1,25 @@
+// Copyright 2023 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package pull
+
+import (
+	repo_model "code.gitea.io/gitea/models/repo"
+	"code.gitea.io/gitea/modules/git"
+	"code.gitea.io/gitea/modules/log"
+)
+
+// doMergeStyleMerge merges the tracking into the current HEAD - which is assumed to tbe staging branch (equal to the pr.BaseBranch)
+func doMergeStyleMerge(ctx *mergeContext, message string) error {
+	cmd := git.NewCommand(ctx, "merge", "--no-ff", "--no-commit").AddDynamicArguments(trackingBranch)
+	if err := runMergeCommand(ctx, repo_model.MergeStyleMerge, cmd); err != nil {
+		log.Error("%-v Unable to merge tracking into base: %v", ctx.pr, err)
+		return err
+	}
+
+	if err := commitAndSignNoAuthor(ctx, message); err != nil {
+		log.Error("%-v Unable to make final commit: %v", ctx.pr, err)
+		return err
+	}
+	return nil
+}

services/pull/merge_prepare.go

@@ -0,0 +1,297 @@
+// Copyright 2023 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package pull
+
+import (
+	"bufio"
+	"bytes"
+	"context"
+	"fmt"
+	"io"
+	"os"
+	"path/filepath"
+	"strings"
+	"time"
+
+	"code.gitea.io/gitea/models"
+	issues_model "code.gitea.io/gitea/models/issues"
+	repo_model "code.gitea.io/gitea/models/repo"
+	user_model "code.gitea.io/gitea/models/user"
+	"code.gitea.io/gitea/modules/git"
+	"code.gitea.io/gitea/modules/log"
+	asymkey_service "code.gitea.io/gitea/services/asymkey"
+)
+
+type mergeContext struct {
+	*prContext
+	doer      *user_model.User
+	sig       *git.Signature
+	committer *git.Signature
+	signArg   git.TrustedCmdArgs
+	env       []string
+}
+
+func (ctx *mergeContext) RunOpts() *git.RunOpts {
+	ctx.outbuf.Reset()
+	ctx.errbuf.Reset()
+	return &git.RunOpts{
+		Env:    ctx.env,
+		Dir:    ctx.tmpBasePath,
+		Stdout: ctx.outbuf,
+		Stderr: ctx.errbuf,
+	}
+}
+
+func createTemporaryRepoForMerge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User, expectedHeadCommitID string) (mergeCtx *mergeContext, cancel context.CancelFunc, err error) {
+	// Clone base repo.
+	prCtx, cancel, err := createTemporaryRepoForPR(ctx, pr)
+	if err != nil {
+		log.Error("createTemporaryRepoForPR: %v", err)
+		return nil, cancel, err
+	}
+
+	mergeCtx = &mergeContext{
+		prContext: prCtx,
+		doer:      doer,
+	}
+
+	if expectedHeadCommitID != "" {
+		trackingCommitID, _, err := git.NewCommand(ctx, "show-ref", "--hash").AddDynamicArguments(git.BranchPrefix + trackingBranch).RunStdString(&git.RunOpts{Dir: mergeCtx.tmpBasePath})
+		if err != nil {
+			defer cancel()
+			log.Error("failed to get sha of head branch in %-v: show-ref[%s] --hash refs/heads/tracking: %v", mergeCtx.pr, mergeCtx.tmpBasePath, err)
+			return nil, nil, fmt.Errorf("unable to get sha of head branch in %v %w", pr, err)
+		}
+		if strings.TrimSpace(trackingCommitID) != expectedHeadCommitID {
+			defer cancel()
+			return nil, nil, models.ErrSHADoesNotMatch{
+				GivenSHA:   expectedHeadCommitID,
+				CurrentSHA: trackingCommitID,
+			}
+		}
+	}
+
+	mergeCtx.outbuf.Reset()
+	mergeCtx.errbuf.Reset()
+	if err := prepareTemporaryRepoForMerge(mergeCtx); err != nil {
+		defer cancel()
+		return nil, nil, err
+	}
+
+	mergeCtx.sig = doer.NewGitSig()
+	mergeCtx.committer = mergeCtx.sig
+
+	// Determine if we should sign
+	sign, keyID, signer, _ := asymkey_service.SignMerge(ctx, mergeCtx.pr, mergeCtx.doer, mergeCtx.tmpBasePath, "HEAD", trackingBranch)
+	if sign {
+		mergeCtx.signArg = git.ToTrustedCmdArgs([]string{"-S" + keyID})
+		if pr.BaseRepo.GetTrustModel() == repo_model.CommitterTrustModel || pr.BaseRepo.GetTrustModel() == repo_model.CollaboratorCommitterTrustModel {
+			mergeCtx.committer = signer
+		}
+	} else {
+		mergeCtx.signArg = git.ToTrustedCmdArgs([]string{"--no-gpg-sign"})
+	}
+
+	commitTimeStr := time.Now().Format(time.RFC3339)
+
+	// Because this may call hooks we should pass in the environment
+	mergeCtx.env = append(os.Environ(),
+		"GIT_AUTHOR_NAME="+mergeCtx.sig.Name,
+		"GIT_AUTHOR_EMAIL="+mergeCtx.sig.Email,
+		"GIT_AUTHOR_DATE="+commitTimeStr,
+		"GIT_COMMITTER_NAME="+mergeCtx.committer.Name,
+		"GIT_COMMITTER_EMAIL="+mergeCtx.committer.Email,
+		"GIT_COMMITTER_DATE="+commitTimeStr,
+	)
+
+	return mergeCtx, cancel, nil
+}
+
+// prepareTemporaryRepoForMerge takes a repository that has been created using createTemporaryRepo
+// it then sets up the sparse-checkout and other things
+func prepareTemporaryRepoForMerge(ctx *mergeContext) error {
+	infoPath := filepath.Join(ctx.tmpBasePath, ".git", "info")
+	if err := os.MkdirAll(infoPath, 0o700); err != nil {
+		log.Error("%-v Unable to create .git/info in %s: %v", ctx.pr, ctx.tmpBasePath, err)
+		return fmt.Errorf("Unable to create .git/info in tmpBasePath: %w", err)
+	}
+
+	// Enable sparse-checkout
+	// Here we use the .git/info/sparse-checkout file as described in the git documentation
+	sparseCheckoutListFile, err := os.OpenFile(filepath.Join(infoPath, "sparse-checkout"), os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o600)
+	if err != nil {
+		log.Error("%-v Unable to write .git/info/sparse-checkout file in %s: %v", ctx.pr, ctx.tmpBasePath, err)
+		return fmt.Errorf("Unable to write .git/info/sparse-checkout file in tmpBasePath: %w", err)
+	}
+	defer sparseCheckoutListFile.Close() // we will close it earlier but we need to ensure it is closed if there is an error
+
+	if err := getDiffTree(ctx, ctx.tmpBasePath, baseBranch, trackingBranch, sparseCheckoutListFile); err != nil {
+		log.Error("%-v getDiffTree(%s, %s, %s): %v", ctx.pr, ctx.tmpBasePath, baseBranch, trackingBranch, err)
+		return fmt.Errorf("getDiffTree: %w", err)
+	}
+
+	if err := sparseCheckoutListFile.Close(); err != nil {
+		log.Error("%-v Unable to close .git/info/sparse-checkout file in %s: %v", ctx.pr, ctx.tmpBasePath, err)
+		return fmt.Errorf("Unable to close .git/info/sparse-checkout file in tmpBasePath: %w", err)
+	}
+
+	gitConfigCommand := func() *git.Command {
+		return git.NewCommand(ctx, "config", "--local")
+	}
+
+	setConfig := func(key, value string) error {
+		if err := gitConfigCommand().AddArguments(git.ToTrustedCmdArgs([]string{key, value})...).
+			Run(ctx.RunOpts()); err != nil {
+			if value == "" {
+				value = "<>"
+			}
+			log.Error("git config [%s -> %s ]: %v\n%s\n%s", key, value, err, ctx.outbuf.String(), ctx.errbuf.String())
+			return fmt.Errorf("git config [%s -> %s ]: %w\n%s\n%s", key, value, err, ctx.outbuf.String(), ctx.errbuf.String())
+		}
+		ctx.outbuf.Reset()
+		ctx.errbuf.Reset()
+
+		return nil
+	}
+
+	// Switch off LFS process (set required, clean and smudge here also)
+	if err := setConfig("filter.lfs.process", ""); err != nil {
+		return err
+	}
+
+	if err := setConfig("filter.lfs.required", "false"); err != nil {
+		return err
+	}
+
+	if err := setConfig("filter.lfs.clean", ""); err != nil {
+		return err
+	}
+
+	if err := setConfig("filter.lfs.smudge", ""); err != nil {
+		return err
+	}
+
+	if err := setConfig("core.sparseCheckout", "true"); err != nil {
+		return err
+	}
+
+	// Read base branch index
+	if err := git.NewCommand(ctx, "read-tree", "HEAD").
+		Run(ctx.RunOpts()); err != nil {
+		log.Error("git read-tree HEAD: %v\n%s\n%s", err, ctx.outbuf.String(), ctx.errbuf.String())
+		return fmt.Errorf("Unable to read base branch in to the index: %w\n%s\n%s", err, ctx.outbuf.String(), ctx.errbuf.String())
+	}
+	ctx.outbuf.Reset()
+	ctx.errbuf.Reset()
+
+	return nil
+}
+
+// getDiffTree returns a string containing all the files that were changed between headBranch and baseBranch
+// the filenames are escaped so as to fit the format required for .git/info/sparse-checkout
+func getDiffTree(ctx context.Context, repoPath, baseBranch, headBranch string, out io.Writer) error {
+	diffOutReader, diffOutWriter, err := os.Pipe()
+	if err != nil {
+		log.Error("Unable to create os.Pipe for %s", repoPath)
+		return err
+	}
+	defer func() {
+		_ = diffOutReader.Close()
+		_ = diffOutWriter.Close()
+	}()
+
+	scanNullTerminatedStrings := func(data []byte, atEOF bool) (advance int, token []byte, err error) {
+		if atEOF && len(data) == 0 {
+			return 0, nil, nil
+		}
+		if i := bytes.IndexByte(data, '\x00'); i >= 0 {
+			return i + 1, data[0:i], nil
+		}
+		if atEOF {
+			return len(data), data, nil
+		}
+		return 0, nil, nil
+	}
+
+	err = git.NewCommand(ctx, "diff-tree", "--no-commit-id", "--name-only", "-r", "-r", "-z", "--root").AddDynamicArguments(baseBranch, headBranch).
+		Run(&git.RunOpts{
+			Dir:    repoPath,
+			Stdout: diffOutWriter,
+			PipelineFunc: func(ctx context.Context, cancel context.CancelFunc) error {
+				// Close the writer end of the pipe to begin processing
+				_ = diffOutWriter.Close()
+				defer func() {
+					// Close the reader on return to terminate the git command if necessary
+					_ = diffOutReader.Close()
+				}()
+
+				// Now scan the output from the command
+				scanner := bufio.NewScanner(diffOutReader)
+				scanner.Split(scanNullTerminatedStrings)
+				for scanner.Scan() {
+					filepath := scanner.Text()
+					// escape '*', '?', '[', spaces and '!' prefix
+					filepath = escapedSymbols.ReplaceAllString(filepath, `\$1`)
+					// no necessary to escape the first '#' symbol because the first symbol is '/'
+					fmt.Fprintf(out, "/%s\n", filepath)
+				}
+				return scanner.Err()
+			},
+		})
+	return err
+}
+
+// rebaseTrackingOnToBase checks out the tracking branch as staging and rebases it on to the base branch
+// if there is a conflict it will return a models.ErrRebaseConflicts
+func rebaseTrackingOnToBase(ctx *mergeContext, mergeStyle repo_model.MergeStyle) error {
+	// Checkout head branch
+	if err := git.NewCommand(ctx, "checkout", "-b").AddDynamicArguments(stagingBranch, trackingBranch).
+		Run(ctx.RunOpts()); err != nil {
+		return fmt.Errorf("unable to git checkout tracking as staging in temp repo for %v: %w\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String())
+	}
+	ctx.outbuf.Reset()
+	ctx.errbuf.Reset()
+
+	// Rebase before merging
+	if err := git.NewCommand(ctx, "rebase").AddDynamicArguments(baseBranch).
+		Run(ctx.RunOpts()); err != nil {
+		// Rebase will leave a REBASE_HEAD file in .git if there is a conflict
+		if _, statErr := os.Stat(filepath.Join(ctx.tmpBasePath, ".git", "REBASE_HEAD")); statErr == nil {
+			var commitSha string
+			ok := false
+			failingCommitPaths := []string{
+				filepath.Join(ctx.tmpBasePath, ".git", "rebase-apply", "original-commit"), // Git < 2.26
+				filepath.Join(ctx.tmpBasePath, ".git", "rebase-merge", "stopped-sha"),     // Git >= 2.26
+			}
+			for _, failingCommitPath := range failingCommitPaths {
+				if _, statErr := os.Stat(failingCommitPath); statErr == nil {
+					commitShaBytes, readErr := os.ReadFile(failingCommitPath)
+					if readErr != nil {
+						// Abandon this attempt to handle the error
+						return fmt.Errorf("unable to git rebase staging on to base in temp repo for %v: %w\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String())
+					}
+					commitSha = strings.TrimSpace(string(commitShaBytes))
+					ok = true
+					break
+				}
+			}
+			if !ok {
+				log.Error("Unable to determine failing commit sha for failing rebase in temp repo for %-v. Cannot cast as models.ErrRebaseConflicts.", ctx.pr)
+				return fmt.Errorf("unable to git rebase staging on to base in temp repo for %v: %w\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String())
+			}
+			log.Debug("Conflict when rebasing staging on to base in %-v at %s: %v\n%s\n%s", ctx.pr, commitSha, err, ctx.outbuf.String(), ctx.errbuf.String())
+			return models.ErrRebaseConflicts{
+				CommitSHA: commitSha,
+				Style:     mergeStyle,
+				StdOut:    ctx.outbuf.String(),
+				StdErr:    ctx.errbuf.String(),
+				Err:       err,
+			}
+		}
+		return fmt.Errorf("unable to git rebase staging on to base in temp repo for %v: %w\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String())
+	}
+	ctx.outbuf.Reset()
+	ctx.errbuf.Reset()
+	return nil
+}

services/pull/merge_rebase.go

@@ -0,0 +1,50 @@
+// Copyright 2023 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package pull
+
+import (
+	"fmt"
+
+	repo_model "code.gitea.io/gitea/models/repo"
+	"code.gitea.io/gitea/modules/git"
+	"code.gitea.io/gitea/modules/log"
+)
+
+// doMergeStyleRebase rebaases the tracking branch on the base branch as the current HEAD with or with a merge commit to the original pr branch
+func doMergeStyleRebase(ctx *mergeContext, mergeStyle repo_model.MergeStyle, message string) error {
+	if err := rebaseTrackingOnToBase(ctx, mergeStyle); err != nil {
+		return err
+	}
+
+	// Checkout base branch again
+	if err := git.NewCommand(ctx, "checkout").AddDynamicArguments(baseBranch).
+		Run(ctx.RunOpts()); err != nil {
+		log.Error("git checkout base prior to merge post staging rebase %-v: %v\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String())
+		return fmt.Errorf("git checkout base prior to merge post staging rebase  %v: %w\n%s\n%s", ctx.pr, err, ctx.outbuf.String(), ctx.errbuf.String())
+	}
+	ctx.outbuf.Reset()
+	ctx.errbuf.Reset()
+
+	cmd := git.NewCommand(ctx, "merge")
+	if mergeStyle == repo_model.MergeStyleRebase {
+		cmd.AddArguments("--ff-only")
+	} else {
+		cmd.AddArguments("--no-ff", "--no-commit")
+	}
+	cmd.AddDynamicArguments(stagingBranch)
+
+	// Prepare merge with commit
+	if err := runMergeCommand(ctx, mergeStyle, cmd); err != nil {
+		log.Error("Unable to merge staging into base: %v", err)
+		return err
+	}
+	if mergeStyle == repo_model.MergeStyleRebaseMerge {
+		if err := commitAndSignNoAuthor(ctx, message); err != nil {
+			log.Error("Unable to make final commit: %v", err)
+			return err
+		}
+	}
+
+	return nil
+}