From f48dce3176649916c262ab080d5f38a0433f38c0 Mon Sep 17 00:00:00 2001 From: Norwin Date: Mon, 27 Sep 2021 23:09:49 +0200 Subject: [PATCH] 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 Co-authored-by: zeripath --- modules/git/repo_compare.go | 20 ++++++++++++++------ routers/api/v1/repo/pull.go | 8 +++++++- routers/web/repo/pull.go | 3 ++- services/pull/patch.go | 6 +++--- templates/swagger/v1_json.tmpl | 6 ++++++ 5 files changed, 32 insertions(+), 11 deletions(-) diff --git a/modules/git/repo_compare.go b/modules/git/repo_compare.go index 866b3d9133..50e9005511 100644 --- a/modules/git/repo_compare.go +++ b/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 } diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 1c28363e8a..3974069ab4 100644 --- a/routers/api/v1/repo/pull.go +++ b/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 } diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index e6d67f3fcc..1b15ea9a79 100644 --- a/routers/web/repo/pull.go +++ b/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 } diff --git a/services/pull/patch.go b/services/pull/patch.go index 184da9f659..27d7991919 100644 --- a/services/pull/patch.go +++ b/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) diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl index 63eba45832..ef4f9247cf 100644 --- a/templates/swagger/v1_json.tmpl +++ b/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": {