Fix the wrong push commits in the pull request when force push

models/issues/comment.go

@@ -7,6 +7,7 @@ package issues
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"html/template"
 	"slices"
@@ -21,6 +22,7 @@ import (
 	user_model "code.gitea.io/gitea/models/user"
 	"code.gitea.io/gitea/modules/container"
 	"code.gitea.io/gitea/modules/htmlutil"
+	"code.gitea.io/gitea/modules/json"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/optional"
 	"code.gitea.io/gitea/modules/references"
@@ -326,21 +328,34 @@ type Comment struct {
 	RefIssue   *Issue                 `xorm:"-"`
 	RefComment *Comment               `xorm:"-"`
 
-	Commits     []*git_model.SignCommitWithStatuses `xorm:"-"`
-	OldCommit   string                              `xorm:"-"`
-	NewCommit   string                              `xorm:"-"`
-	CommitsNum  int64                               `xorm:"-"`
-	IsForcePush bool                                `xorm:"-"`
+	Commits    []*git_model.SignCommitWithStatuses `xorm:"-"`
+	OldCommit  string                              `xorm:"-"`
+	NewCommit  string                              `xorm:"-"`
+	CommitsNum int64                               `xorm:"-"`
+
+	// Templates still use it. It is not persisted in database, it is only set when creating or loading
+	IsForcePush bool `xorm:"-"`
 }
 
 func init() {
 	db.RegisterModel(new(Comment))
 }
 
-// PushActionContent is content of push pull comment
+// PushActionContent is content of pull request's push comment
 type PushActionContent struct {
-	IsForcePush bool     `json:"is_force_push"`
-	CommitIDs   []string `json:"commit_ids"`
+	IsForcePush bool `json:"is_force_push"`
+	// if IsForcePush=true, CommitIDs contains the commit pair [old head, new head]
+	// if IsForcePush=false, CommitIDs contains the new commits newly pushed to the head branch
+	CommitIDs []string `json:"commit_ids"`
+}
+
+func (c *Comment) GetPushActionContent() (*PushActionContent, error) {
+	if c.Type != CommentTypePullRequestPush {
+		return nil, errors.New("not a pull request push comment")
+	}
+	var data PushActionContent
+	_ = json.Unmarshal(util.UnsafeStringToBytes(c.Content), &data)
+	return &data, nil
 }
 
 // LoadIssue loads the issue reference for the comment

modules/git/object_id_test.go

@@ -22,4 +22,6 @@ func TestIsValidSHAPattern(t *testing.T) {
 	assert.Equal(t, "2e65efe2a145dda7ee51d1741299f848e5bf752e", ComputeBlobHash(Sha1ObjectFormat, []byte("a")).String())
 	assert.Equal(t, "473a0f4c3be8a93681a267e3b1e9a7dcda1185436fe141f7749120a303721813", ComputeBlobHash(Sha256ObjectFormat, nil).String())
 	assert.Equal(t, "eb337bcee2061c5313c9a1392116b6c76039e9e30d71467ae359b36277e17dc7", ComputeBlobHash(Sha256ObjectFormat, []byte("a")).String())
+	assert.True(t, IsEmptyCommitID(""))
+	assert.True(t, IsEmptyCommitID("0000000000000000000000000000000000000000"))
 }

modules/git/repo_commit.go

@@ -362,39 +362,6 @@ func (repo *Repository) CommitsBetweenLimit(last, before *Commit, limit, skip in
 	return repo.parsePrettyFormatLogToList(bytes.TrimSpace(stdout))
 }
 
-// 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
-	var err error
-	if before == nil {
-		stdout, _, err = gitcmd.NewCommand("rev-list").
-			AddDynamicArguments(last.ID.String()).
-			AddOptionValues("--not", baseBranch).
-			WithDir(repo.Path).
-			RunStdBytes(repo.Ctx)
-	} else {
-		stdout, _, err = gitcmd.NewCommand("rev-list").
-			AddDynamicArguments(before.ID.String()+".."+last.ID.String()).
-			AddOptionValues("--not", baseBranch).
-			WithDir(repo.Path).
-			RunStdBytes(repo.Ctx)
-		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 = gitcmd.NewCommand("rev-list").
-				AddDynamicArguments(before.ID.String(), last.ID.String()).
-				AddOptionValues("--not", baseBranch).
-				WithDir(repo.Path).
-				RunStdBytes(repo.Ctx)
-		}
-	}
-	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)

modules/gitrepo/compare.go

@@ -42,3 +42,30 @@ func GetDivergingCommits(ctx context.Context, repo Repository, baseBranch, targe
 	}
 	return &DivergeObject{Ahead: ahead, Behind: behind}, nil
 }
