Fix Rebase implementation (#1049)

app/api/controller/util.go

@@ -95,11 +95,12 @@ func MapCommit(c *git.Commit) (*types.Commit, error) {
 	}
 
 	return &types.Commit{
-		SHA:       c.SHA,
-		Title:     c.Title,
-		Message:   c.Message,
-		Author:    *author,
-		Committer: *committer,
+		SHA:        c.SHA,
+		ParentSHAs: c.ParentSHAs,
+		Title:      c.Title,
+		Message:    c.Message,
+		Author:     *author,
+		Committer:  *committer,
 	}, nil
 }
 

git/adapter/commit.go

@@ -574,14 +574,15 @@ func GetCommit(
 ) (*types.Commit, error) {
 	const format = "" +
 		fmtCommitHash + fmtZero + // 0
-		fmtAuthorName + fmtZero + // 1
-		fmtAuthorEmail + fmtZero + // 2
-		fmtAuthorTime + fmtZero + // 3
-		fmtCommitterName + fmtZero + // 4
-		fmtCommitterEmail + fmtZero + // 5
-		fmtCommitterTime + fmtZero + // 6
-		fmtSubject + fmtZero + // 7
-		fmtBody // 8
+		fmtParentHashes + fmtZero + // 1
+		fmtAuthorName + fmtZero + // 2
+		fmtAuthorEmail + fmtZero + // 3
+		fmtAuthorTime + fmtZero + // 4
+		fmtCommitterName + fmtZero + // 5
+		fmtCommitterEmail + fmtZero + // 6
+		fmtCommitterTime + fmtZero + // 7
+		fmtSubject + fmtZero + // 8
+		fmtBody // 9
 
 	cmd := command.New("log",
 		command.WithFlag("--max-count", "1"),
@@ -606,7 +607,7 @@ func GetCommit(
 		return nil, errors.InvalidArgument("path %q not found in %s", path, rev)
 	}
 
-	const columnCount = 9
+	const columnCount = 10
 
 	commitData := strings.Split(strings.TrimSpace(commitLine), separatorZero)
 	if len(commitData) != columnCount {
@@ -615,22 +616,24 @@ func GetCommit(
 	}
 
 	sha := commitData[0]
-	authorName := commitData[1]
-	authorEmail := commitData[2]
-	authorTimestamp := commitData[3]
-	committerName := commitData[4]
-	committerEmail := commitData[5]
-	committerTimestamp := commitData[6]
-	subject := commitData[7]
-	body := commitData[8]
+	parentSHAs := strings.Split(commitData[1], " ")
+	authorName := commitData[2]
+	authorEmail := commitData[3]
+	authorTimestamp := commitData[4]
+	committerName := commitData[5]
+	committerEmail := commitData[6]
+	committerTimestamp := commitData[7]
+	subject := commitData[8]
+	body := commitData[9]
 
 	authorTime, _ := time.Parse(time.RFC3339Nano, authorTimestamp)
 	committerTime, _ := time.Parse(time.RFC3339Nano, committerTimestamp)
 
 	return &types.Commit{
-		SHA:     sha,
-		Title:   subject,
-		Message: body,
+		SHA:        sha,
+		ParentSHAs: parentSHAs,
+		Title:      subject,
+		Message:    body,
 		Author: types.Signature{
 			Identity: types.Identity{
 				Name:  authorName,

git/adapter/format.go

@@ -19,9 +19,9 @@ const (
 	fmtPerc = "%%"
 	fmtZero = "%x00"
 
-	fmtCommitHash = "%H"
-	fmtTreeHash   = "%T"
-	fmtParentHash = "%T"
+	fmtCommitHash   = "%H"
+	fmtTreeHash     = "%T"
+	fmtParentHashes = "%P"
 
 	fmtAuthorName  = "%an"
 	fmtAuthorEmail = "%ae"

git/adapter/mapping.go

@@ -72,9 +72,14 @@ func mapGiteaCommit(giteaCommit *gitea.Commit) (*types.Commit, error) {
 	if err != nil {
 		return nil, fmt.Errorf("failed to map gitea commiter: %w", err)
 	}
+	parentShas := make([]string, len(giteaCommit.Parents))
+	for i := range giteaCommit.Parents {
+		parentShas[i] = giteaCommit.Parents[i].String()
+	}
 	return &types.Commit{
-		SHA:   giteaCommit.ID.String(),
-		Title: giteaCommit.Summary(),
+		SHA:        giteaCommit.ID.String(),
+		ParentSHAs: parentShas,
+		Title:      giteaCommit.Summary(),
 		// remove potential tailing newlines from message
 		Message:   strings.TrimRight(giteaCommit.Message(), "\n"),
 		Author:    author,

git/commit.go

@@ -30,12 +30,13 @@ type GetCommitParams struct {
 }
 
 type Commit struct {
-	SHA       string          `json:"sha"`
-	Title     string          `json:"title"`
-	Message   string          `json:"message,omitempty"`
-	Author    Signature       `json:"author"`
-	Committer Signature       `json:"committer"`
-	FileStats CommitFileStats `json:"file_stats,omitempty"`
+	SHA        string          `json:"sha"`
+	ParentSHAs []string        `json:"parent_shas,omitempty"`
+	Title      string          `json:"title"`
+	Message    string          `json:"message,omitempty"`
+	Author     Signature       `json:"author"`
+	Committer  Signature       `json:"committer"`
+	FileStats  CommitFileStats `json:"file_stats,omitempty"`
 }
 
 type GetCommitOutput struct {

git/mapping.go

@@ -56,12 +56,13 @@ func mapCommit(c *types.Commit) (*Commit, error) {
 		return nil, fmt.Errorf("failed to map rpc committer: %w", err)
 	}
 	return &Commit{
-		SHA:       c.SHA,
-		Title:     c.Title,
-		Message:   c.Message,
-		Author:    *author,
-		Committer: *comitter,
-		FileStats: *mapFileStats(&c.FileStats),
+		SHA:        c.SHA,
+		ParentSHAs: c.ParentSHAs,
+		Title:      c.Title,
+		Message:    c.Message,
+		Author:     *author,
+		Committer:  *comitter,
+		FileStats:  *mapFileStats(&c.FileStats),
 	}, nil
 }
 

git/merge/merge.go

@@ -21,6 +21,8 @@ import (
 	"github.com/harness/gitness/git/adapter"
 	"github.com/harness/gitness/git/sharedrepo"
 	"github.com/harness/gitness/git/types"
+
+	"github.com/rs/zerolog/log"
 )
 
 // Func represents a merge method function. The concrete merge implementation functions must have this signature.
@@ -108,6 +110,8 @@ func mergeInternal(
 }
 
 // Rebase merges two the commits (targetSHA and sourceSHA) using the Rebase method.
+//
+//nolint:gocognit // refactor if needed.
 func Rebase(
 	ctx context.Context,
 	repoPath, tmpDir string,
@@ -116,18 +120,19 @@ func Rebase(
 	mergeBaseSHA, targetSHA, sourceSHA string,
 ) (mergeSHA string, conflicts []string, err error) {
 	err = runInSharedRepo(ctx, tmpDir, repoPath, func(s *sharedrepo.SharedRepo) error {
-		sourceSHAs, err := s.CommitSHAList(ctx, mergeBaseSHA, sourceSHA)
+		sourceSHAs, err := s.CommitSHAsForRebase(ctx, mergeBaseSHA, sourceSHA)
 		if err != nil {
 			return fmt.Errorf("failed to find commit list in rebase merge: %w", err)
 		}
 
 		lastCommitSHA := targetSHA
+		lastTreeSHA, err := s.GetTreeSHA(ctx, targetSHA)
+		if err != nil {
+			return fmt.Errorf("failed to get tree sha for target: %w", err)
+		}
 
-		for i := len(sourceSHAs) - 1; i >= 0; i-- {
-			commitSHA := sourceSHAs[i]
-
+		for _, commitSHA := range sourceSHAs {
 			var treeSHA string
-			var commitConflicts []string
 
 			commitInfo, err := adapter.GetCommit(ctx, s.Directory(), commitSHA, "")
 			if err != nil {
@@ -141,29 +146,41 @@ func Rebase(
 				message += "\n\n" + commitInfo.Message
 			}
 
-			treeSHA, commitConflicts, err = s.MergeTree(ctx, mergeBaseSHA, lastCommitSHA, commitSHA)
+			mergeTreeMergeBaseSHA := ""
+			if len(commitInfo.ParentSHAs) > 0 {
+				// use parent of commit as merge base to only apply changes introduced by commit.
+				// See example usage of when --merge-base was introduced:
+				// https://github.com/git/git/commit/66265a693e8deb3ab86577eb7f69940410044081
+				//
+				// NOTE: CommitSHAsForRebase only returns non-merge commits.
+				mergeTreeMergeBaseSHA = commitInfo.ParentSHAs[0]
+			}
+
+			treeSHA, conflicts, err = s.MergeTree(ctx, mergeTreeMergeBaseSHA, lastCommitSHA, commitSHA)
 			if err != nil {
 				return fmt.Errorf("failed to merge tree in rebase merge: %w", err)
 			}
-
-			if len(commitConflicts) > 0 {
-				_, _, conflicts, err = FindConflicts(ctx, s.Directory(), targetSHA, sourceSHA)
-				if err != nil {
-					return fmt.Errorf("failed to find conflicts in rebase merge: %w", err)
-				}
-
-				if len(conflicts) == 0 {
-					return fmt.Errorf("expected to find conflicts after rebase merge between %s and %s, but couldn't",
-						mergeBaseSHA, sourceSHA)
-				}
-
+			if len(conflicts) > 0 {
 				return nil
 			}
 
+			// Drop any commit which after being rebased would be empty.
+			// There's two cases in which that can happen:
+			// 1. Empty commit.
+			//    Github is dropping empty commits, so we'll do the same.
+			// 2. The changes of the commit already exist on the target branch.
+			//    Git's `git rebase` is dropping such commits on default (and so does Github)
+			//    https://git-scm.com/docs/git-rebase#Documentation/git-rebase.txt---emptydropkeepask
+			if treeSHA == lastTreeSHA {
+				log.Ctx(ctx).Debug().Msgf("skipping commit %s as it's empty after rebase", commitSHA)
+				continue
+			}
+
 			lastCommitSHA, err = s.CommitTree(ctx, author, committer, treeSHA, message, false, lastCommitSHA)
 			if err != nil {
 				return fmt.Errorf("failed to commit tree in rebase merge: %w", err)
 			}
+			lastTreeSHA = treeSHA
 		}
 
 		mergeSHA = lastCommitSHA

git/sharedrepo/sharedrepo.go

@@ -209,6 +209,31 @@ func (r *SharedRepo) WriteGitObject(
 	return strings.TrimSpace(stdout.String()), nil
 }
 
+// GetTreeSHA returns the tree SHA of the rev.
+func (r *SharedRepo) GetTreeSHA(
+	ctx context.Context,
+	rev string,
+) (string, error) {
+	cmd := command.New("show",
+		command.WithFlag("--no-patch"),
+		command.WithFlag("--format=%T"),
+		command.WithArg(rev),
+	)
+	stdout := &bytes.Buffer{}
+	err := cmd.Run(ctx,
+		command.WithDir(r.temporaryPath),
+		command.WithStdout(stdout),
+	)
+	if err != nil {
+		if strings.Contains(err.Error(), "ambiguous argument") {
+			return "", errors.NotFound("could not resolve git revision %q", rev)
+		}
+		return "", fmt.Errorf("failed to get tree sha: %w", err)
+	}
+
+	return strings.TrimSpace(stdout.String()), nil
+}
+
 // ShowFile dumps show file and write to io.Writer.
 func (r *SharedRepo) ShowFile(
 	ctx context.Context,
@@ -356,12 +381,22 @@ func (r *SharedRepo) CommitTree(
 	return strings.TrimSpace(stdout.String()), nil
 }
 
-// CommitSHAList returns list of SHAs of the commits between the two git revisions.
-func (r *SharedRepo) CommitSHAList(
+// CommitSHAsForRebase returns list of SHAs of the commits between the two git revisions
+// for a rebase operation - in the order they should be rebased in.
+func (r *SharedRepo) CommitSHAsForRebase(
 	ctx context.Context,
-	start, end string,
+	target, source string,
 ) ([]string, error) {
-	cmd := command.New("rev-list", command.WithArg(start+".."+end))
+	// the command line arguments are mostly matching default `git rebase` behavior.
+	// Only difference is we use `--date-order` (matches github behavior) whilst `git rebase` uses `--topo-order`.
+	// Git Rebase's rev-list: https://github.com/git/git/blob/v2.41.0/sequencer.c#L5703-L5714
+	cmd := command.New("rev-list",
+		command.WithFlag("--max-parents=1"), // exclude merge commits
+		command.WithFlag("--cherry-pick"),   // drop commits that already exist on target
+		command.WithFlag("--reverse"),
+		command.WithFlag("--right-only"), // only return commits from source
+		command.WithFlag("--date-order"), // childs always before parents, otherwise by commit time stamp
+		command.WithArg(target+"..."+source))
 
 	stdout := bytes.NewBuffer(nil)
 

git/types/types.go

@@ -141,12 +141,13 @@ type WalkReferencesOptions struct {
 }
 
 type Commit struct {
-	SHA       string          `json:"sha"`
-	Title     string          `json:"title"`
-	Message   string          `json:"message,omitempty"`
-	Author    Signature       `json:"author"`
-	Committer Signature       `json:"committer"`
-	FileStats CommitFileStats `json:"file_stats,omitempty"`
+	SHA        string          `json:"sha"`
+	ParentSHAs []string        `json:"parent_shas,omitempty"`
+	Title      string          `json:"title"`
+	Message    string          `json:"message,omitempty"`
+	Author     Signature       `json:"author"`
+	Committer  Signature       `json:"committer"`
+	FileStats  CommitFileStats `json:"file_stats,omitempty"`
 }
 
 type CommitFileStats struct {

types/git.go

@@ -57,11 +57,12 @@ type TagFilter struct {
 }
 
 type Commit struct {
-	SHA       string    `json:"sha"`
-	Title     string    `json:"title"`
-	Message   string    `json:"message"`
-	Author    Signature `json:"author"`
-	Committer Signature `json:"committer"`
+	SHA        string    `json:"sha"`
+	ParentSHAs []string  `json:"parent_shas,omitempty"`
+	Title      string    `json:"title"`
+	Message    string    `json:"message"`
+	Author     Signature `json:"author"`
+	Committer  Signature `json:"committer"`
 }
 
 type Signature struct {