Skip to content

Commit

Permalink
Distinction between two-dot and three-dot ("Compare") diffs (#1239)
Browse files Browse the repository at this point in the history 
Merging so everyone can pull, will address comments separately later today
  • Loading branch information
@johnnyaug
johnnyaug authored Jan 21, 2021
1 parent ba81451 commit 4f16652
Show file tree
Hide file tree
Showing 16 changed files with 457 additions and 106 deletions.
7 changes: 6 additions & 1 deletion api/api_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -842,7 +842,12 @@ func (c *Controller) RefsDiffRefsHandler() refs.DiffRefsHandler {
cataloger := deps.Cataloger
limit := int(swag.Int64Value(params.Amount))
after := swag.StringValue(params.After)
diff, hasMore, err := cataloger.Diff(deps.ctx, params.Repository, params.LeftRef, params.RightRef, catalog.DiffParams{
var diffFunc func(ctx context.Context, repository, leftReference string, rightReference string, params catalog.DiffParams) (catalog.Differences, bool, error)
diffFunc = cataloger.Compare // default diff type is three-dot
if swag.StringValue(params.Type) == string(models.DiffTypeTwoDot) {
diffFunc = cataloger.Diff
}
diff, hasMore, err := diffFunc(deps.ctx, params.Repository, params.LeftRef, params.RightRef, catalog.DiffParams{
Limit: limit,
After: after,
})
Expand Down
1 change: 1 addition & 0 deletions catalog/cataloger.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ type Cataloger interface {
RollbackCommit(ctx context.Context, repository, branch string, reference string) error

Diff(ctx context.Context, repository, leftReference string, rightReference string, params DiffParams) (Differences, bool, error)
Compare(ctx context.Context, repository, leftReference string, rightReference string, params DiffParams) (Differences, bool, error)
DiffUncommitted(ctx context.Context, repository, branch string, limit int, after string) (Differences, bool, error)

Merge(ctx context.Context, repository, leftBranch, rightBranch, committer, message string, metadata Metadata) (*MergeResult, error)
Expand Down
6 changes: 5 additions & 1 deletion catalog/mvcc/cataloger_diff.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,11 @@ import (

const DiffMaxLimit = 1000

func (c *cataloger) Diff(ctx context.Context, repository string, leftReference string, rightReference string, params catalog.DiffParams) (catalog.Differences, bool, error) {
func (c *cataloger) Diff(_ context.Context, _ string, _ string, _ string, _ catalog.DiffParams) (catalog.Differences, bool, error) {
return nil, false, catalog.ErrFeatureNotSupported
}

func (c *cataloger) Compare(ctx context.Context, repository string, leftReference string, rightReference string, params catalog.DiffParams) (catalog.Differences, bool, error) {
if err := Validate(ValidateFields{
{Name: "repository", IsValid: ValidateRepositoryName(repository)},
{Name: "leftReference", IsValid: ValidateReference(leftReference)},
Expand Down
42 changes: 21 additions & 21 deletions catalog/mvcc/cataloger_diff_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ func TestCataloger_DiffEmpty(t *testing.T) {
}
commitChanges(10, "Changes on master", "master")

res, hasMore, err := c.Diff(ctx, repository, "master", "master", catalog.DiffParams{Limit: 10})
res, hasMore, err := c.Compare(ctx, repository, "master", "master", catalog.DiffParams{Limit: 10})
testutil.MustDo(t, "Diff", err)
if len(res) != 0 {
t.Errorf("Diff: got %+v but expected nothing", res)
Expand All @@ -36,7 +36,7 @@ func TestCataloger_DiffEmpty(t *testing.T) {
}
}

func TestCataloger_Diff(t *testing.T) {
func TestCataloger_Compare(t *testing.T) {
ctx := context.Background()
c := testCataloger(t)
repository := testCatalogerRepo(t, ctx, c, "repo", "master")
Expand Down Expand Up @@ -64,13 +64,13 @@ func TestCataloger_Diff(t *testing.T) {
var after string
var differences catalog.Differences
for {
res, hasMore, err := c.Diff(ctx, repository, "branch1", "master", catalog.DiffParams{
res, hasMore, err := c.Compare(ctx, repository, "branch1", "master", catalog.DiffParams{
Limit: limit,
After: after,
})
testutil.MustDo(t, "list diff changes", err)
if len(res) > limit {
t.Fatalf("Diff() result length=%d, expected no more than %d", len(res), limit)
t.Fatalf("Compare() result length=%d, expected no more than %d", len(res), limit)
}
differences = append(differences, res...)
if !hasMore {
Expand Down Expand Up @@ -111,13 +111,13 @@ func TestCataloger_Diff(t *testing.T) {
}

// check the case of 0 amount
res, hasMore, err := c.Diff(ctx, repository, "branch1", "master", catalog.DiffParams{Limit: 0})
res, hasMore, err := c.Compare(ctx, repository, "branch1", "master", catalog.DiffParams{Limit: 0})
testutil.MustDo(t, "list diff changes with 0 limit", err)
if !hasMore {
t.Error("Diff() limit 0 hasMore should be true")
t.Error("Compare() limit 0 hasMore should be true")
}
if len(res) != 0 {
t.Errorf("Diff() limit 0 len results is %d, expected none", len(res))
t.Errorf("Compare() limit 0 len results is %d, expected none", len(res))
}
}

Expand Down Expand Up @@ -153,7 +153,7 @@ func TestCataloger_Diff_FromChild(t *testing.T) {
testutil.MustDo(t, "commit changes", err)

// diff changes between "branch1" and "master" (from child)
res, more, err := c.Diff(ctx, repository, "branch1", catalog.DefaultBranchName, catalog.DiffParams{Limit: -1})
res, more, err := c.Compare(ctx, repository, "branch1", catalog.DefaultBranchName, catalog.DiffParams{Limit: -1})
testutil.MustDo(t, "Diff changes between branch1 and master", err)
if more {
t.Fatal("Diff has more than expected differences")
Expand Down Expand Up @@ -197,7 +197,7 @@ func TestCataloger_Diff_SameBranch(t *testing.T) {
testutil.MustDo(t, "commit branch changes", err)

// diff changes between second and first commit
res, more, err := c.Diff(ctx, repository, secondCommit.Reference, firstCommit.Reference, catalog.DiffParams{Limit: -1})
res, more, err := c.Compare(ctx, repository, secondCommit.Reference, firstCommit.Reference, catalog.DiffParams{Limit: -1})
testutil.MustDo(t, "Diff changes from second and first commits", err)
if more {
t.Fatal("Diff has more than expected differences")
Expand All @@ -212,7 +212,7 @@ func TestCataloger_Diff_SameBranch(t *testing.T) {
}

// diff changes between first and second commit
res, more, err = c.Diff(ctx, repository, firstCommit.Reference, secondCommit.Reference, catalog.DiffParams{Limit: -1})
res, more, err = c.Compare(ctx, repository, firstCommit.Reference, secondCommit.Reference, catalog.DiffParams{Limit: -1})
testutil.MustDo(t, "Diff changes from first and second commits", err)
if more {
t.Fatal("Diff has more than expected differences")
Expand Down Expand Up @@ -261,7 +261,7 @@ func TestCataloger_Diff_SameBranchDiffMergedChanges(t *testing.T) {
testutil.MustDo(t, "merge more changes from master to branch1", err)

// diff changes between second and first commit
res, more, err := c.Diff(ctx, repository, secondCommit.Reference, firstCommit.Reference, catalog.DiffParams{Limit: -1})
res, more, err := c.Compare(ctx, repository, secondCommit.Reference, firstCommit.Reference, catalog.DiffParams{Limit: -1})
testutil.MustDo(t, "Diff changes from second and first commits", err)
if more {
t.Fatal("Diff has more than expected differences")
Expand All @@ -276,7 +276,7 @@ func TestCataloger_Diff_SameBranchDiffMergedChanges(t *testing.T) {
}

// diff changes between first and second commit
res, more, err = c.Diff(ctx, repository, firstCommit.Reference, secondCommit.Reference, catalog.DiffParams{Limit: -1})
res, more, err = c.Compare(ctx, repository, firstCommit.Reference, secondCommit.Reference, catalog.DiffParams{Limit: -1})
testutil.MustDo(t, "Diff changes from first and second commits", err)
if more {
t.Fatal("Diff has more than expected differences")
Expand All @@ -294,7 +294,7 @@ func TestCataloger_Diff_SameBranchDiffMergedChanges(t *testing.T) {
rewriteCommit, err := c.Commit(ctx, repository, "branch1", "rewrite file2", "tester", nil)
testutil.MustDo(t, "rewrite file2", err)

res, more, err = c.Diff(ctx, repository, rewriteCommit.Reference, secondCommit.Reference, catalog.DiffParams{Limit: -1})
res, more, err = c.Compare(ctx, repository, rewriteCommit.Reference, secondCommit.Reference, catalog.DiffParams{Limit: -1})
testutil.MustDo(t, "Diff changes from rewrite and second commits", err)
if more {
t.Fatal("Diff has more than expected differences")
Expand Down Expand Up @@ -366,7 +366,7 @@ func TestCataloger_Diff_FromChildThreeBranches(t *testing.T) {
t.Fatal("Merge Summary", diff)
}
// TODO(barak): enable test after diff between commits is supported
//differences, _, err := c.Diff(ctx, repository, commitLog.Parents[0], commitLog.Parents[1], -1, "")
//differences, _, err := c.Compare(ctx, repository, commitLog.Parents[0], commitLog.Parents[1], -1, "")
//testutil.MustDo(t, "diff merge changes", err)
//expectedDifferences := catalog.Differences{
// catalog.Difference{Type: catalog.DifferenceTypeChanged, Path: "/file2"},
Expand Down Expand Up @@ -442,7 +442,7 @@ func TestCataloger_Diff_FromParentThreeBranches(t *testing.T) {
testutil.MustDo(t, "commit branch changes", err)

// diff changes between master and branch0
res, more, err := c.Diff(ctx, repository, "master", "branch0", catalog.DiffParams{Limit: -1})
res, more, err := c.Compare(ctx, repository, "master", "branch0", catalog.DiffParams{Limit: -1})
testutil.MustDo(t, "Diff changes from master to branch0", err)
if more {
t.Fatal("Diff has more than expected differences")
Expand Down Expand Up @@ -477,31 +477,31 @@ func TestCataloger_Diff_AdditionalFields(t *testing.T) {
_, err := c.Commit(ctx, repository, "master", "checking changes on master", "tester", nil)
testutil.Must(t, err)

res, hasMore, err := c.Diff(ctx, repository, "master", "branch1", catalog.DiffParams{Limit: numOfEntries})
res, hasMore, err := c.Compare(ctx, repository, "master", "branch1", catalog.DiffParams{Limit: numOfEntries})
testutil.MustDo(t, "diff changes", err)
if hasMore {
t.Fatal("Diff() hasMore should be false")
t.Fatal("Compare() hasMore should be false")
}
expectedLen := 3
if len(res) != expectedLen {
t.Fatalf("Diff() len of result %d, expected %d", len(res), expectedLen)
t.Fatalf("Compare() len of result %d, expected %d", len(res), expectedLen)
}
for _, d := range res {
if d.PhysicalAddress != "" {
t.Fatalf("Diff result entry should not have physical address set (%s)", d.PhysicalAddress)
}
}

res, hasMore, err = c.Diff(ctx, repository, "master", "branch1", catalog.DiffParams{
res, hasMore, err = c.Compare(ctx, repository, "master", "branch1", catalog.DiffParams{
Limit: numOfEntries,
AdditionalFields: []string{catalog.DBEntryFieldPhysicalAddress, catalog.DBEntryFieldChecksum},
})
testutil.MustDo(t, "diff changes", err)
if hasMore {
t.Fatal("Diff() hasMore should be false")
t.Fatal("Compare() hasMore should be false")
}
if len(res) != expectedLen {
t.Fatalf("Diff() len of result %d, expected %d", len(res), expectedLen)
t.Fatalf("Compare() len of result %d, expected %d", len(res), expectedLen)
}
for _, d := range res {
if d.PhysicalAddress == "" {
Expand Down
2 changes: 1 addition & 1 deletion catalog/mvcc/cataloger_list_commits_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -373,7 +373,7 @@ func TestCataloger_ListCommits_LineageFromChild(t *testing.T) {
t.Fatal("Merge Summary", diff)
}
// TODO(barak): enable test after diff between commits is supported
//differences, _, err := c.Diff(ctx, repository, commitLog.Parents[0], commitLog.Parents[1], -1, "")
//differences, _, err := c.Compare(ctx, repository, commitLog.Parents[0], commitLog.Parents[1], -1, "")
//testutil.MustDo(t, "diff merge changes", err)
//
//if differences[0].Type != catalog.DifferenceTypeChanged || differences[0].Path != "master-file" {
Expand Down
Loading

0 comments on commit 4f16652

Please sign in to comment.