+
+// GetCommitIDsBetweenReverse returns the last commit IDs between two commits in reverse order (from old to new) with limit.
+// If the result exceeds the limit, the old commits IDs will be ignored
+func GetCommitIDsBetweenReverse(ctx context.Context, repo Repository, startRef, endRef, notRef string, limit int) ([]string, error) {
+	genCmd := func(reversions ...string) *gitcmd.Command {
+		cmd := gitcmd.NewCommand("rev-list", "--reverse").
+			AddArguments("-n").AddDynamicArguments(strconv.Itoa(limit)).
+			AddDynamicArguments(reversions...)
+		if notRef != "" { // --not should be kept as the last parameter of git command, otherwise the result will be wrong
+			cmd.AddOptionValues("--not", notRef)
+		}
+		return cmd
+	}
+	stdout, _, err := RunCmdString(ctx, repo, genCmd(startRef+".."+endRef))
+	// example git error message: fatal: origin/main..HEAD: no merge base
+	if err != nil && strings.Contains(err.Stderr(), "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 = RunCmdString(ctx, repo, genCmd(startRef, endRef))
+	}
+	if err != nil {
+		return nil, err
+	}
+
+	commitIDs := strings.Fields(strings.TrimSpace(stdout))
+	return commitIDs, nil
+}

modules/gitrepo/compare_test.go

@@ -40,3 +40,75 @@ func TestRepoGetDivergingCommits(t *testing.T) {
 		Behind: 2,
 	}, do)
 }
+
+func TestGetCommitIDsBetweenReverse(t *testing.T) {
+	repo := &mockRepository{path: "repo1_bare"}
+
+	// tests raw commit IDs
+	commitIDs, err := GetCommitIDsBetweenReverse(t.Context(), repo,
+		"8d92fc957a4d7cfd98bc375f0b7bb189a0d6c9f2",
+		"ce064814f4a0d337b333e646ece456cd39fab612",
+		"",
+		100,
+	)
+	assert.NoError(t, err)
+	assert.Equal(t, []string{
+		"8006ff9adbf0cb94da7dad9e537e53817f9fa5c0",
+		"6fbd69e9823458e6c4a2fc5c0f6bc022b2f2acd1",
+		"37991dec2c8e592043f47155ce4808d4580f9123",
+		"feaf4ba6bc635fec442f46ddd4512416ec43c2c2",
+		"ce064814f4a0d337b333e646ece456cd39fab612",
+	}, commitIDs)
+
+	commitIDs, err = GetCommitIDsBetweenReverse(t.Context(), repo,
+		"8d92fc957a4d7cfd98bc375f0b7bb189a0d6c9f2",
+		"ce064814f4a0d337b333e646ece456cd39fab612",
+		"6fbd69e9823458e6c4a2fc5c0f6bc022b2f2acd1",
+		100,
+	)
+	assert.NoError(t, err)
+	assert.Equal(t, []string{
+		"37991dec2c8e592043f47155ce4808d4580f9123",
+		"feaf4ba6bc635fec442f46ddd4512416ec43c2c2",
+		"ce064814f4a0d337b333e646ece456cd39fab612",
+	}, commitIDs)
+
+	commitIDs, err = GetCommitIDsBetweenReverse(t.Context(), repo,
+		"8d92fc957a4d7cfd98bc375f0b7bb189a0d6c9f2",
+		"ce064814f4a0d337b333e646ece456cd39fab612",
+		"",
+		3,
+	)
+	assert.NoError(t, err)
+	assert.Equal(t, []string{
+		"37991dec2c8e592043f47155ce4808d4580f9123",
+		"feaf4ba6bc635fec442f46ddd4512416ec43c2c2",
+		"ce064814f4a0d337b333e646ece456cd39fab612",
+	}, commitIDs)
+
+	// test branch names instead of raw commit IDs.
+	commitIDs, err = GetCommitIDsBetweenReverse(t.Context(), repo,
+		"test",
+		"master",
+		"",
+		100,
+	)
+	assert.NoError(t, err)
+	assert.Equal(t, []string{
+		"feaf4ba6bc635fec442f46ddd4512416ec43c2c2",
+		"ce064814f4a0d337b333e646ece456cd39fab612",
+	}, commitIDs)
+
+	// add notref to exclude test
+	commitIDs, err = GetCommitIDsBetweenReverse(t.Context(), repo,
+		"test",
+		"master",
+		"test",
+		100,
+	)
+	assert.NoError(t, err)
+	assert.Equal(t, []string{
+		"feaf4ba6bc635fec442f46ddd4512416ec43c2c2",
+		"ce064814f4a0d337b333e646ece456cd39fab612",
+	}, commitIDs)
+}

services/agit/agit.go

@@ -282,12 +282,15 @@ func ProcReceive(ctx context.Context, repo *repo_model.Repository, gitRepo *git.
 		if err != nil {
 			return nil, fmt.Errorf("failed to load pull issue. Error: %w", err)
 		}
-		comment, err := pull_service.CreatePushPullComment(ctx, pusher, pr, oldCommitID, opts.NewCommitIDs[i], forcePush.Value())
-		if err == nil && comment != nil {
+
+		isForcePush := forcePush.Value()
+		comment, commentCreated, err := pull_service.CreatePushPullComment(ctx, pusher, pr, oldCommitID, opts.NewCommitIDs[i], isForcePush)
+		if err != nil {
+			log.Error("CreatePushPullComment: %v", err)
+		} else if commentCreated {
 			notify_service.PullRequestPushCommits(ctx, pusher, pr, comment)
 		}
 		notify_service.PullRequestSynchronized(ctx, pusher, pr)
-		isForcePush := comment != nil && comment.IsForcePush
 
 		results = append(results, private.HookProcReceiveRefResult{
 			OldOID:            oldCommitID,