From c5b71c7c6a2a1a3735f9a0823d7a6061f166f9fc Mon Sep 17 00:00:00 2001 From: Brecht Van Lommel Date: Fri, 17 Feb 2023 21:33:18 +0100 Subject: [PATCH 1/2] Improve squash merge commit author and co-author with private emails When emails addresses are private, squash merges always use @noreply.localhost for the author of the squash commit. And the author is redundantly added as a co-author in the commit message. Also without private mails, the redundant co-author is possible when committing with a signature that's different than the user full name and primary email. Now try to find a commit by the same user in the list of commits, and prefer the signature from that over one constructed from the account settings. --- services/pull/merge.go | 43 +++++++++++++++++++++++++++++++++++++----- services/pull/pull.go | 12 ++++++++++-- 2 files changed, 48 insertions(+), 7 deletions(-) diff --git a/services/pull/merge.go b/services/pull/merge.go index 3ac67d91b72f1..63cdb09b12493 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -239,6 +239,38 @@ func Merge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.U return nil } +// Get commit author signature for squash commits +func getSquashAuthorSignature(ctx context.Context, pr *issues_model.PullRequest, repoBasePath, newCommit, oldCommit string) (*git.Signature, error) { + if err := pr.Issue.LoadPoster(ctx); err != nil { + return nil, fmt.Errorf("LoadPoster: %w", err) + } + + // Try to get an signature from the same user in one of the commits, as the + // poster email might be private or commits with a different signature than + // the primary email address in their account. + gitRepo, closer, err := git.RepositoryFromContextOrOpen(ctx, repoBasePath) + if err != nil { + return nil, fmt.Errorf("Unable to open repository: Error %v", err) + } + defer closer.Close() + + commits, err := gitRepo.CommitsBetweenIDs(newCommit, oldCommit) + if err != nil { + return nil, fmt.Errorf("Unable to get commits between: %s %s Error %v", oldCommit, newCommit, err) + } + + for _, commit := range commits { + if commit.Author != nil { + commitUser, _ := user_model.GetUserByEmail(ctx, commit.Author.Email) + if commitUser != nil && commitUser.ID == pr.Issue.Poster.ID { + return commit.Author, nil + } + } + } + + return pr.Issue.Poster.NewGitSig(), nil +} + // rawMerge perform the merge operation without changing any pull information in database func rawMerge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.User, mergeStyle repo_model.MergeStyle, expectedHeadCommitID, message string) (string, error) { // Clone base repo. @@ -514,6 +546,12 @@ func rawMerge(ctx context.Context, pr *issues_model.PullRequest, doer *user_mode } } case repo_model.MergeStyleSquash: + sig, err := getSquashAuthorSignature(ctx, pr, tmpBasePath, trackingBranch, "HEAD") + if err != nil { + log.Error("getSquashAuthorSignature: %v", err) + return "", err + } + // Merge with squash cmd := git.NewCommand(ctx, "merge", "--squash").AddDynamicArguments(trackingBranch) if err := runMergeCommand(pr, mergeStyle, cmd, tmpBasePath); err != nil { @@ -521,11 +559,6 @@ func rawMerge(ctx context.Context, pr *issues_model.PullRequest, doer *user_mode return "", err } - if err = pr.Issue.LoadPoster(ctx); err != nil { - log.Error("LoadPoster: %v", err) - return "", fmt.Errorf("LoadPoster: %w", err) - } - sig := pr.Issue.Poster.NewGitSig() if setting.Repository.PullRequest.AddCoCommitterTrailers && committer.String() != sig.String() { // add trailer message += fmt.Sprintf("\nCo-authored-by: %s\nCo-committed-by: %s\n", sig.String(), sig.String()) diff --git a/services/pull/pull.go b/services/pull/pull.go index 0d260c93b1ec3..ae36ed4890228 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -673,7 +673,12 @@ func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequ authorString := commit.Author.String() if uniqueAuthors.Add(authorString) && authorString != posterSig { - authors = append(authors, authorString) + // Compare use account as well to avoid adding the same author multiple times + // times when email addresses are private or multiple emails are used. + commitUser, _ := user_model.GetUserByEmail(ctx, commit.Author.Email) + if commitUser == nil || commitUser.ID != pr.Issue.Poster.ID { + authors = append(authors, authorString) + } } } @@ -694,7 +699,10 @@ func GetSquashMergeCommitMessages(ctx context.Context, pr *issues_model.PullRequ for _, commit := range commits { authorString := commit.Author.String() if uniqueAuthors.Add(authorString) && authorString != posterSig { - authors = append(authors, authorString) + commitUser, _ := user_model.GetUserByEmail(ctx, commit.Author.Email) + if commitUser == nil || commitUser.ID != pr.Issue.Poster.ID { + authors = append(authors, authorString) + } } } skip += limit From 870efbe7cb77e69c1cd5834b8dea7d91935a2ab6 Mon Sep 17 00:00:00 2001 From: Brecht Van Lommel Date: Thu, 9 Mar 2023 11:39:23 +0100 Subject: [PATCH 2/2] Test same email only once --- services/pull/merge_squash.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/services/pull/merge_squash.go b/services/pull/merge_squash.go index e3d7d64641e70..3306a1aa7a458 100644 --- a/services/pull/merge_squash.go +++ b/services/pull/merge_squash.go @@ -8,6 +8,7 @@ import ( repo_model "code.gitea.io/gitea/models/repo" user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/container" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" @@ -33,8 +34,9 @@ func getAuthorSignatureSquash(ctx *mergeContext) (*git.Signature, error) { return nil, fmt.Errorf("Unable to get commits between: %s %s Error %v", "HEAD", trackingBranch, err) } + uniqueEmails := make(container.Set[string]) for _, commit := range commits { - if commit.Author != nil { + if commit.Author != nil && uniqueEmails.Add(commit.Author.Email) { commitUser, _ := user_model.GetUserByEmail(ctx, commit.Author.Email) if commitUser != nil && commitUser.ID == ctx.pr.Issue.Poster.ID { return commit.Author, nil