diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 25c85e7531..0a2ff03c7e 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -1628,7 +1628,7 @@ func GetPullRequestFiles(ctx *context.APIContext) { maxLines := setting.Git.MaxGitDiffLines // FIXME: If there are too many files in the repo, may cause some unpredictable issues. - diff, err := gitdiff.GetDiff(ctx, baseGitRepo, + diff, _, err := gitdiff.GetDiffSimple(ctx, baseGitRepo, &gitdiff.DiffOptions{ BeforeCommitID: startCommitID, AfterCommitID: endCommitID, diff --git a/routers/web/repo/commit.go b/routers/web/repo/commit.go index 3cd80a6777..89463d9d03 100644 --- a/routers/web/repo/commit.go +++ b/routers/web/repo/commit.go @@ -329,7 +329,7 @@ func Diff(ctx *context.Context) { maxLines, maxFiles = -1, -1 } - diff, err := gitdiff.GetDiff(ctx, gitRepo, &gitdiff.DiffOptions{ + diff, err := gitdiff.GetDiffFull(ctx, gitRepo, &gitdiff.DiffOptions{ AfterCommitID: commitID, SkipTo: ctx.FormString("skip-to"), MaxLines: maxLines, diff --git a/routers/web/repo/compare.go b/routers/web/repo/compare.go index cd49a33b99..f5826cf249 100644 --- a/routers/web/repo/compare.go +++ b/routers/web/repo/compare.go @@ -621,7 +621,7 @@ func PrepareCompareDiff( fileOnly := ctx.FormBool("file-only") - diff, err := gitdiff.GetDiff(ctx, ci.HeadGitRepo, + diff, err := gitdiff.GetDiffFull(ctx, ci.HeadGitRepo, &gitdiff.DiffOptions{ BeforeCommitID: beforeCommitID, AfterCommitID: headCommitID, diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index 7ae7fd416c..47a4aac499 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -981,7 +981,7 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi // as the viewed information is designed to be loaded only on latest PR // diff and if you're signed in. if !ctx.IsSigned || willShowSpecifiedCommit || willShowSpecifiedCommitRange { - diff, err = gitdiff.GetDiff(ctx, gitRepo, diffOptions, files...) + diff, err = gitdiff.GetDiffFull(ctx, gitRepo, diffOptions, files...) methodWithError = "GetDiff" } else { diff, err = gitdiff.SyncAndGetUserSpecificDiff(ctx, ctx.Doer.ID, pull, gitRepo, diffOptions, files...) diff --git a/services/convert/git_commit.go b/services/convert/git_commit.go index e041361737..4603cfac4d 100644 --- a/services/convert/git_commit.go +++ b/services/convert/git_commit.go @@ -210,7 +210,7 @@ func ToCommit(ctx context.Context, repo *repo_model.Repository, gitRepo *git.Rep // Get diff stats for commit if opts.Stat { - diff, err := gitdiff.GetDiff(ctx, gitRepo, &gitdiff.DiffOptions{ + diff, _, err := gitdiff.GetDiffSimple(ctx, gitRepo, &gitdiff.DiffOptions{ AfterCommitID: commit.ID.String(), }) if err != nil { diff --git a/services/gitdiff/gitdiff.go b/services/gitdiff/gitdiff.go index 01de76d8b5..989f69d4f4 100644 --- a/services/gitdiff/gitdiff.go +++ b/services/gitdiff/gitdiff.go @@ -1088,53 +1088,53 @@ type DiffOptions struct { FileOnly bool } -// GetDiff builds a Diff between two commits of a repository. +// GetDiffSimple builds a Diff between two commits of a repository. // Passing the empty string as beforeCommitID returns a diff from the parent commit. -// The whitespaceBehavior is either an empty string or a git flag -func GetDiff(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, files ...string) (*Diff, error) { - repoPath := gitRepo.Path - - var beforeCommit *git.Commit - commit, err := gitRepo.GetCommit(opts.AfterCommitID) +// The whitespaceBehavior is either an empty string or a git flag. +func GetDiffSimple(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, files ...string) (diff *Diff, afterCommit *git.Commit, err error) { + afterCommit, err = gitRepo.GetCommit(opts.AfterCommitID) if err != nil { - return nil, err + return nil, nil, fmt.Errorf("unable to get the after commit %q: %w", opts.AfterCommitID, err) } cmdCtx, cmdCancel := context.WithCancel(ctx) defer cmdCancel() - cmdDiff := git.NewCommand(cmdCtx) + cmdDiff := git.NewCommand(cmdCtx). + AddArguments("diff", "--src-prefix=\\a/", "--dst-prefix=\\b/", "-M"). + AddArguments(opts.WhitespaceBehavior...) + objectFormat, err := gitRepo.GetObjectFormat() if err != nil { - return nil, err + return nil, nil, fmt.Errorf("not able to determine the object format: %w", err) } - if (len(opts.BeforeCommitID) == 0 || opts.BeforeCommitID == objectFormat.EmptyObjectID().String()) && commit.ParentCount() == 0 { - cmdDiff.AddArguments("diff", "--src-prefix=\\a/", "--dst-prefix=\\b/", "-M"). - AddArguments(opts.WhitespaceBehavior...). - AddDynamicArguments(objectFormat.EmptyTree().String()). - AddDynamicArguments(opts.AfterCommitID) + // If before commit is empty or the empty object and the after commit has no + // parents, then use the empty tree as before commit. + // + // This is the case for a 'initial commit' of a Git tree, which obviously has + // no parents. + if (len(opts.BeforeCommitID) == 0 || opts.BeforeCommitID == objectFormat.EmptyObjectID().String()) && afterCommit.ParentCount() == 0 { + // Reset before commit ID to indicate empty tree was used. + opts.BeforeCommitID = "" + // Add enpty tree as before commit. + cmdDiff.AddDynamicArguments(objectFormat.EmptyTree().String()) } else { - actualBeforeCommitID := opts.BeforeCommitID - if len(actualBeforeCommitID) == 0 { - parentCommit, err := commit.Parent(0) + // If before commit ID is empty, use the first parent of the after commit. + if len(opts.BeforeCommitID) == 0 { + parentCommit, err := afterCommit.Parent(0) if err != nil { - return nil, err + return nil, nil, fmt.Errorf("not able to get first parent of %q: %w", afterCommit.ID.String(), err) } - actualBeforeCommitID = parentCommit.ID.String() + opts.BeforeCommitID = parentCommit.ID.String() } - cmdDiff.AddArguments("diff", "--src-prefix=\\a/", "--dst-prefix=\\b/", "-M"). - AddArguments(opts.WhitespaceBehavior...). - AddDynamicArguments(actualBeforeCommitID, opts.AfterCommitID) - opts.BeforeCommitID = actualBeforeCommitID - - beforeCommit, err = gitRepo.GetCommit(opts.BeforeCommitID) - if err != nil { - return nil, err - } + cmdDiff.AddDynamicArguments(opts.BeforeCommitID) } + // Add the after commit to the diff command. + cmdDiff.AddDynamicArguments(opts.AfterCommitID) + // In git 2.31, git diff learned --skip-to which we can use to shortcut skip to file // so if we are using at least this version of git we don't have to tell ParsePatch to do // the skipping for us @@ -1144,6 +1144,7 @@ func GetDiff(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, fi parsePatchSkipToFile = "" } + // If we only want to diff for some files, add that as well. cmdDiff.AddDashesAndList(files...) reader, writer := io.Pipe() @@ -1153,6 +1154,7 @@ func GetDiff(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, fi }() go func() { + repoPath := gitRepo.Path stderr := &bytes.Buffer{} cmdDiff.SetDescription(fmt.Sprintf("GetDiffRange [repo_path: %s]", repoPath)) if err := cmdDiff.Run(&git.RunOpts{ @@ -1167,14 +1169,33 @@ func GetDiff(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, fi _ = writer.Close() }() - diff, err := ParsePatch(cmdCtx, opts.MaxLines, opts.MaxLineCharacters, opts.MaxFiles, reader, parsePatchSkipToFile) + diff, err = ParsePatch(cmdCtx, opts.MaxLines, opts.MaxLineCharacters, opts.MaxFiles, reader, parsePatchSkipToFile) // Ensure the git process is killed if it didn't exit already cmdCancel() if err != nil { - return nil, fmt.Errorf("unable to ParsePatch: %w", err) + return nil, nil, fmt.Errorf("unable to parse a git diff: %w", err) } diff.Start = opts.SkipTo + return diff, afterCommit, nil +} + +func GetDiffFull(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, files ...string) (*Diff, error) { + diff, afterCommit, err := GetDiffSimple(ctx, gitRepo, opts, files...) + if err != nil { + return nil, err + } + + // If there's a before commit, then GetDiffSimple will set it, otherwise it + // is empty. + var beforeCommit *git.Commit + if len(opts.BeforeCommitID) != 0 { + beforeCommit, err = gitRepo.GetCommit(opts.BeforeCommitID) + if err != nil { + return nil, fmt.Errorf("unable to get before commit %q: %w", opts.BeforeCommitID, err) + } + } + checker, err := gitRepo.GitAttributeChecker(opts.AfterCommitID, git.LinguistAttributes...) if err != nil { return nil, fmt.Errorf("unable to GitAttributeChecker: %w", err) @@ -1210,7 +1231,7 @@ func GetDiff(ctx context.Context, gitRepo *git.Repository, opts *DiffOptions, fi diffFile.IsGenerated = analyze.IsGenerated(diffFile.Name) } - tailSection := diffFile.GetTailSection(gitRepo, beforeCommit, commit) + tailSection := diffFile.GetTailSection(gitRepo, beforeCommit, afterCommit) if tailSection != nil { diffFile.Sections = append(diffFile.Sections, tailSection) } @@ -1272,7 +1293,7 @@ func GetPullDiffStats(gitRepo *git.Repository, opts *DiffOptions) (*PullDiffStat // SyncAndGetUserSpecificDiff is like GetDiff, except that user specific data such as which files the given user has already viewed on the given PR will also be set // Additionally, the database asynchronously is updated if files have changed since the last review func SyncAndGetUserSpecificDiff(ctx context.Context, userID int64, pull *issues_model.PullRequest, gitRepo *git.Repository, opts *DiffOptions, files ...string) (*Diff, error) { - diff, err := GetDiff(ctx, gitRepo, opts, files...) + diff, err := GetDiffFull(ctx, gitRepo, opts, files...) if err != nil { return nil, err } diff --git a/services/gitdiff/gitdiff_test.go b/services/gitdiff/gitdiff_test.go index 532255fe84..3d3c8432c4 100644 --- a/services/gitdiff/gitdiff_test.go +++ b/services/gitdiff/gitdiff_test.go @@ -635,7 +635,7 @@ func TestGetDiffRangeWithWhitespaceBehavior(t *testing.T) { defer gitRepo.Close() for _, behavior := range []git.TrustedCmdArgs{{"-w"}, {"--ignore-space-at-eol"}, {"-b"}, nil} { - diffs, err := GetDiff(db.DefaultContext, gitRepo, + diffs, _, err := GetDiffSimple(db.DefaultContext, gitRepo, &DiffOptions{ AfterCommitID: "bd7063cc7c04689c4d082183d32a604ed27a24f9", BeforeCommitID: "559c156f8e0178b71cb44355428f24001b08fc68", @@ -651,6 +651,70 @@ func TestGetDiffRangeWithWhitespaceBehavior(t *testing.T) { } } +func TestGetDiffFull(t *testing.T) { + gitRepo, err := git.OpenRepository(git.DefaultContext, "./../../modules/git/tests/repos/language_stats_repo") + require.NoError(t, err) + + defer gitRepo.Close() + + t.Run("Initial commit", func(t *testing.T) { + diff, err := GetDiffFull(db.DefaultContext, gitRepo, + &DiffOptions{ + AfterCommitID: "8fee858da5796dfb37704761701bb8e800ad9ef3", + MaxLines: setting.Git.MaxGitDiffLines, + MaxLineCharacters: setting.Git.MaxGitDiffLineCharacters, + MaxFiles: setting.Git.MaxGitDiffFiles, + }) + require.NoError(t, err) + + assert.Empty(t, diff.Start) + assert.Empty(t, diff.End) + assert.False(t, diff.IsIncomplete) + assert.Equal(t, 5, diff.NumFiles) + assert.Equal(t, 23, diff.TotalAddition) + assert.Len(t, diff.Files, 5) + + assert.True(t, diff.Files[0].IsVendored) + assert.Equal(t, ".gitattributes", diff.Files[0].Name) + assert.Equal(t, "24139dae656713ba861751fb2c2ac38839349a7a", diff.Files[0].NameHash) + + assert.Equal(t, "Python", diff.Files[1].Language) + assert.Equal(t, "i-am-a-python.p", diff.Files[1].Name) + assert.Equal(t, "32154957b043de62cbcdbe254a53ec4c3e00c5a0", diff.Files[1].NameHash) + + assert.Equal(t, "java-hello/main.java", diff.Files[2].Name) + assert.Equal(t, "ef9f6a406a4cde7bb5480ba7b027bdc8cd6fa11d", diff.Files[2].NameHash) + + assert.Equal(t, "main.vendor.java", diff.Files[3].Name) + assert.Equal(t, "c94fd7272f109d4d21d6df2b637c864a5ab63f46", diff.Files[3].NameHash) + + assert.Equal(t, "python-hello/hello.py", diff.Files[4].Name) + assert.Equal(t, "021705ba8b98778dc4e277d3a6ea1b8c6122a7f9", diff.Files[4].NameHash) + }) + + t.Run("Normal diff", func(t *testing.T) { + diff, err := GetDiffFull(db.DefaultContext, gitRepo, + &DiffOptions{ + AfterCommitID: "341fca5b5ea3de596dc483e54c2db28633cd2f97", + BeforeCommitID: "8fee858da5796dfb37704761701bb8e800ad9ef3", + MaxLines: setting.Git.MaxGitDiffLines, + MaxLineCharacters: setting.Git.MaxGitDiffLineCharacters, + MaxFiles: setting.Git.MaxGitDiffFiles, + }) + require.NoError(t, err) + + assert.Empty(t, diff.Start) + assert.Empty(t, diff.End) + assert.False(t, diff.IsIncomplete) + assert.Equal(t, 1, diff.NumFiles) + assert.Equal(t, 1, diff.TotalAddition) + assert.Len(t, diff.Files, 1) + + assert.Equal(t, ".gitattributes", diff.Files[0].Name) + assert.Equal(t, "24139dae656713ba861751fb2c2ac38839349a7a", diff.Files[0].NameHash) + }) +} + func TestNoCrashes(t *testing.T) { type testcase struct { gitdiff string