From 7f7f8bf6254d2c0037697bc7c0ba03581428a693 Mon Sep 17 00:00:00 2001 From: Enver Bisevac Date: Tue, 23 Jan 2024 12:36:04 +0000 Subject: [PATCH] [code-1288] new diff api features [paths, line expanders] (#968) --- .golangci.yml | 5 - app/api/controller/pullreq/pr_diff.go | 11 +- app/api/controller/repo/diff.go | 11 +- app/api/handler/pullreq/pr_diff.go | 20 +- app/api/handler/repo/diff.go | 20 +- app/api/openapi/openapi.go | 6 + app/api/openapi/pullreq.go | 38 +++- app/api/openapi/repo.go | 33 ++- app/api/request/git.go | 28 +++ app/api/request/git_test.go | 145 ++++++++++++ app/router/api.go | 2 + git/adapter.go | 3 +- git/adapter/blame_int_test.go | 2 +- git/adapter/diff.go | 188 +++++++++++++++- git/adapter/diff_int_test.go | 153 +++++++++++++ git/adapter/diff_test.go | 305 +++++++++++++++++--------- git/adapter/errors.go | 1 + git/adapter/merge_test.go | 4 +- git/adapter/setup_test.go | 10 +- git/diff.go | 16 +- git/interface.go | 6 +- git/types/types.go | 8 + 22 files changed, 856 insertions(+), 159 deletions(-) create mode 100644 app/api/request/git_test.go create mode 100644 git/adapter/diff_int_test.go diff --git a/.golangci.yml b/.golangci.yml index 024b94ba6..c50920bbc 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -34,11 +34,6 @@ linters-settings: # Default: 40 statements: 50 - gocognit: - # Minimal code complexity to report - # Default: 30 (but we recommend 10-20) - min-complexity: 20 - gocritic: # Settings passed to gocritic. # The settings key is the name of a supported gocritic checker. diff --git a/app/api/controller/pullreq/pr_diff.go b/app/api/controller/pullreq/pr_diff.go index 0a2a59ddc..987d5104b 100644 --- a/app/api/controller/pullreq/pr_diff.go +++ b/app/api/controller/pullreq/pr_diff.go @@ -21,6 +21,7 @@ import ( "github.com/harness/gitness/app/auth" "github.com/harness/gitness/git" + gittypes "github.com/harness/gitness/git/types" "github.com/harness/gitness/types" "github.com/harness/gitness/types/enum" ) @@ -28,11 +29,12 @@ import ( // RawDiff writes raw git diff to writer w. func (c *Controller) RawDiff( ctx context.Context, + w io.Writer, session *auth.Session, repoRef string, pullreqNum int64, setSHAs func(sourceSHA, mergeBaseSHA string), - w io.Writer, + files ...gittypes.FileDiffRequest, ) error { repo, err := c.getRepoCheckAccess(ctx, session, repoRef, enum.PermissionRepoView) if err != nil { @@ -48,12 +50,12 @@ func (c *Controller) RawDiff( setSHAs(pr.SourceSHA, pr.MergeBaseSHA) } - return c.git.RawDiff(ctx, &git.DiffParams{ + return c.git.RawDiff(ctx, w, &git.DiffParams{ ReadParams: git.CreateReadParams(repo), BaseRef: pr.MergeBaseSHA, HeadRef: pr.SourceSHA, MergeBase: true, - }, w) + }, files...) } func (c *Controller) Diff( @@ -63,6 +65,7 @@ func (c *Controller) Diff( pullreqNum int64, setSHAs func(sourceSHA, mergeBaseSHA string), includePatch bool, + files ...gittypes.FileDiffRequest, ) (types.Stream[*git.FileDiff], error) { repo, err := c.getRepoCheckAccess(ctx, session, repoRef, enum.PermissionRepoView) if err != nil { @@ -84,7 +87,7 @@ func (c *Controller) Diff( HeadRef: pr.SourceSHA, MergeBase: true, IncludePatch: includePatch, - })) + }, files...)) return reader, nil } diff --git a/app/api/controller/repo/diff.go b/app/api/controller/repo/diff.go index eb30f9518..1a06d4886 100644 --- a/app/api/controller/repo/diff.go +++ b/app/api/controller/repo/diff.go @@ -23,16 +23,18 @@ import ( "github.com/harness/gitness/app/api/usererror" "github.com/harness/gitness/app/auth" "github.com/harness/gitness/git" + gittypes "github.com/harness/gitness/git/types" "github.com/harness/gitness/types" "github.com/harness/gitness/types/enum" ) func (c *Controller) RawDiff( ctx context.Context, + w io.Writer, session *auth.Session, repoRef string, path string, - w io.Writer, + files ...gittypes.FileDiffRequest, ) error { repo, err := c.getRepoCheckAccess(ctx, session, repoRef, enum.PermissionRepoView, true) if err != nil { @@ -44,12 +46,12 @@ func (c *Controller) RawDiff( return err } - return c.git.RawDiff(ctx, &git.DiffParams{ + return c.git.RawDiff(ctx, w, &git.DiffParams{ ReadParams: git.CreateReadParams(repo), BaseRef: info.BaseRef, HeadRef: info.HeadRef, MergeBase: info.MergeBase, - }, w) + }, files...) } func (c *Controller) CommitDiff( @@ -130,6 +132,7 @@ func (c *Controller) Diff( repoRef string, path string, includePatch bool, + files ...gittypes.FileDiffRequest, ) (types.Stream[*git.FileDiff], error) { repo, err := c.repoStore.FindByRef(ctx, repoRef) if err != nil { @@ -151,7 +154,7 @@ func (c *Controller) Diff( HeadRef: info.HeadRef, MergeBase: info.MergeBase, IncludePatch: includePatch, - })) + }, files...)) return reader, nil } diff --git a/app/api/handler/pullreq/pr_diff.go b/app/api/handler/pullreq/pr_diff.go index 754ac7e01..861a489a3 100644 --- a/app/api/handler/pullreq/pr_diff.go +++ b/app/api/handler/pullreq/pr_diff.go @@ -15,12 +15,16 @@ package pullreq import ( + "encoding/json" + "io" "net/http" "strings" "github.com/harness/gitness/app/api/controller/pullreq" "github.com/harness/gitness/app/api/render" "github.com/harness/gitness/app/api/request" + "github.com/harness/gitness/errors" + gittypes "github.com/harness/gitness/git/types" ) // HandleDiff returns a http.HandlerFunc that returns diff. @@ -45,9 +49,21 @@ func HandleDiff(pullreqCtrl *pullreq.Controller) http.HandlerFunc { w.Header().Set("X-Source-Sha", sourceSHA) w.Header().Set("X-Merge-Base-Sha", mergeBaseSHA) } + files := gittypes.FileDiffRequests{} + + switch r.Method { + case http.MethodPost: + if err = json.NewDecoder(r.Body).Decode(&files); err != nil && !errors.Is(err, io.EOF) { + render.TranslatedUserError(w, err) + return + } + case http.MethodGet: + // TBD: this will be removed in future because of URL limit in browser to 2048 chars. + files = request.GetFileDiffFromQuery(r) + } if strings.HasPrefix(r.Header.Get("Accept"), "text/plain") { - err := pullreqCtrl.RawDiff(ctx, session, repoRef, pullreqNumber, setSHAs, w) + err := pullreqCtrl.RawDiff(ctx, w, session, repoRef, pullreqNumber, setSHAs, files...) if err != nil { http.Error(w, err.Error(), http.StatusOK) } @@ -55,7 +71,7 @@ func HandleDiff(pullreqCtrl *pullreq.Controller) http.HandlerFunc { } _, includePatch := request.QueryParam(r, "include_patch") - stream, err := pullreqCtrl.Diff(ctx, session, repoRef, pullreqNumber, setSHAs, includePatch) + stream, err := pullreqCtrl.Diff(ctx, session, repoRef, pullreqNumber, setSHAs, includePatch, files...) if err != nil { render.TranslatedUserError(w, err) return diff --git a/app/api/handler/repo/diff.go b/app/api/handler/repo/diff.go index 8afba980a..129fb7e7a 100644 --- a/app/api/handler/repo/diff.go +++ b/app/api/handler/repo/diff.go @@ -15,12 +15,16 @@ package repo import ( + "encoding/json" + "io" "net/http" "strings" "github.com/harness/gitness/app/api/controller/repo" "github.com/harness/gitness/app/api/render" "github.com/harness/gitness/app/api/request" + "github.com/harness/gitness/errors" + gittypes "github.com/harness/gitness/git/types" ) // HandleDiff returns the diff between two commits, branches or tags. @@ -36,8 +40,20 @@ func HandleDiff(repoCtrl *repo.Controller) http.HandlerFunc { path := request.GetOptionalRemainderFromPath(r) + files := gittypes.FileDiffRequests{} + switch r.Method { + case http.MethodPost: + if err = json.NewDecoder(r.Body).Decode(&files); err != nil && !errors.Is(err, io.EOF) { + render.TranslatedUserError(w, err) + return + } + case http.MethodGet: + // TBD: this will be removed in future because of URL limit in browser to 2048 chars. + files = request.GetFileDiffFromQuery(r) + } + if strings.HasPrefix(r.Header.Get("Accept"), "text/plain") { - err := repoCtrl.RawDiff(ctx, session, repoRef, path, w) + err := repoCtrl.RawDiff(ctx, w, session, repoRef, path, files...) if err != nil { http.Error(w, err.Error(), http.StatusOK) } @@ -45,7 +61,7 @@ func HandleDiff(repoCtrl *repo.Controller) http.HandlerFunc { } _, includePatch := request.QueryParam(r, "include_patch") - stream, err := repoCtrl.Diff(ctx, session, repoRef, path, includePatch) + stream, err := repoCtrl.Diff(ctx, session, repoRef, path, includePatch, files...) if err != nil { render.TranslatedUserError(w, err) return diff --git a/app/api/openapi/openapi.go b/app/api/openapi/openapi.go index 4dbce4a8c..dfd0c782a 100644 --- a/app/api/openapi/openapi.go +++ b/app/api/openapi/openapi.go @@ -89,3 +89,9 @@ func Generate() *openapi3.Spec { return reflector.Spec } + +func panicOnErr(err error) { + if err != nil { + panic(err) + } +} diff --git a/app/api/openapi/pullreq.go b/app/api/openapi/pullreq.go index 7cfe11dbd..1f69375de 100644 --- a/app/api/openapi/pullreq.go +++ b/app/api/openapi/pullreq.go @@ -21,6 +21,7 @@ import ( "github.com/harness/gitness/app/api/request" "github.com/harness/gitness/app/api/usererror" "github.com/harness/gitness/git" + gittypes "github.com/harness/gitness/git/types" "github.com/harness/gitness/types" "github.com/harness/gitness/types/enum" @@ -122,6 +123,16 @@ type fileViewDeletePullReqRequest struct { Path string `path:"file_path"` } +type getRawPRDiffRequest struct { + pullReqRequest + Path []string `query:"path" description:"provide path for diff operation"` +} + +type postRawPRDiffRequest struct { + pullReqRequest + gittypes.FileDiffRequests +} + var queryParameterQueryPullRequest = openapi3.ParameterOrRef{ Parameter: &openapi3.Parameter{ Name: request.QueryParamQuery, @@ -549,11 +560,24 @@ func pullReqOperations(reflector *openapi3.Reflector) { opDiff := openapi3.Operation{} opDiff.WithTags("pullreq") opDiff.WithMapOfAnything(map[string]interface{}{"operationId": "diffPullReq"}) - _ = reflector.SetStringResponse(&opDiff, http.StatusOK, "text/plain") - _ = reflector.SetJSONResponse(&opDiff, new([]git.FileDiff), http.StatusOK) - _ = reflector.SetJSONResponse(&opDiff, new(usererror.Error), http.StatusInternalServerError) - _ = reflector.SetJSONResponse(&opDiff, new(usererror.Error), http.StatusUnauthorized) - _ = reflector.SetJSONResponse(&opDiff, new(usererror.Error), http.StatusForbidden) - _ = reflector.SetJSONResponse(&opDiff, new(usererror.Error), http.StatusNotFound) - _ = reflector.Spec.AddOperation(http.MethodGet, "/repos/{repo_ref}/pullreq/{pullreq_number}/diff", opDiff) + panicOnErr(reflector.SetRequest(&opDiff, new(getRawPRDiffRequest), http.MethodGet)) + panicOnErr(reflector.SetStringResponse(&opDiff, http.StatusOK, "text/plain")) + panicOnErr(reflector.SetJSONResponse(&opDiff, new([]git.FileDiff), http.StatusOK)) + panicOnErr(reflector.SetJSONResponse(&opDiff, new(usererror.Error), http.StatusInternalServerError)) + panicOnErr(reflector.SetJSONResponse(&opDiff, new(usererror.Error), http.StatusUnauthorized)) + panicOnErr(reflector.SetJSONResponse(&opDiff, new(usererror.Error), http.StatusForbidden)) + panicOnErr(reflector.SetJSONResponse(&opDiff, new(usererror.Error), http.StatusNotFound)) + panicOnErr(reflector.Spec.AddOperation(http.MethodGet, "/repos/{repo_ref}/pullreq/{pullreq_number}/diff", opDiff)) + + opPostDiff := openapi3.Operation{} + opPostDiff.WithTags("pullreq") + opPostDiff.WithMapOfAnything(map[string]interface{}{"operationId": "diffPullReqPost"}) + panicOnErr(reflector.SetRequest(&opPostDiff, new(postRawPRDiffRequest), http.MethodPost)) + panicOnErr(reflector.SetStringResponse(&opPostDiff, http.StatusOK, "text/plain")) + panicOnErr(reflector.SetJSONResponse(&opPostDiff, new([]git.FileDiff), http.StatusOK)) + panicOnErr(reflector.SetJSONResponse(&opPostDiff, new(usererror.Error), http.StatusInternalServerError)) + panicOnErr(reflector.SetJSONResponse(&opPostDiff, new(usererror.Error), http.StatusUnauthorized)) + panicOnErr(reflector.SetJSONResponse(&opPostDiff, new(usererror.Error), http.StatusForbidden)) + panicOnErr(reflector.SetJSONResponse(&opPostDiff, new(usererror.Error), http.StatusNotFound)) + panicOnErr(reflector.Spec.AddOperation(http.MethodPost, "/repos/{repo_ref}/pullreq/{pullreq_number}/diff", opPostDiff)) } diff --git a/app/api/openapi/repo.go b/app/api/openapi/repo.go index 338fe5ca3..c32b617b4 100644 --- a/app/api/openapi/repo.go +++ b/app/api/openapi/repo.go @@ -22,6 +22,7 @@ import ( "github.com/harness/gitness/app/api/usererror" "github.com/harness/gitness/app/services/protection" "github.com/harness/gitness/git" + gittypes "github.com/harness/gitness/git/types" "github.com/harness/gitness/types" "github.com/harness/gitness/types/enum" @@ -154,6 +155,13 @@ type deleteTagRequest struct { type getRawDiffRequest struct { repoRequest + Range string `path:"range" example:"main..dev"` + Path []string `query:"path" description:"provide path for diff operation"` +} + +type postRawDiffRequest struct { + repoRequest + gittypes.FileDiffRequests Range string `path:"range" example:"main..dev"` } @@ -720,13 +728,24 @@ func repoOperations(reflector *openapi3.Reflector) { opDiff := openapi3.Operation{} opDiff.WithTags("repository") opDiff.WithMapOfAnything(map[string]interface{}{"operationId": "rawDiff"}) - _ = reflector.SetRequest(&opDiff, new(getRawDiffRequest), http.MethodGet) - _ = reflector.SetStringResponse(&opDiff, http.StatusOK, "text/plain") - _ = reflector.SetJSONResponse(&opDiff, []git.FileDiff{}, http.StatusOK) - _ = reflector.SetJSONResponse(&opDiff, new(usererror.Error), http.StatusInternalServerError) - _ = reflector.SetJSONResponse(&opDiff, new(usererror.Error), http.StatusUnauthorized) - _ = reflector.SetJSONResponse(&opDiff, new(usererror.Error), http.StatusForbidden) - _ = reflector.Spec.AddOperation(http.MethodGet, "/repos/{repo_ref}/diff/{range}", opDiff) + panicOnErr(reflector.SetRequest(&opDiff, new(getRawDiffRequest), http.MethodGet)) + panicOnErr(reflector.SetStringResponse(&opDiff, http.StatusOK, "text/plain")) + panicOnErr(reflector.SetJSONResponse(&opDiff, []git.FileDiff{}, http.StatusOK)) + panicOnErr(reflector.SetJSONResponse(&opDiff, new(usererror.Error), http.StatusInternalServerError)) + panicOnErr(reflector.SetJSONResponse(&opDiff, new(usererror.Error), http.StatusUnauthorized)) + panicOnErr(reflector.SetJSONResponse(&opDiff, new(usererror.Error), http.StatusForbidden)) + panicOnErr(reflector.Spec.AddOperation(http.MethodGet, "/repos/{repo_ref}/diff/{range}", opDiff)) + + opPostDiff := openapi3.Operation{} + opPostDiff.WithTags("repository") + opPostDiff.WithMapOfAnything(map[string]interface{}{"operationId": "rawDiffPost"}) + panicOnErr(reflector.SetRequest(&opPostDiff, new(postRawDiffRequest), http.MethodPost)) + panicOnErr(reflector.SetStringResponse(&opPostDiff, http.StatusOK, "text/plain")) + panicOnErr(reflector.SetJSONResponse(&opPostDiff, []git.FileDiff{}, http.StatusOK)) + panicOnErr(reflector.SetJSONResponse(&opPostDiff, new(usererror.Error), http.StatusInternalServerError)) + panicOnErr(reflector.SetJSONResponse(&opPostDiff, new(usererror.Error), http.StatusUnauthorized)) + panicOnErr(reflector.SetJSONResponse(&opPostDiff, new(usererror.Error), http.StatusForbidden)) + panicOnErr(reflector.Spec.AddOperation(http.MethodPost, "/repos/{repo_ref}/diff/{range}", opPostDiff)) opCommitDiff := openapi3.Operation{} opCommitDiff.WithTags("repository") diff --git a/app/api/request/git.go b/app/api/request/git.go index 0f927e000..18193c80d 100644 --- a/app/api/request/git.go +++ b/app/api/request/git.go @@ -17,9 +17,11 @@ package request import ( "fmt" "net/http" + "strconv" "strings" "github.com/harness/gitness/app/api/usererror" + gittypes "github.com/harness/gitness/git/types" "github.com/harness/gitness/types" "github.com/harness/gitness/types/enum" ) @@ -132,3 +134,29 @@ func GetGitServiceTypeFromQuery(r *http.Request) (enum.GitServiceType, error) { return enum.ParseGitServiceType(val[len(gitPrefix):]) } + +func GetFileDiffFromQuery(r *http.Request) (files gittypes.FileDiffRequests) { + paths, _ := QueryParamList(r, "path") + ranges, _ := QueryParamList(r, "range") + + for i, filepath := range paths { + start := 0 + end := 0 + if i < len(ranges) { + linesRange := ranges[i] + parts := strings.Split(linesRange, ":") + if len(parts) > 1 { + end, _ = strconv.Atoi(parts[1]) + } + if len(parts) > 0 { + start, _ = strconv.Atoi(parts[0]) + } + } + files = append(files, gittypes.FileDiffRequest{ + Path: filepath, + StartLine: start, + EndLine: end, + }) + } + return +} diff --git a/app/api/request/git_test.go b/app/api/request/git_test.go new file mode 100644 index 000000000..4763fe9d3 --- /dev/null +++ b/app/api/request/git_test.go @@ -0,0 +1,145 @@ +// Copyright 2023 Harness, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package request + +import ( + "net/http" + "net/url" + "reflect" + "testing" + + "github.com/harness/gitness/git/types" +) + +func TestGetFileDiffRequestsFromQuery(t *testing.T) { + type args struct { + r *http.Request + } + tests := []struct { + name string + args args + wantFiles types.FileDiffRequests + }{ + { + name: "full range", + args: args{ + r: &http.Request{ + URL: &url.URL{ + Path: "/diff", + RawQuery: "path=file.txt&range=1:20", + }, + }, + }, + wantFiles: types.FileDiffRequests{ + { + Path: "file.txt", + StartLine: 1, + EndLine: 20, + }, + }, + }, + { + name: "start range", + args: args{ + r: &http.Request{ + URL: &url.URL{ + Path: "/diff", + RawQuery: "path=file.txt&range=1", + }, + }, + }, + wantFiles: types.FileDiffRequests{ + { + Path: "file.txt", + StartLine: 1, + }, + }, + }, + { + name: "end range", + args: args{ + r: &http.Request{ + URL: &url.URL{ + Path: "/diff", + RawQuery: "path=file.txt&range=:20", + }, + }, + }, + wantFiles: types.FileDiffRequests{ + { + Path: "file.txt", + EndLine: 20, + }, + }, + }, + { + name: "multi path", + args: args{ + r: &http.Request{ + URL: &url.URL{ + Path: "/diff", + RawQuery: "path=file.txt&range=:20&path=file1.txt&range=&path=file2.txt&range=1:15", + }, + }, + }, + wantFiles: types.FileDiffRequests{ + { + Path: "file.txt", + EndLine: 20, + }, + { + Path: "file1.txt", + }, + { + Path: "file2.txt", + StartLine: 1, + EndLine: 15, + }, + }, + }, + { + name: "multi path without some range", + args: args{ + r: &http.Request{ + URL: &url.URL{ + Path: "/diff", + RawQuery: "path=file.txt&range=:20&path=file1.txt&path=file2.txt&range=1:15", + }, + }, + }, + wantFiles: types.FileDiffRequests{ + { + Path: "file.txt", + EndLine: 20, + }, + { + Path: "file1.txt", + StartLine: 1, + EndLine: 15, + }, + { + Path: "file2.txt", + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if gotFiles := GetFileDiffFromQuery(tt.args.r); !reflect.DeepEqual(gotFiles, tt.wantFiles) { + t.Errorf("GetFileDiffFromQuery() = %v, want %v", gotFiles, tt.wantFiles) + } + }) + } +} diff --git a/app/router/api.go b/app/router/api.go index 32508ff3d..37c336448 100644 --- a/app/router/api.go +++ b/app/router/api.go @@ -320,6 +320,7 @@ func setupRepos(r chi.Router, // diffs r.Route("/diff", func(r chi.Router) { r.Get("/*", handlerrepo.HandleDiff(repoCtrl)) + r.Post("/*", handlerrepo.HandleDiff(repoCtrl)) }) r.Route("/diff-stats", func(r chi.Router) { r.Get("/*", handlerrepo.HandleDiffStats(repoCtrl)) @@ -518,6 +519,7 @@ func SetupPullReq(r chi.Router, pullreqCtrl *pullreq.Controller) { }) r.Get("/codeowners", handlerpullreq.HandleCodeOwner(pullreqCtrl)) r.Get("/diff", handlerpullreq.HandleDiff(pullreqCtrl)) + r.Post("/diff", handlerpullreq.HandleDiff(pullreqCtrl)) }) }) } diff --git a/git/adapter.go b/git/adapter.go index a28a599cd..0ed78b97a 100644 --- a/git/adapter.go +++ b/git/adapter.go @@ -88,11 +88,12 @@ type Adapter interface { headBranch string) (string, error) RawDiff(ctx context.Context, + w io.Writer, repoPath, base, head string, mergeBase bool, - w io.Writer) error + paths ...types.FileDiffRequest) error CommitDiff(ctx context.Context, repoPath, diff --git a/git/adapter/blame_int_test.go b/git/adapter/blame_int_test.go index 6fb19d3ea..e5469dd96 100644 --- a/git/adapter/blame_int_test.go +++ b/git/adapter/blame_int_test.go @@ -29,7 +29,7 @@ func TestBlameEmptyFile(t *testing.T) { baseBranch := "main" // write empty file to main branch - parentSHA := writeFile(t, repo, "file.txt", "", nil) + _, parentSHA := writeFile(t, repo, "file.txt", "", nil) err := repo.SetReference("refs/heads/"+baseBranch, parentSHA.String()) if err != nil { diff --git a/git/adapter/diff.go b/git/adapter/diff.go index a78c7112b..0265e41c6 100644 --- a/git/adapter/diff.go +++ b/git/adapter/diff.go @@ -15,10 +15,13 @@ package adapter import ( + "bufio" "bytes" "context" "fmt" "io" + "math" + "strconv" "strings" "github.com/harness/gitness/errors" @@ -28,13 +31,122 @@ import ( "code.gitea.io/gitea/modules/git" ) +// modifyHeader needs to modify diff hunk header with the new start line +// and end line with calculated span. +// if diff hunk header is -100, 50 +100, 50 and startLine = 120, endLine=140 +// then we need to modify header to -120,20 +120,20. +// warning: changes are possible and param endLine may not exist in the future. +func modifyHeader(hunk types.HunkHeader, startLine, endLine int) []byte { + oldStartLine := hunk.OldLine + newStartLine := hunk.NewLine + oldSpan := hunk.OldSpan + newSpan := hunk.NewSpan + + oldEndLine := oldStartLine + oldSpan + newEndLine := newStartLine + newSpan + + if startLine > 0 { + if startLine < oldEndLine { + oldStartLine = startLine + } + + if startLine < newEndLine { + newStartLine = startLine + } + } + + if endLine > 0 { + if endLine < oldEndLine { + oldSpan = endLine - startLine + } else if oldEndLine > startLine { + oldSpan = oldEndLine - startLine + } + + if endLine < newEndLine { + newSpan = endLine - startLine + } else if newEndLine > startLine { + newSpan = newEndLine - startLine + } + } + + return []byte(fmt.Sprintf("@@ -%d,%d +%d,%d @@", + oldStartLine, oldSpan, newStartLine, newSpan)) +} + +// cutLinesFromFullFileDiff reads from r and writes to w headers and between +// startLine and endLine. if startLine and endLine is equal to 0 then it uses io.Copy +// warning: changes are possible and param endLine may not exist in the future +func cutLinesFromFullFileDiff(w io.Writer, r io.Reader, startLine, endLine int) error { + if startLine < 0 { + startLine = 0 + } + + if endLine < 0 { + endLine = 0 + } + + if startLine == 0 && endLine > 0 { + startLine = 1 + } + + if endLine < startLine { + endLine = 0 + } + + // no need for processing lines just copy the data + if startLine == 0 && endLine == 0 { + _, err := io.Copy(w, r) + return err + } + + linePos := 0 + start := false + scanner := bufio.NewScanner(r) + for scanner.Scan() { + line := scanner.Bytes() + + if start { + linePos++ + } + + if endLine > 0 && linePos > endLine { + break + } + + if linePos > 0 && + (startLine > 0 && linePos < startLine) { + continue + } + + if len(line) >= 2 && bytes.HasPrefix(line, []byte{'@', '@'}) { + hunk, ok := parser.ParseDiffHunkHeader(string(line)) // TBD: maybe reader? + if !ok { + return fmt.Errorf("failed to extract lines from diff, range [%d,%d] : %w", + startLine, endLine, ErrParseDiffHunkHeader) + } + line = modifyHeader(hunk, startLine, endLine) + start = true + } + + if _, err := w.Write(line); err != nil { + return err + } + + if _, err := w.Write([]byte{'\n'}); err != nil { + return err + } + } + return scanner.Err() +} + func (a Adapter) RawDiff( ctx context.Context, + w io.Writer, repoPath string, baseRef string, headRef string, mergeBase bool, - w io.Writer, + files ...types.FileDiffRequest, ) error { if repoPath == "" { return ErrRepositoryPathEmpty @@ -55,8 +167,78 @@ func (a Adapter) RawDiff( if mergeBase { args = append(args, "--merge-base") } - args = append(args, baseRef, headRef) + perFileDiffRequired := false + paths := make([]string, 0, len(files)) + if len(files) > 0 { + for _, file := range files { + paths = append(paths, file.Path) + if file.StartLine > 0 || file.EndLine > 0 { + perFileDiffRequired = true + } + } + } + processed := 0 + +again: + startLine := 0 + endLine := 0 + newargs := make([]string, len(args), len(args)+8) + copy(newargs, args) + + if len(files) > 0 { + startLine = files[processed].StartLine + endLine = files[processed].EndLine + } + + if perFileDiffRequired { + if startLine > 0 || endLine > 0 { + newargs = append(newargs, "-U"+strconv.Itoa(math.MaxInt32)) + } + paths = []string{files[processed].Path} + } + + newargs = append(newargs, baseRef, headRef) + + if len(paths) > 0 { + newargs = append(newargs, "--") + newargs = append(newargs, paths...) + } + + pipeRead, pipeWrite := io.Pipe() + go func() { + var err error + + defer func() { + // If running of the command below fails, make the pipe reader also fail with the same error. + _ = pipeWrite.CloseWithError(err) + }() + + err = a.rawDiff(ctx, pipeWrite, repoPath, baseRef, headRef, newargs...) + }() + + if err = cutLinesFromFullFileDiff(w, pipeRead, startLine, endLine); err != nil { + return err + } + + if perFileDiffRequired { + processed++ + if processed < len(files) { + goto again + } + } + + return nil +} + +func (a Adapter) rawDiff( + ctx context.Context, + w io.Writer, + repoPath string, + baseRef string, + headRef string, + args ...string, +) error { cmd := git.NewCommand(ctx, args...) cmd.SetDescription(fmt.Sprintf("GetDiffRange [repo_path: %s]", repoPath)) errbuf := bytes.Buffer{} @@ -68,7 +250,7 @@ func (a Adapter) RawDiff( if errbuf.Len() > 0 { err = &runStdError{err: err, stderr: errbuf.String()} } - return processGiteaErrorf(err, "git diff failed between '%s' and '%s'", baseRef, headRef) + return processGiteaErrorf(err, "git diff failed between %q and %q", baseRef, headRef) } return nil } diff --git a/git/adapter/diff_int_test.go b/git/adapter/diff_int_test.go new file mode 100644 index 000000000..3f15aa8b8 --- /dev/null +++ b/git/adapter/diff_int_test.go @@ -0,0 +1,153 @@ +// Copyright 2023 Harness, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package adapter_test + +import ( + "bytes" + "context" + "testing" +) + +func TestAdapter_RawDiff(t *testing.T) { + git := setupGit(t) + repo, teardown := setupRepo(t, git, "testrawdiff") + defer teardown() + + testFileName := "file.txt" + + baseBranch := "main" + // write file to main branch + oid1, parentSHA := writeFile(t, repo, testFileName, "some content", nil) + + err := repo.SetReference("refs/heads/"+baseBranch, parentSHA.String()) + if err != nil { + t.Fatalf("failed updating reference '%s': %v", baseBranch, err) + } + + baseTag := "0.0.1" + err = repo.CreateAnnotatedTag(baseTag, "test tag 1", parentSHA.String()) + if err != nil { + t.Fatalf("error creating annotated tag '%s': %v", baseTag, err) + } + + headBranch := "dev" + + // create branch + err = repo.CreateBranch(headBranch, baseBranch) + if err != nil { + t.Fatalf("failed creating a branch '%s': %v", headBranch, err) + } + + // write file to main branch + oid2, sha := writeFile(t, repo, testFileName, "new content", []string{parentSHA.String()}) + + err = repo.SetReference("refs/heads/"+headBranch, sha.String()) + if err != nil { + t.Fatalf("failed updating reference '%s': %v", headBranch, err) + } + + headTag := "0.0.2" + err = repo.CreateAnnotatedTag(headTag, "test tag 2", sha.String()) + if err != nil { + t.Fatalf("error creating annotated tag '%s': %v", headTag, err) + } + + want := `diff --git a/` + testFileName + ` b/` + testFileName + ` +index ` + oid1.String() + `..` + oid2.String() + ` 100644 +--- a/` + testFileName + ` ++++ b/` + testFileName + ` +@@ -1 +1 @@ +-some content +\ No newline at end of file ++new content +\ No newline at end of file +` + + type args struct { + ctx context.Context + repoPath string + baseRef string + headRef string + mergeBase bool + } + tests := []struct { + name string + args args + wantW string + wantErr bool + }{ + { + name: "test branches", + args: args{ + ctx: context.Background(), + repoPath: repo.Path, + baseRef: baseBranch, + headRef: headBranch, + mergeBase: false, + }, + wantW: want, + wantErr: false, + }, + { + name: "test annotated tag", + args: args{ + ctx: context.Background(), + repoPath: repo.Path, + baseRef: baseTag, + headRef: headTag, + mergeBase: false, + }, + wantW: want, + wantErr: false, + }, + { + name: "test branches using merge-base", + args: args{ + ctx: context.Background(), + repoPath: repo.Path, + baseRef: baseBranch, + headRef: headBranch, + mergeBase: true, + }, + wantW: want, + wantErr: false, + }, + { + name: "test annotated tag using merge-base", + args: args{ + ctx: context.Background(), + repoPath: repo.Path, + baseRef: baseTag, + headRef: headTag, + mergeBase: true, + }, + wantW: want, + wantErr: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + w := &bytes.Buffer{} + err := git.RawDiff(tt.args.ctx, w, tt.args.repoPath, tt.args.baseRef, tt.args.headRef, tt.args.mergeBase) + if (err != nil) != tt.wantErr { + t.Errorf("RawDiff() error = %v, wantErr %v", err, tt.wantErr) + return + } + if gotW := w.String(); gotW != tt.wantW { + t.Errorf("RawDiff() gotW = %v, want %v", gotW, tt.wantW) + } + }) + } +} diff --git a/git/adapter/diff_test.go b/git/adapter/diff_test.go index e6e9bc574..0ea01dd48 100644 --- a/git/adapter/diff_test.go +++ b/git/adapter/diff_test.go @@ -12,146 +12,233 @@ // See the License for the specific language governing permissions and // limitations under the License. -package adapter_test +package adapter import ( "bytes" - "context" + "io" + "reflect" + "strings" "testing" - "github.com/harness/gitness/git/adapter" + "github.com/harness/gitness/git/types" + + "github.com/google/go-cmp/cmp" ) -func TestAdapter_RawDiff(t *testing.T) { - git := setupGit(t) - repo, teardown := setupRepo(t, git, "testrawdiff") - defer teardown() - - baseBranch := "main" - // write file to main branch - parentSHA := writeFile(t, repo, "file.txt", "some content", nil) - - err := repo.SetReference("refs/heads/"+baseBranch, parentSHA.String()) - if err != nil { - t.Fatalf("failed updating reference '%s': %v", baseBranch, err) - } - - baseTag := "0.0.1" - err = repo.CreateAnnotatedTag(baseTag, "test tag 1", parentSHA.String()) - if err != nil { - t.Fatalf("error creating annotated tag '%s': %v", baseTag, err) - } - - headBranch := "dev" - - // create branch - err = repo.CreateBranch(headBranch, baseBranch) - if err != nil { - t.Fatalf("failed creating a branch '%s': %v", headBranch, err) - } - - // write file to main branch - sha := writeFile(t, repo, "file.txt", "new content", []string{parentSHA.String()}) - - err = repo.SetReference("refs/heads/"+headBranch, sha.String()) - if err != nil { - t.Fatalf("failed updating reference '%s': %v", headBranch, err) - } - - headTag := "0.0.2" - err = repo.CreateAnnotatedTag(headTag, "test tag 2", sha.String()) - if err != nil { - t.Fatalf("error creating annotated tag '%s': %v", headTag, err) - } - - want := `diff --git a/file.txt b/file.txt -index f0eec86f614944a81f87d879ebdc9a79aea0d7ea..47d2739ba2c34690248c8f91b84bb54e8936899a 100644 ---- a/file.txt -+++ b/file.txt -@@ -1 +1 @@ --some content -\ No newline at end of file -+new content -\ No newline at end of file -` - +func Test_modifyHeader(t *testing.T) { type args struct { - ctx context.Context - repoPath string - baseRef string - headRef string - mergeBase bool + hunk types.HunkHeader + startLine int + endLine int + } + tests := []struct { + name string + args args + want []byte + }{ + { + name: "test empty", + args: args{ + hunk: types.HunkHeader{ + OldLine: 0, + OldSpan: 0, + NewLine: 0, + NewSpan: 0, + }, + startLine: 2, + endLine: 10, + }, + want: []byte("@@ -0,0 +0,0 @@"), + }, + { + name: "test empty 1", + args: args{ + hunk: types.HunkHeader{ + OldLine: 0, + OldSpan: 0, + NewLine: 0, + NewSpan: 0, + }, + startLine: 0, + endLine: 0, + }, + want: []byte("@@ -0,0 +0,0 @@"), + }, + { + name: "test empty old", + args: args{ + hunk: types.HunkHeader{ + OldLine: 0, + OldSpan: 0, + NewLine: 1, + NewSpan: 10, + }, + startLine: 2, + endLine: 10, + }, + want: []byte("@@ -0,0 +2,8 @@"), + }, + { + name: "test empty new", + args: args{ + hunk: types.HunkHeader{ + OldLine: 1, + OldSpan: 10, + NewLine: 0, + NewSpan: 0, + }, + startLine: 2, + endLine: 10, + }, + want: []byte("@@ -2,8 +0,0 @@"), + }, + { + name: "test 1", + args: args{ + hunk: types.HunkHeader{ + OldLine: 2, + OldSpan: 20, + NewLine: 2, + NewSpan: 20, + }, + startLine: 5, + endLine: 10, + }, + want: []byte("@@ -5,5 +5,5 @@"), + }, + { + name: "test 2", + args: args{ + hunk: types.HunkHeader{ + OldLine: 2, + OldSpan: 20, + NewLine: 2, + NewSpan: 20, + }, + startLine: 15, + endLine: 25, + }, + want: []byte("@@ -15,7 +15,7 @@"), + }, + { + name: "test 4", + args: args{ + hunk: types.HunkHeader{ + OldLine: 1, + OldSpan: 10, + NewLine: 1, + NewSpan: 10, + }, + startLine: 15, + endLine: 20, + }, + want: []byte("@@ -1,10 +1,10 @@"), + }, + { + name: "test 5", + args: args{ + hunk: types.HunkHeader{ + OldLine: 1, + OldSpan: 108, + NewLine: 1, + NewSpan: 108, + }, + startLine: 5, + endLine: 0, + }, + want: []byte("@@ -5,108 +5,108 @@"), + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := modifyHeader(tt.args.hunk, tt.args.startLine, tt.args.endLine); !reflect.DeepEqual(got, tt.want) { + t.Errorf("modifyHeader() = %s, want %s", got, tt.want) + } + }) + } +} + +func Test_cutLinesFromFullDiff(t *testing.T) { + type args struct { + r io.Reader + startLine int + endLine int } tests := []struct { name string - adapter adapter.Adapter args args wantW string wantErr bool }{ { - name: "test branches", - adapter: git, + name: "test empty", args: args{ - ctx: context.Background(), - repoPath: repo.Path, - baseRef: baseBranch, - headRef: headBranch, - mergeBase: false, + r: strings.NewReader(`diff --git a/file.txt b/file.txt +index f0eec86f614944a81f87d879ebdc9a79aea0d7ea..47d2739ba2c34690248c8f91b84bb54e8936899a 100644 +--- a/file.txt ++++ b/file.txt +@@ -0,0 +0,0 @@ +`), + startLine: 2, + endLine: 10, }, - wantW: want, - wantErr: false, + wantW: `diff --git a/file.txt b/file.txt +index f0eec86f614944a81f87d879ebdc9a79aea0d7ea..47d2739ba2c34690248c8f91b84bb54e8936899a 100644 +--- a/file.txt ++++ b/file.txt +@@ -0,0 +0,0 @@ +`, }, { - name: "test annotated tag", - adapter: git, + name: "test 1", args: args{ - ctx: context.Background(), - repoPath: repo.Path, - baseRef: baseTag, - headRef: headTag, - mergeBase: false, + r: strings.NewReader(`diff --git a/file.txt b/file.txt +index f0eec86f614944a81f87d879ebdc9a79aea0d7ea..47d2739ba2c34690248c8f91b84bb54e8936899a 100644 +--- a/file.txt ++++ b/file.txt +@@ -1,9 +1,9 @@ +some content +some content +some content +-some content ++some content +some content +some content +some content +some content +some content +`), + startLine: 2, + endLine: 10, }, - wantW: want, - wantErr: false, - }, - { - name: "test branches using merge-base", - adapter: git, - args: args{ - ctx: context.Background(), - repoPath: repo.Path, - baseRef: baseBranch, - headRef: headBranch, - mergeBase: true, - }, - wantW: want, - wantErr: false, - }, - { - name: "test annotated tag using merge-base", - adapter: git, - args: args{ - ctx: context.Background(), - repoPath: repo.Path, - baseRef: baseTag, - headRef: headTag, - mergeBase: true, - }, - wantW: want, - wantErr: false, + wantW: `diff --git a/file.txt b/file.txt +index f0eec86f614944a81f87d879ebdc9a79aea0d7ea..47d2739ba2c34690248c8f91b84bb54e8936899a 100644 +--- a/file.txt ++++ b/file.txt +@@ -2,8 +2,8 @@ +some content +some content +-some content ++some content +some content +some content +some content +some content +some content +`, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { w := &bytes.Buffer{} - err := tt.adapter.RawDiff(tt.args.ctx, tt.args.repoPath, tt.args.baseRef, tt.args.headRef, tt.args.mergeBase, w) + err := cutLinesFromFullFileDiff(w, tt.args.r, tt.args.startLine, tt.args.endLine) if (err != nil) != tt.wantErr { - t.Errorf("RawDiff() error = %v, wantErr %v", err, tt.wantErr) + t.Errorf("cutLinesFromFullFileDiff() error = %v, wantErr %v", err, tt.wantErr) return } if gotW := w.String(); gotW != tt.wantW { - t.Errorf("RawDiff() gotW = %v, want %v", gotW, tt.wantW) + t.Errorf("cutLinesFromFullFileDiff() gotW = %v, want %v, diff: %s", gotW, tt.wantW, cmp.Diff(gotW, tt.wantW)) } }) } diff --git a/git/adapter/errors.go b/git/adapter/errors.go index d76112f2e..ceab74070 100644 --- a/git/adapter/errors.go +++ b/git/adapter/errors.go @@ -27,6 +27,7 @@ import ( var ( ErrRepositoryPathEmpty = errors.InvalidArgument("repository path cannot be empty") ErrBranchNameEmpty = errors.InvalidArgument("branch name cannot be empty") + ErrParseDiffHunkHeader = errors.Internal("failed to parse diff hunk header") ) type runStdError struct { diff --git a/git/adapter/merge_test.go b/git/adapter/merge_test.go index c5620a87a..78b516eee 100644 --- a/git/adapter/merge_test.go +++ b/git/adapter/merge_test.go @@ -28,7 +28,7 @@ func TestAdapter_GetMergeBase(t *testing.T) { baseBranch := "main" // write file to main branch - parentSHA := writeFile(t, repo, "file1.txt", "some content", nil) + _, parentSHA := writeFile(t, repo, "file1.txt", "some content", nil) err := repo.SetReference("refs/heads/"+baseBranch, parentSHA.String()) if err != nil { @@ -50,7 +50,7 @@ func TestAdapter_GetMergeBase(t *testing.T) { } // write file to main branch - sha := writeFile(t, repo, "file1.txt", "new content", []string{parentSHA.String()}) + _, sha := writeFile(t, repo, "file1.txt", "new content", []string{parentSHA.String()}) err = repo.SetReference("refs/heads/"+headBranch, sha.String()) if err != nil { diff --git a/git/adapter/setup_test.go b/git/adapter/setup_test.go index 82e617a79..b6df0d914 100644 --- a/git/adapter/setup_test.go +++ b/git/adapter/setup_test.go @@ -108,14 +108,14 @@ func writeFile( path string, content string, parents []string, -) gitea.SHA1 { +) (oid gitea.SHA1, commitSha gitea.SHA1) { t.Helper() - sha, err := repo.HashObject(strings.NewReader(content)) + oid, err := repo.HashObject(strings.NewReader(content)) if err != nil { t.Fatalf("failed to hash object: %v", err) } - err = repo.AddObjectToIndex("100644", sha, path) + err = repo.AddObjectToIndex("100644", oid, path) if err != nil { t.Fatalf("failed to add object to index: %v", err) } @@ -125,12 +125,12 @@ func writeFile( t.Fatalf("failed to write tree: %v", err) } - sha, err = repo.CommitTree(testAuthor, testCommitter, tree, gitea.CommitTreeOpts{ + commitSha, err = repo.CommitTree(testAuthor, testCommitter, tree, gitea.CommitTreeOpts{ Message: "write file operation", Parents: parents, }) if err != nil { t.Fatalf("failed to commit tree: %v", err) } - return sha + return oid, commitSha } diff --git a/git/diff.go b/git/diff.go index 393813f64..1430a7b4b 100644 --- a/git/diff.go +++ b/git/diff.go @@ -48,18 +48,23 @@ func (p DiffParams) Validate() error { return nil } -func (s *Service) RawDiff(ctx context.Context, params *DiffParams, out io.Writer) error { - return s.rawDiff(ctx, params, out) +func (s *Service) RawDiff( + ctx context.Context, + out io.Writer, + params *DiffParams, + files ...types.FileDiffRequest, +) error { + return s.rawDiff(ctx, out, params, files...) } -func (s *Service) rawDiff(ctx context.Context, params *DiffParams, w io.Writer) error { +func (s *Service) rawDiff(ctx context.Context, w io.Writer, params *DiffParams, files ...types.FileDiffRequest) error { if err := params.Validate(); err != nil { return err } repoPath := getFullPathForRepo(s.reposRoot, params.RepoUID) - err := s.adapter.RawDiff(ctx, repoPath, params.BaseRef, params.HeadRef, params.MergeBase, w) + err := s.adapter.RawDiff(ctx, w, repoPath, params.BaseRef, params.HeadRef, params.MergeBase, files...) if err != nil { return err } @@ -336,6 +341,7 @@ func parseFileDiffStatus(ftype diff.FileType) FileDiffStatus { func (s *Service) Diff( ctx context.Context, params *DiffParams, + files ...types.FileDiffRequest, ) (<-chan *FileDiff, <-chan error) { wg := sync.WaitGroup{} ch := make(chan *FileDiff) @@ -353,7 +359,7 @@ func (s *Service) Diff( return } - err := s.rawDiff(ctx, params, pw) + err := s.rawDiff(ctx, pw, params, files...) if err != nil { cherr <- err return diff --git a/git/interface.go b/git/interface.go index ab9dc8be5..42d529f15 100644 --- a/git/interface.go +++ b/git/interface.go @@ -17,6 +17,8 @@ package git import ( "context" "io" + + "github.com/harness/gitness/git/types" ) type Interface interface { @@ -66,8 +68,8 @@ type Interface interface { /* * Diff services */ - RawDiff(ctx context.Context, in *DiffParams, w io.Writer) error - Diff(ctx context.Context, in *DiffParams) (<-chan *FileDiff, <-chan error) + RawDiff(ctx context.Context, w io.Writer, in *DiffParams, files ...types.FileDiffRequest) error + Diff(ctx context.Context, in *DiffParams, files ...types.FileDiffRequest) (<-chan *FileDiff, <-chan error) DiffFileNames(ctx context.Context, in *DiffParams) (DiffFileNamesOutput, error) CommitDiff(ctx context.Context, params *GetCommitParams, w io.Writer) error DiffShortStat(ctx context.Context, params *DiffParams) (DiffShortStatOutput, error) diff --git a/git/types/types.go b/git/types/types.go index 9be011f8b..fb9e346da 100644 --- a/git/types/types.go +++ b/git/types/types.go @@ -383,3 +383,11 @@ type ObjectCount struct { Garbage int SizeGarbage int64 } + +type FileDiffRequest struct { + Path string `json:"path"` + StartLine int `json:"start_line"` + EndLine int `json:"-"` // warning: changes are possible and this field may not exist in the future +} + +type FileDiffRequests []FileDiffRequest