Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite compare functions #32786

Draft
wants to merge 17 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 33 additions & 57 deletions modules/git/repo_compare.go
Original file line number Diff line number Diff line change
Expand Up @@ -233,72 +233,63 @@ func parseDiffStat(stdout string) (numFiles, totalAdditions, totalDeletions int,
return numFiles, totalAdditions, totalDeletions, err
}

// GetDiffOrPatch generates either diff or formatted patch data between given revisions
func (repo *Repository) GetDiffOrPatch(base, head string, w io.Writer, patch, binary bool) error {
if patch {
return repo.GetPatch(base, head, w)
func parseCompareArgs(compareArgs string) (args []string) {
parts := strings.Split(compareArgs, "...")
if len(parts) == 2 {
return []string{compareArgs}
}
if binary {
return repo.GetDiffBinary(base, head, w)
parts = strings.Split(compareArgs, "..")
if len(parts) == 2 {
return parts
}
return repo.GetDiff(base, head, w)
parts = strings.Fields(compareArgs)
if len(parts) == 2 {
return parts
}

return nil
}

// GetDiff generates and returns patch data between given revisions, optimized for human readability
func (repo *Repository) GetDiff(base, head string, w io.Writer) error {
func (repo *Repository) GetDiff(compareArgs string, w io.Writer) error {
args := parseCompareArgs(compareArgs)
if len(args) == 0 {
return fmt.Errorf("invalid compareArgs: %s", compareArgs)
}
stderr := new(bytes.Buffer)
err := NewCommand(repo.Ctx, "diff", "-p").AddDynamicArguments(base + "..." + head).
return NewCommand(repo.Ctx, "diff", "-p").AddDynamicArguments(args...).
Run(&RunOpts{
Dir: repo.Path,
Stdout: w,
Stderr: stderr,
})
if err != nil && bytes.Contains(stderr.Bytes(), []byte("no merge base")) {
return NewCommand(repo.Ctx, "diff", "-p").AddDynamicArguments(base, head).
Run(&RunOpts{
Dir: repo.Path,
Stdout: w,
})
}
return err
}

// GetDiffBinary generates and returns patch data between given revisions, including binary diffs.
func (repo *Repository) GetDiffBinary(base, head string, w io.Writer) error {
stderr := new(bytes.Buffer)
err := NewCommand(repo.Ctx, "diff", "-p", "--binary", "--histogram").AddDynamicArguments(base + "..." + head).
Run(&RunOpts{
Dir: repo.Path,
Stdout: w,
Stderr: stderr,
})
if err != nil && bytes.Contains(stderr.Bytes(), []byte("no merge base")) {
return NewCommand(repo.Ctx, "diff", "-p", "--binary", "--histogram").AddDynamicArguments(base, head).
Run(&RunOpts{
Dir: repo.Path,
Stdout: w,
})
func (repo *Repository) GetDiffBinary(compareArgs string, w io.Writer) error {
args := parseCompareArgs(compareArgs)
if len(args) == 0 {
return fmt.Errorf("invalid compareArgs: %s", compareArgs)
}
return err
return NewCommand(repo.Ctx, "diff", "-p", "--binary", "--histogram").AddDynamicArguments(args...).Run(&RunOpts{
Dir: repo.Path,
Stdout: w,
})
}

// 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 {
func (repo *Repository) GetPatch(compareArgs string, w io.Writer) error {
args := parseCompareArgs(compareArgs)
if len(args) == 0 {
return fmt.Errorf("invalid compareArgs: %s", compareArgs)
}
stderr := new(bytes.Buffer)
err := NewCommand(repo.Ctx, "format-patch", "--binary", "--stdout").AddDynamicArguments(base + "..." + head).
return NewCommand(repo.Ctx, "format-patch", "--binary", "--stdout").AddDynamicArguments(args...).
Run(&RunOpts{
Dir: repo.Path,
Stdout: w,
Stderr: stderr,
})
if err != nil && bytes.Contains(stderr.Bytes(), []byte("no merge base")) {
return NewCommand(repo.Ctx, "format-patch", "--binary", "--stdout").AddDynamicArguments(base, head).
Run(&RunOpts{
Dir: repo.Path,
Stdout: w,
})
}
return err
}

// GetFilesChangedBetween returns a list of all files that have been changed between the given commits
Expand Down Expand Up @@ -329,21 +320,6 @@ func (repo *Repository) GetFilesChangedBetween(base, head string) ([]string, err
return split, err
}

// GetDiffFromMergeBase generates and return patch data from merge base to head
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove unused function.

func (repo *Repository) GetDiffFromMergeBase(base, head string, w io.Writer) error {
stderr := new(bytes.Buffer)
err := NewCommand(repo.Ctx, "diff", "-p", "--binary").AddDynamicArguments(base + "..." + head).
Run(&RunOpts{
Dir: repo.Path,
Stdout: w,
Stderr: stderr,
})
if err != nil && bytes.Contains(stderr.Bytes(), []byte("no merge base")) {
return repo.GetDiffBinary(base, head, w)
}
return err
}

// ReadPatchCommit will check if a diff patch exists and return stats
func (repo *Repository) ReadPatchCommit(prID int64) (commitSHA string, err error) {
// Migrated repositories download patches to "pulls" location
Expand Down
27 changes: 26 additions & 1 deletion modules/git/repo_compare_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,31 @@ import (
"github.com/stretchr/testify/assert"
)

func Test_parseCompareArgs(t *testing.T) {
testCases := []struct {
compareString string
expected []string
}{
{
"master..develop",
[]string{"master", "develop"},
},
{
"master HEAD",
[]string{"master", "HEAD"},
},
{
"HEAD...develop",
[]string{"HEAD...develop"},
},
}

for _, tc := range testCases {
args := parseCompareArgs(tc.compareString)
assert.Equal(t, tc.expected, args)
}
}

func TestGetFormatPatch(t *testing.T) {
bareRepo1Path := filepath.Join(testReposDir, "repo1_bare")
clonedPath, err := cloneRepo(t, bareRepo1Path)
Expand All @@ -28,7 +53,7 @@ func TestGetFormatPatch(t *testing.T) {
defer repo.Close()

rd := &bytes.Buffer{}
err = repo.GetPatch("8d92fc95^", "8d92fc95", rd)
err = repo.GetPatch("8d92fc95^ 8d92fc95", rd)
if err != nil {
assert.NoError(t, err)
return
Expand Down
19 changes: 15 additions & 4 deletions services/pull/patch.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,17 @@ import (
"github.com/gobwas/glob"
)

// getDiffOrPatch generates either diff or formatted patch data between given revisions
func getDiffOrPatch(repo *git.Repository, compareString string, w io.Writer, patch, binary bool) error {
if patch {
return repo.GetPatch(compareString, w)
}
if binary {
return repo.GetDiffBinary(compareString, w)
}
return repo.GetDiff(compareString, w)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is only one call to this function? Maybe it could be removed and just write the if/else in the caller.


// DownloadDiffOrPatch will write the patch for the pr to the writer
func DownloadDiffOrPatch(ctx context.Context, pr *issues_model.PullRequest, w io.Writer, patch, binary bool) error {
if err := pr.LoadBaseRepo(ctx); err != nil {
Expand All @@ -42,9 +53,9 @@ func DownloadDiffOrPatch(ctx context.Context, pr *issues_model.PullRequest, w io
}
defer closer.Close()

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: %w", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err)
if err := getDiffOrPatch(gitRepo, 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: %w", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err)
}
return nil
}
Expand Down Expand Up @@ -355,7 +366,7 @@ func checkConflicts(ctx context.Context, pr *issues_model.PullRequest, gitRepo *
_ = util.Remove(tmpPatchFile.Name())
}()

if err := gitRepo.GetDiffBinary(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: %w", pr.MergeBase, pr.HeadBranch, pr.BaseRepo.FullName(), err)
Expand Down
Loading