Don't return binary file changes in raw PR diffs by default (#17158)

* return diffs without binary file content change

* ?binary=true option to restore old behaviour

Co-authored-by: techknowlogick <[email protected]>
Co-authored-by: zeripath <[email protected]>

Author: 3 people

modules/git/repo_compare.go

@@ -215,20 +215,29 @@ func parseDiffStat(stdout string) (numFiles, totalAdditions, totalDeletions int,
 }
 
 // GetDiffOrPatch generates either diff or formatted patch data between given revisions
-func (repo *Repository) GetDiffOrPatch(base, head string, w io.Writer, formatted bool) error {
-	if formatted {
+func (repo *Repository) GetDiffOrPatch(base, head string, w io.Writer, patch, binary bool) error {
+	if patch {
 		return repo.GetPatch(base, head, w)
 	}
+	if binary {
+		return repo.GetDiffBinary(base, head, w)
+	}
 	return repo.GetDiff(base, head, w)
 }
 
-// GetDiff generates and returns patch data between given revisions.
+// GetDiff generates and returns patch data between given revisions, optimized for human readability
 func (repo *Repository) GetDiff(base, head string, w io.Writer) error {
+	return NewCommand("diff", "-p", base, head).
+		RunInDirPipeline(repo.Path, w, nil)
+}
+
+// GetDiffBinary generates and returns patch data between given revisions, including binary diffs.
+func (repo *Repository) GetDiffBinary(base, head string, w io.Writer) error {
 	return NewCommand("diff", "-p", "--binary", base, head).
 		RunInDirPipeline(repo.Path, w, nil)
 }
 
-// GetPatch generates and returns format-patch data between given revisions.
+// GetPatch generates and returns format-patch data between given revisions, able to be used with `git apply`
 func (repo *Repository) GetPatch(base, head string, w io.Writer) error {
 	stderr := new(bytes.Buffer)
 	err := NewCommand("format-patch", "--binary", "--stdout", base+"..."+head).
@@ -246,8 +255,7 @@ func (repo *Repository) GetDiffFromMergeBase(base, head string, w io.Writer) err
 	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 repo.GetDiffBinary(base, head, w)
 	}
 	return err
 }

routers/api/v1/repo/pull.go

@@ -204,6 +204,10 @@ func DownloadPullDiffOrPatch(ctx *context.APIContext) {
 	//   type: string
 	//   enum: [diff, patch]
 	//   required: true
+	// - name: binary
+	//   in: query
+	//   description: whether to include binary file changes. if true, the diff is applicable with `git apply`
+	//   type: boolean
 	// responses:
 	//   "200":
 	//     "$ref": "#/responses/string"
@@ -225,7 +229,9 @@ func DownloadPullDiffOrPatch(ctx *context.APIContext) {
 		patch = true
 	}
 
-	if err := pull_service.DownloadDiffOrPatch(pr, ctx, patch); err != nil {
+	binary := ctx.FormBool("binary")
+
+	if err := pull_service.DownloadDiffOrPatch(pr, ctx, patch, binary); err != nil {
 		ctx.InternalServerError(err)
 		return
 	}

routers/web/repo/pull.go

@@ -1345,8 +1345,9 @@ func DownloadPullDiffOrPatch(ctx *context.Context, patch bool) {
 	}
 
 	pr := issue.PullRequest
+	binary := ctx.FormBool("binary")
 
-	if err := pull_service.DownloadDiffOrPatch(pr, ctx, patch); err != nil {
+	if err := pull_service.DownloadDiffOrPatch(pr, ctx, patch, binary); err != nil {
 		ctx.ServerError("DownloadDiffOrPatch", err)
 		return
 	}

services/pull/patch.go

@@ -22,7 +22,7 @@ import (
 )
 
 // DownloadDiffOrPatch will write the patch for the pr to the writer
-func DownloadDiffOrPatch(pr *models.PullRequest, w io.Writer, patch bool) error {
+func DownloadDiffOrPatch(pr *models.PullRequest, w io.Writer, patch, binary 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
@@ -33,7 +33,7 @@ func DownloadDiffOrPatch(pr *models.PullRequest, w io.Writer, patch bool) error
 		return fmt.Errorf("OpenRepository: %v", err)
 	}
 	defer gitRepo.Close()
-	if err := gitRepo.GetDiffOrPatch(pr.MergeBase, pr.GetGitRefName(), w, patch); err != nil {
+	if err := gitRepo.GetDiffOrPatch(pr.MergeBase, pr.GetGitRefName(), w, patch, binary); 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)
 	}
@@ -108,7 +108,7 @@ func checkConflicts(pr *models.PullRequest, gitRepo *git.Repository, tmpBasePath
 		_ = util.Remove(tmpPatchFile.Name())
 	}()
 
-	if err := gitRepo.GetDiff(pr.MergeBase, "tracking", tmpPatchFile); err != nil {
+	if err := gitRepo.GetDiffBinary(pr.MergeBase, "tracking", tmpPatchFile); err != nil {
 		tmpPatchFile.Close()
 		log.Error("Unable to get patch file from %s to %s in %s Error: %v", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err)
 		return false, fmt.Errorf("Unable to get patch file from %s to %s in %s Error: %v", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err)

templates/swagger/v1_json.tmpl

@@ -7400,6 +7400,12 @@
             "name": "diffType",
             "in": "path",
             "required": true
+          },
+          {
+            "type": "boolean",
+            "description": "whether to include binary file changes. if true, the diff is applicable with `git apply`",
+            "name": "binary",
+            "in": "query"
           }
         ],
         "responses": {