From dd11c0eff56b1c0052277751cd355c6f0065d68b Mon Sep 17 00:00:00 2001 From: Johannes Batzill Date: Tue, 20 Feb 2024 17:45:09 +0000 Subject: [PATCH] Fix `Rebase` implementation (#1049) --- app/api/controller/util.go | 11 ++++---- git/adapter/commit.go | 43 +++++++++++++++-------------- git/adapter/format.go | 6 ++-- git/adapter/mapping.go | 9 ++++-- git/commit.go | 13 +++++---- git/mapping.go | 13 +++++---- git/merge/merge.go | 53 ++++++++++++++++++++++++------------ git/sharedrepo/sharedrepo.go | 43 ++++++++++++++++++++++++++--- git/types/types.go | 13 +++++---- types/git.go | 11 ++++---- 10 files changed, 140 insertions(+), 75 deletions(-) diff --git a/app/api/controller/util.go b/app/api/controller/util.go index 2fc655607..2e0299c6a 100644 --- a/app/api/controller/util.go +++ b/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 } diff --git a/git/adapter/commit.go b/git/adapter/commit.go index 94e6b19ea..9db604ea9 100644 --- a/git/adapter/commit.go +++ b/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, diff --git a/git/adapter/format.go b/git/adapter/format.go index 4033118e5..eea73051d 100644 --- a/git/adapter/format.go +++ b/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" diff --git a/git/adapter/mapping.go b/git/adapter/mapping.go index a608dc55b..2d47c3f76 100644 --- a/git/adapter/mapping.go +++ b/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, diff --git a/git/commit.go b/git/commit.go index 08c74d49d..5ed2b378f 100644 --- a/git/commit.go +++ b/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 { diff --git a/git/mapping.go b/git/mapping.go index d778ad70b..ac35ba57d 100644 --- a/git/mapping.go +++ b/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 } diff --git a/git/merge/merge.go b/git/merge/merge.go index a94978436..400552913 100644 --- a/git/merge/merge.go +++ b/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 diff --git a/git/sharedrepo/sharedrepo.go b/git/sharedrepo/sharedrepo.go index 77236d233..fbfc5a553 100644 --- a/git/sharedrepo/sharedrepo.go +++ b/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) diff --git a/git/types/types.go b/git/types/types.go index 084a11592..676c840bf 100644 --- a/git/types/types.go +++ b/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 { diff --git a/types/git.go b/types/git.go index b89a9be68..53abfa4a0 100644 --- a/types/git.go +++ b/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 {