Git 2.28 no longer permits diff with ... on unrelated branches (#12370)

Backport #12364

Signed-off-by: Andrew Thornton <art27@cantab.net>

4 files changed, 54 insertions, 11 deletions

diff --git a/modules/git/commit.go b/modules/git/commit.go
index 5e492e27ea..5ba8c2c8af 100644
--- a/modules/git/commit.go
+++ b/modules/git/commit.go
@@ -271,11 +271,12 @@ func AllCommitsCount(repoPath string) (int64, error) {
 	return strconv.ParseInt(strings.TrimSpace(stdout), 10, 64)
 }
 
-func commitsCount(repoPath, revision, relpath string) (int64, error) {
+func commitsCount(repoPath string, revision, relpath []string) (int64, error) {
 	cmd := NewCommand("rev-list", "--count")
-	cmd.AddArguments(revision)
+	cmd.AddArguments(revision...)
 	if len(relpath) > 0 {
-		cmd.AddArguments("--", relpath)
+		cmd.AddArguments("--")
+		cmd.AddArguments(relpath...)
 	}
 
 	stdout, err := cmd.RunInDir(repoPath)
@@ -288,7 +289,7 @@ func commitsCount(repoPath, revision, relpath string) (int64, error) {
 
 // CommitsCount returns number of total commits of until given revision.
 func CommitsCount(repoPath, revision string) (int64, error) {
-	return commitsCount(repoPath, revision, "")
+	return commitsCount(repoPath, []string{revision}, []string{})
 }
 
 // CommitsCount returns number of total commits of until current revision.
diff --git a/modules/git/repo_commit.go b/modules/git/repo_commit.go
index 18c732c6f2..bab3d8852c 100644
--- a/modules/git/repo_commit.go
+++ b/modules/git/repo_commit.go
@@ -293,7 +293,7 @@ func (repo *Repository) FileChangedBetweenCommits(filename, id1, id2 string) (bo
 
 // FileCommitsCount return the number of files at a revison
 func (repo *Repository) FileCommitsCount(revision, file string) (int64, error) {
-	return commitsCount(repo.Path, revision, file)
+	return commitsCount(repo.Path, []string{revision}, []string{file})
 }
 
 // CommitsByFileAndRange return the commits according revison file and the page
@@ -319,6 +319,11 @@ func (repo *Repository) CommitsByFileAndRangeNoFollow(revision, file string, pag
 // FilesCountBetween return the number of files changed between two commits
 func (repo *Repository) FilesCountBetween(startCommitID, endCommitID string) (int, error) {
 	stdout, err := NewCommand("diff", "--name-only", startCommitID+"..."+endCommitID).RunInDir(repo.Path)
+	if err != nil && strings.Contains(err.Error(), "no merge base") {
+		// git >= 2.28 now returns an error if startCommitID and endCommitID have become unrelated.
+		// previously it would return the results of git diff --name-only startCommitID endCommitID so let's try that...
+		stdout, err = NewCommand("diff", "--name-only", startCommitID, endCommitID).RunInDir(repo.Path)
+	}
 	if err != nil {
 		return 0, err
 	}
@@ -333,6 +338,11 @@ func (repo *Repository) CommitsBetween(last *Commit, before *Commit) (*list.List
 		stdout, err = NewCommand("rev-list", last.ID.String()).RunInDirBytes(repo.Path)
 	} else {
 		stdout, err = NewCommand("rev-list", before.ID.String()+"..."+last.ID.String()).RunInDirBytes(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("rev-list", before.ID.String(), last.ID.String()).RunInDirBytes(repo.Path)
+		}
 	}
 	if err != nil {
 		return nil, err
@@ -348,6 +358,11 @@ func (repo *Repository) CommitsBetweenLimit(last *Commit, before *Commit, limit,
 		stdout, err = NewCommand("rev-list", "--max-count", strconv.Itoa(limit), "--skip", strconv.Itoa(skip), last.ID.String()).RunInDirBytes(repo.Path)
 	} else {
 		stdout, err = NewCommand("rev-list", "--max-count", strconv.Itoa(limit), "--skip", strconv.Itoa(skip), before.ID.String()+"..."+last.ID.String()).RunInDirBytes(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 --max-count n before last so let's try that...
+			stdout, err = NewCommand("rev-list", "--max-count", strconv.Itoa(limit), "--skip", strconv.Itoa(skip), before.ID.String(), last.ID.String()).RunInDirBytes(repo.Path)
+		}
 	}
 	if err != nil {
 		return nil, err
@@ -373,7 +388,14 @@ func (repo *Repository) CommitsBetweenIDs(last, before string) (*list.List, erro
 
 // CommitsCountBetween return numbers of commits between two commits
 func (repo *Repository) CommitsCountBetween(start, end string) (int64, error) {
-	return commitsCount(repo.Path, start+"..."+end, "")
+	count, err := commitsCount(repo.Path, []string{start + "..." + end}, []string{})
+	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...
+		return commitsCount(repo.Path, []string{start, end}, []string{})
+	}
+
+	return count, err
 }
 
 // commitsBefore the limit is depth, not total number of returned commits.
diff --git a/modules/git/repo_compare.go b/modules/git/repo_compare.go
index 5bc7f9ca5a..16ea068db2 100644
--- a/modules/git/repo_compare.go
+++ b/modules/git/repo_compare.go
@@ -6,6 +6,7 @@
 package git
 
 import (
+	"bytes"
 	"container/list"
 	"fmt"
 	"io"
@@ -66,7 +67,7 @@ func (repo *Repository) GetCompareInfo(basePath, baseBranch, headBranch string)
 	compareInfo := new(CompareInfo)
 	compareInfo.MergeBase, remoteBranch, err = repo.GetMergeBase(tmpRemote, baseBranch, headBranch)
 	if err == nil {
-		// We have a common base
+		// We have a common base - therefore we know that ... should work
 		logs, err := NewCommand("log", compareInfo.MergeBase+"..."+headBranch, prettyLogFormat).RunInDirBytes(repo.Path)
 		if err != nil {
 			return nil, err
@@ -85,6 +86,11 @@ func (repo *Repository) GetCompareInfo(basePath, baseBranch, headBranch string)
 
 	// Count number of changed files.
 	stdout, err := NewCommand("diff", "--name-only", remoteBranch+"..."+headBranch).RunInDir(repo.Path)
+	if err != nil && strings.Contains(err.Error(), "no merge base") {
+		// git >= 2.28 now returns an error if base and head have become unrelated.
+		// previously it would return the results of git diff --name-only base head so let's try that...
+		stdout, err = NewCommand("diff", "--name-only", remoteBranch, headBranch).RunInDir(repo.Path)
+	}
 	if err != nil {
 		return nil, err
 	}
@@ -108,12 +114,24 @@ func (repo *Repository) GetDiff(base, head string, w io.Writer) error {
 
 // GetPatch generates and returns format-patch data between given revisions.
 func (repo *Repository) GetPatch(base, head string, w io.Writer) error {
-	return NewCommand("format-patch", "--binary", "--stdout", base+"..."+head).
-		RunInDirPipeline(repo.Path, w, nil)
+	stderr := new(bytes.Buffer)
+	err := NewCommand("format-patch", "--binary", "--stdout", base+"..."+head).
+		RunInDirPipeline(repo.Path, w, stderr)
+	if err != nil && bytes.Contains(stderr.Bytes(), []byte("no merge base")) {
+		return NewCommand("format-patch", "--binary", "--stdout", base, head).
+			RunInDirPipeline(repo.Path, w, nil)
+	}
+	return err
 }
 
 // GetDiffFromMergeBase generates and return patch data from merge base to head
 func (repo *Repository) GetDiffFromMergeBase(base, head string, w io.Writer) error {
-	return NewCommand("diff", "-p", "--binary", base+"..."+head).
-		RunInDirPipeline(repo.Path, w, nil)
+	stderr := new(bytes.Buffer)
+	err := NewCommand("diff", "-p", "--binary", base+"..."+head).
+		RunInDirPipeline(repo.Path, w, stderr)
+	if err != nil && bytes.Contains(stderr.Bytes(), []byte("no merge base")) {
+		return NewCommand("diff", "-p", "--binary", base, head).
+			RunInDirPipeline(repo.Path, w, nil)
+	}
+	return err
 }
diff --git a/routers/private/hook.go b/routers/private/hook.go
index 4b57aff588..215793c970 100644
--- a/routers/private/hook.go
+++ b/routers/private/hook.go
@@ -39,6 +39,7 @@ func verifyCommits(oldCommitID, newCommitID string, repo *git.Repository, env []
 		_ = stdoutWriter.Close()
 	}()
 
+	// This is safe as force pushes are already forbidden
 	err = git.NewCommand("rev-list", oldCommitID+"..."+newCommitID).
 		RunInDirTimeoutEnvFullPipelineFunc(env, -1, repo.Path,
 			stdoutWriter, nil, nil,
@@ -70,6 +71,7 @@ func checkFileProtection(oldCommitID, newCommitID string, patterns []glob.Glob,
 		_ = stdoutWriter.Close()
 	}()
 
+	// This use of ...  is safe as force-pushes have already been ruled out.
 	err = git.NewCommand("diff", "--name-only", oldCommitID+"..."+newCommitID).
 		RunInDirTimeoutEnvFullPipelineFunc(env, -1, repo.Path,
 			stdoutWriter, nil, nil,