[BugFix] Use MergedCommitID for merged pulls to create Diff/Patch File

[6543]

fix #10932

and also fix "Empty Diff/Patch File when pull is merged"

[codecov-io]

Codecov Report

Merging #10934 into master will decrease coverage by 0.06%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master   #10934      +/-   ##
==========================================
- Coverage   43.50%   43.43%   -0.07%     
==========================================
  Files         597      597              
  Lines       83916    83930      +14     
==========================================
- Hits        36504    36454      -50     
- Misses      42903    42973      +70     
+ Partials     4509     4503       -6     

Impacted Files	Coverage Δ
routers/repo/pull.go	27.82% <0.00%> (-0.40%)	⬇️
services/pull/check.go	32.92% <0.00%> (-20.74%)	⬇️
modules/git/repo_language_stats.go	66.17% <0.00%> (-4.42%)	⬇️
modules/git/command.go	86.95% <0.00%> (-2.61%)	⬇️
services/pull/patch.go	61.93% <0.00%> (-2.59%)	⬇️
services/pull/temp_repo.go	29.05% <0.00%> (-2.57%)	⬇️
models/unit.go	41.97% <0.00%> (-2.47%)	⬇️
modules/queue/workerpool.go	56.93% <0.00%> (-1.07%)	⬇️
modules/log/event.go	64.61% <0.00%> (-1.03%)	⬇️
models/notification.go	61.68% <0.00%> (-0.79%)	⬇️
... and 6 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3723b06...adb549f. Read the comment docs.

[6543]

ToDo: add a test

[zeripath]

So actually I wonder if we should just simplify services/pull/patch.go:23:func DownloadDiffOrPatch(...) to:

// DownloadDiffOrPatch will write the patch for the pr to the writer
func DownloadDiffOrPatch(pr *models.PullRequest, w io.Writer, patch bool) error {
	if err := pr.LoadBaseRepo(); err != nil {
		log.Error("Unable to load base repository ID %d for pr #%d [%d]", pr.BaseRepoID, pr.Index, pr.ID)
		return err
	}

	gitRepo, err := git.OpenRepository(pr.BaseRepo.RepoPath())
	if err != nil {
		return fmt.Errorf("OpenRepository: %v", err)
	}
	defer gitRepo.Close()
	if err := gitRepo.GetDiffOrPatch(pr.MergeBase, pr.GetGitRefName(), w, patch); err != nil {
		log.Error("Unable to get patch file from %s to %s in %s Error: %v", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err)
		return fmt.Errorf("Unable to get patch file from %s to %s in %s Error: %v", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err)
	}
	return nil
}

E.g. the patch is:

diff --git a/services/pull/patch.go b/services/pull/patch.go
index 96145fcd9..776bfdb75 100644
--- a/services/pull/patch.go
+++ b/services/pull/patch.go
@@ -21,30 +21,17 @@ import (
 
 // DownloadDiffOrPatch will write the patch for the pr to the writer
 func DownloadDiffOrPatch(pr *models.PullRequest, w io.Writer, patch bool) error {
-       // Clone base repo.
-       tmpBasePath, err := createTemporaryRepo(pr)
-       if err != nil {
-               log.Error("CreateTemporaryPath: %v", err)
+       if err := pr.LoadBaseRepo(); err != nil {
+               log.Error("Unable to load base repository ID %d for pr #%d [%d]", pr.BaseRepoID, pr.Index
                return err
        }
-       defer func() {
-               if err := models.RemoveTemporaryPath(tmpBasePath); err != nil {
-                       log.Error("DownloadDiff: RemoveTemporaryPath: %s", err)
-               }
-       }()
 
-       gitRepo, err := git.OpenRepository(tmpBasePath)
+       gitRepo, err := git.OpenRepository(pr.BaseRepo.RepoPath())
        if err != nil {
                return fmt.Errorf("OpenRepository: %v", err)
        }
        defer gitRepo.Close()
-
-       pr.MergeBase, err = git.NewCommand("merge-base", "--", "base", "tracking").RunInDir(tmpBasePath)
-       if err != nil {
-               pr.MergeBase = "base"
-       }
-       pr.MergeBase = strings.TrimSpace(pr.MergeBase)
-       if err := gitRepo.GetDiffOrPatch(pr.MergeBase, "tracking", w, patch); err != nil {
+       if err := gitRepo.GetDiffOrPatch(pr.MergeBase, pr.GetGitRefName(), w, patch); err != nil {
                log.Error("Unable to get patch file from %s to %s in %s Error: %v", pr.MergeBase, pr.Head
                return fmt.Errorf("Unable to get patch file from %s to %s in %s Error: %v", pr.MergeBase,
        }

[zeripath]

Otherwise, if that's too much - i.e. we use DownloadDiffOrPatch before the head is updated, we should make DownloadDiffOrPatch :

// DownloadDiffOrPatch will write the patch for the pr to the writer
func DownloadDiffOrPatch(pr *models.PullRequest, w io.Writer, patch bool) error {
	if pr.HasMerged {
		if err := pr.LoadBaseRepo(); err != nil {
			log.Error("Unable to load base repository ID %d for pr #%d [%d]", pr.BaseRepoID, pr.Index, pr.ID)
			return err
		}

		gitRepo, err := git.OpenRepository(pr.BaseRepo.RepoPath())
		if err != nil {
			return fmt.Errorf("OpenRepository: %v", err)
		}
		defer gitRepo.Close()
		if err := gitRepo.GetDiffOrPatch(pr.MergeBase, pr.GetGitRefName(), w, patch); err != nil {
			log.Error("Unable to get patch file from %s to %s in %s Error: %v", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err)
			return fmt.Errorf("Unable to get patch file from %s to %s in %s Error: %v", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err)
		}
		return nil
	}

	// Clone base repo.
	tmpBasePath, err := createTemporaryRepo(pr)
	if err != nil {
		log.Error("CreateTemporaryPath: %v", err)
		return err
	}
	defer func() {
		if err := models.RemoveTemporaryPath(tmpBasePath); err != nil {
			log.Error("DownloadDiff: RemoveTemporaryPath: %s", err)
		}
	}()

	gitRepo, err := git.OpenRepository(tmpBasePath)
	if err != nil {
		return fmt.Errorf("OpenRepository: %v", err)
	}
	defer gitRepo.Close()

	pr.MergeBase, err = git.NewCommand("merge-base", "--", "base", "tracking").RunInDir(tmpBasePath)
	if err != nil {
		pr.MergeBase = "base"
	}
	pr.MergeBase = strings.TrimSpace(pr.MergeBase)
	if err := gitRepo.GetDiffOrPatch(pr.MergeBase, "tracking", w, patch); err != nil {
		log.Error("Unable to get patch file from %s to %s in %s Error: %v", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err)
		return fmt.Errorf("Unable to get patch file from %s to %s in %s Error: %v", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err)
	}
	return nil
}

Then you can drop your changes in routers/repo/pull.go.

[zeripath]

As we don't use DownloadDiffOrPatch elsewhere - I've uploaded a different PR that should solve the issue more cleanly.

[6543]

close in faifor of #10936