From 4b8684bbb6bff7adbb00d08b071b3616e0802e81 Mon Sep 17 00:00:00 2001 From: Brecht Van Lommel Date: Sat, 11 Feb 2023 03:26:41 +0100 Subject: [PATCH 1/4] Fix pull request update showing too many commits with multiple branches When the base repository contains multiple branches with the same commits as the base branch, pull requests can show a long list of commits already in the base branch as having been added. What this is supposed to do is exclude commits already in the base branch. But the mechanism to do so assumed a commit only exists in a single branch. Now use `git rev-list A B --not branchName` instead of filtering commits afterwards. The logic to detect if there was a force push also was wrong for multiple branches. If the old commit existed in any branch in the base repository it would assume there was no force push. Instead check if the old commit is an ancestor of the new commit. --- modules/git/repo_commit.go | 22 ++++++++++ services/pull/comment.go | 89 +++++--------------------------------- 2 files changed, 34 insertions(+), 77 deletions(-) diff --git a/modules/git/repo_commit.go b/modules/git/repo_commit.go index a62e0670fedca..32e9f560835e0 100644 --- a/modules/git/repo_commit.go +++ b/modules/git/repo_commit.go @@ -323,6 +323,28 @@ func (repo *Repository) CommitsBetweenLimit(last, before *Commit, limit, skip in return repo.parsePrettyFormatLogToList(bytes.TrimSpace(stdout)) } +// CommitsBetween returns a list that contains commits between [before, last), +// excluding commits in baseBranch. +// If before is detached (removed by reset + push) it is not included. +func (repo *Repository) CommitsBetweenSkipBase(last, before *Commit, baseBranch string) ([]*Commit, error) { + var stdout []byte + var err error + if before == nil { + stdout, _, err = NewCommand(repo.Ctx, "rev-list").AddDynamicArguments(last.ID.String()).AddArguments("--not").AddDynamicArguments(baseBranch).RunStdBytes(&RunOpts{Dir: repo.Path}) + } else { + stdout, _, err = NewCommand(repo.Ctx, "rev-list").AddDynamicArguments(before.ID.String() + ".." + last.ID.String()).AddArguments("--not").AddDynamicArguments(baseBranch).RunStdBytes(&RunOpts{Dir: repo.Path}) + if err != nil && strings.Contains(err.Error(), "no merge base") { + // future versions of git >= 2.28 are likely to return an error if before and last have become unrelated. + // previously it would return the results of git rev-list before last so let's try that... + stdout, _, err = NewCommand(repo.Ctx, "rev-list").AddDynamicArguments(before.ID.String(), last.ID.String()).AddArguments("--not").AddDynamicArguments(baseBranch).RunStdBytes(&RunOpts{Dir: repo.Path}) + } + } + if err != nil { + return nil, err + } + return repo.parsePrettyFormatLogToList(bytes.TrimSpace(stdout)) +} + // CommitsBetweenIDs return commits between twoe commits func (repo *Repository) CommitsBetweenIDs(last, before string) ([]*Commit, error) { lastCommit, err := repo.GetCommit(last) diff --git a/services/pull/comment.go b/services/pull/comment.go index 068aca6cd128b..1e4aa03c8f006 100644 --- a/services/pull/comment.go +++ b/services/pull/comment.go @@ -14,58 +14,6 @@ import ( issue_service "code.gitea.io/gitea/services/issue" ) -type commitBranchCheckItem struct { - Commit *git.Commit - Checked bool -} - -func commitBranchCheck(gitRepo *git.Repository, startCommit *git.Commit, endCommitID, baseBranch string, commitList map[string]*commitBranchCheckItem) error { - if startCommit.ID.String() == endCommitID { - return nil - } - - checkStack := make([]string, 0, 10) - checkStack = append(checkStack, startCommit.ID.String()) - - for len(checkStack) > 0 { - commitID := checkStack[0] - checkStack = checkStack[1:] - - item, ok := commitList[commitID] - if !ok { - continue - } - - if item.Commit.ID.String() == endCommitID { - continue - } - - if err := item.Commit.LoadBranchName(); err != nil { - return err - } - - if item.Commit.Branch == baseBranch { - continue - } - - if item.Checked { - continue - } - - item.Checked = true - - parentNum := item.Commit.ParentCount() - for i := 0; i < parentNum; i++ { - parentCommit, err := item.Commit.Parent(i) - if err != nil { - return err - } - checkStack = append(checkStack, parentCommit.ID.String()) - } - } - return nil -} - // getCommitIDsFromRepo get commit IDs from repo in between oldCommitID and newCommitID // isForcePush will be true if oldCommit isn't on the branch // Commit on baseBranch will skip @@ -82,11 +30,18 @@ func getCommitIDsFromRepo(ctx context.Context, repo *repo_model.Repository, oldC return nil, false, err } - if err = oldCommit.LoadBranchName(); err != nil { + newCommit, err := gitRepo.GetCommit(newCommitID) + if err != nil { + return nil, false, err + } + + // If old commit is not an ancestor of new commits, it's a force push + isAncestor, err := newCommit.HasPreviousCommit(oldCommit.ID) + if err != nil { return nil, false, err } - if len(oldCommit.Branch) == 0 { + if !isAncestor { commitIDs = make([]string, 2) commitIDs[0] = oldCommitID commitIDs[1] = newCommitID @@ -94,35 +49,15 @@ func getCommitIDsFromRepo(ctx context.Context, repo *repo_model.Repository, oldC return commitIDs, true, err } - newCommit, err := gitRepo.GetCommit(newCommitID) - if err != nil { - return nil, false, err - } - - commits, err := newCommit.CommitsBeforeUntil(oldCommitID) + // Find commits between new and old commit exclusing base branch commits + commits, err := gitRepo.CommitsBetweenSkipBase(newCommit, oldCommit, baseBranch) if err != nil { return nil, false, err } commitIDs = make([]string, 0, len(commits)) - commitChecks := make(map[string]*commitBranchCheckItem) - - for _, commit := range commits { - commitChecks[commit.ID.String()] = &commitBranchCheckItem{ - Commit: commit, - Checked: false, - } - } - - if err = commitBranchCheck(gitRepo, newCommit, oldCommitID, baseBranch, commitChecks); err != nil { - return - } - for i := len(commits) - 1; i >= 0; i-- { - commitID := commits[i].ID.String() - if item, ok := commitChecks[commitID]; ok && item.Checked { - commitIDs = append(commitIDs, commitID) - } + commitIDs = append(commitIDs, commits[i].ID.String()) } return commitIDs, isForcePush, err From aca89c95903c74f277770e0c38372b09afe0527a Mon Sep 17 00:00:00 2001 From: Brecht Van Lommel Date: Sat, 11 Feb 2023 10:01:59 +0100 Subject: [PATCH 2/4] Use AddOptionValues --- modules/git/repo_commit.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/modules/git/repo_commit.go b/modules/git/repo_commit.go index 32e9f560835e0..d322934ab91b8 100644 --- a/modules/git/repo_commit.go +++ b/modules/git/repo_commit.go @@ -330,13 +330,13 @@ func (repo *Repository) CommitsBetweenSkipBase(last, before *Commit, baseBranch var stdout []byte var err error if before == nil { - stdout, _, err = NewCommand(repo.Ctx, "rev-list").AddDynamicArguments(last.ID.String()).AddArguments("--not").AddDynamicArguments(baseBranch).RunStdBytes(&RunOpts{Dir: repo.Path}) + stdout, _, err = NewCommand(repo.Ctx, "rev-list").AddDynamicArguments(last.ID.String()).AddOptionValues("--not", baseBranch).RunStdBytes(&RunOpts{Dir: repo.Path}) } else { - stdout, _, err = NewCommand(repo.Ctx, "rev-list").AddDynamicArguments(before.ID.String() + ".." + last.ID.String()).AddArguments("--not").AddDynamicArguments(baseBranch).RunStdBytes(&RunOpts{Dir: repo.Path}) + stdout, _, err = NewCommand(repo.Ctx, "rev-list").AddDynamicArguments(before.ID.String()+".."+last.ID.String()).AddOptionValues("--not", baseBranch).RunStdBytes(&RunOpts{Dir: repo.Path}) if err != nil && strings.Contains(err.Error(), "no merge base") { // future versions of git >= 2.28 are likely to return an error if before and last have become unrelated. // previously it would return the results of git rev-list before last so let's try that... - stdout, _, err = NewCommand(repo.Ctx, "rev-list").AddDynamicArguments(before.ID.String(), last.ID.String()).AddArguments("--not").AddDynamicArguments(baseBranch).RunStdBytes(&RunOpts{Dir: repo.Path}) + stdout, _, err = NewCommand(repo.Ctx, "rev-list").AddDynamicArguments(before.ID.String(), last.ID.String()).AddOptionValues("--not", baseBranch).RunStdBytes(&RunOpts{Dir: repo.Path}) } } if err != nil { From 39f3ab3e4fc909698acacc14fdbd453871cb2ba4 Mon Sep 17 00:00:00 2001 From: Brecht Van Lommel Date: Thu, 9 Mar 2023 00:48:47 +0100 Subject: [PATCH 3/4] Deduplicate force push detection code The IsBranch() test is no longer part of IsForcePush(), because the code path this is called in already tests opts.IsBranch() and forced updates are not just a thing in branches. --- modules/git/commit.go | 13 +++++++++++++ modules/repository/push.go | 18 ------------------ services/pull/comment.go | 7 +++---- services/repository/push.go | 6 +++--- 4 files changed, 19 insertions(+), 25 deletions(-) diff --git a/modules/git/commit.go b/modules/git/commit.go index 4a55645d30346..6e8fcb3e0802e 100644 --- a/modules/git/commit.go +++ b/modules/git/commit.go @@ -218,6 +218,19 @@ func (c *Commit) HasPreviousCommit(commitHash SHA1) (bool, error) { return false, err } +// IsForcePush returns true if a push from oldCommitHash to this is a force push +func (c *Commit) IsForcePush(oldCommitID string) (bool, error) { + if oldCommitID == EmptySHA { + return false, nil + } + oldCommit, err := c.repo.GetCommit(oldCommitID) + if err != nil { + return false, err + } + hasPreviousCommit, err := c.HasPreviousCommit(oldCommit.ID) + return !hasPreviousCommit, err +} + // CommitsBeforeLimit returns num commits before current revision func (c *Commit) CommitsBeforeLimit(num int) ([]*Commit, error) { return c.repo.getCommitsBeforeLimit(c.ID, num) diff --git a/modules/repository/push.go b/modules/repository/push.go index 1fa711b359c34..aa1552351d515 100644 --- a/modules/repository/push.go +++ b/modules/repository/push.go @@ -4,10 +4,8 @@ package repository import ( - "context" "strings" - repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/modules/git" ) @@ -96,19 +94,3 @@ func (opts *PushUpdateOptions) RefName() string { func (opts *PushUpdateOptions) RepoFullName() string { return opts.RepoUserName + "/" + opts.RepoName } - -// IsForcePush detect if a push is a force push -func IsForcePush(ctx context.Context, opts *PushUpdateOptions) (bool, error) { - if !opts.IsUpdateBranch() { - return false, nil - } - - output, _, err := git.NewCommand(ctx, "rev-list", "--max-count=1").AddDynamicArguments(opts.OldCommitID, "^"+opts.NewCommitID). - RunStdString(&git.RunOpts{Dir: repo_model.RepoPath(opts.RepoUserName, opts.RepoName)}) - if err != nil { - return false, err - } else if len(output) > 0 { - return true, nil - } - return false, nil -} diff --git a/services/pull/comment.go b/services/pull/comment.go index eb2e7071b7c4b..933ad09a85e9a 100644 --- a/services/pull/comment.go +++ b/services/pull/comment.go @@ -35,18 +35,17 @@ func getCommitIDsFromRepo(ctx context.Context, repo *repo_model.Repository, oldC return nil, false, err } - // If old commit is not an ancestor of new commits, it's a force push - isAncestor, err := newCommit.HasPreviousCommit(oldCommit.ID) + isForcePush, err = newCommit.IsForcePush(oldCommitID) if err != nil { return nil, false, err } - if !isAncestor { + if isForcePush { commitIDs = make([]string, 2) commitIDs[0] = oldCommitID commitIDs[1] = newCommitID - return commitIDs, true, err + return commitIDs, isForcePush, err } // Find commits between new and old commit exclusing base branch commits diff --git a/services/repository/push.go b/services/repository/push.go index 355c2878113fd..156e3ff791811 100644 --- a/services/repository/push.go +++ b/services/repository/push.go @@ -207,12 +207,12 @@ func pushUpdates(optsList []*repo_module.PushUpdateOptions) error { return fmt.Errorf("newCommit.CommitsBeforeUntil: %w", err) } - isForce, err := repo_module.IsForcePush(ctx, opts) + isForcePush, err := newCommit.IsForcePush(opts.OldCommitID) if err != nil { - log.Error("isForcePush %s:%s failed: %v", repo.FullName(), branch, err) + log.Error("IsForcePush %s:%s failed: %v", repo.FullName(), branch, err) } - if isForce { + if isForcePush { log.Trace("Push %s is a force push", opts.NewCommitID) cache.Remove(repo.GetCommitsCountCacheKey(opts.RefName(), true)) From 9c59a073b8eee4fba965d178c608339f236cd3ff Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 9 Mar 2023 10:58:11 +0800 Subject: [PATCH 4/4] Update modules/git/repo_commit.go --- modules/git/repo_commit.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/git/repo_commit.go b/modules/git/repo_commit.go index 283c9a0202a32..0e1b00ce082e4 100644 --- a/modules/git/repo_commit.go +++ b/modules/git/repo_commit.go @@ -323,7 +323,7 @@ func (repo *Repository) CommitsBetweenLimit(last, before *Commit, limit, skip in return repo.parsePrettyFormatLogToList(bytes.TrimSpace(stdout)) } -// CommitsBetween returns a list that contains commits between [before, last), excluding commits in baseBranch. +// CommitsBetweenNotBase returns a list that contains commits between [before, last), excluding commits in baseBranch. // If before is detached (removed by reset + push) it is not included. func (repo *Repository) CommitsBetweenNotBase(last, before *Commit, baseBranch string) ([]*Commit, error) { var stdout []byte