From 6c55b14ecb19e5800046295700d56e4f4d127aed Mon Sep 17 00:00:00 2001 From: Zach Musgrave Date: Mon, 2 Aug 2021 14:07:23 -0700 Subject: [PATCH 1/8] New merge tests, including two panics --- integration-tests/bats/merge.bats | 90 +++++++++++++++++++++++++++++++ 1 file changed, 90 insertions(+) diff --git a/integration-tests/bats/merge.bats b/integration-tests/bats/merge.bats index 7a1653f1e3..3ab15533c1 100644 --- a/integration-tests/bats/merge.bats +++ b/integration-tests/bats/merge.bats @@ -409,3 +409,93 @@ SQL skip "merge fails on unique index violation, should log conflict" dolt merge other } + +@test "merge: merge a branch with a new table" { + dolt branch feature-branch + + dolt sql << SQL +INSERT INTO test2 VALUES (0, 0, 0); +INSERT INTO test2 VALUES (1, 1, 1); +SQL + dolt add -A && dolt commit -am "add data to test 2" + + dolt checkout feature-branch + dolt sql << SQL +create table test3 (a int primary key); +INSERT INTO test3 VALUES (0), (1); +SQL + dolt commit -am "new table test3" + + dolt checkout master + + run dolt merge feature-branch + [ "$status" -eq 0 ] + + run dolt sql -q "select * from test3" + [ "$status" -eq 0 ] + [[ "$output" =~ 1 ]] || false +} + +@test "merge: merge a branch that deletes a table" { + dolt branch feature-branch + + dolt sql << SQL +INSERT INTO test1 VALUES (0, 0, 0); +INSERT INTO test1 VALUES (1, 1, 1); +SQL + dolt commit -am "add data to test1" + + dolt checkout feature-branch + dolt sql << SQL +INSERT INTO test1 VALUES (2, 2, 2); +drop table test2; +SQL + dolt commit -am "add data to test1, drop test2" + + dolt checkout master + run dolt merge feature-branch + [ "$status" -eq 0 ] + + run dolt sql -q "show tables" + [ "$status" -eq 0 ] + [[ ! "$output" =~ "test2" ]] || false + + run dolt sql -q "select * from test1" + [ "$status" -eq 0 ] + [[ "$output" =~ 2 ]] || false +} + + +@test "merge: merge branch with table that was deleted" { + dolt sql << SQL +INSERT INTO test2 VALUES (0, 0, 0); +INSERT INTO test2 VALUES (1, 1, 1); +SQL + dolt add -A && dolt commit -am "add data to test 2" + + dolt branch feature-branch + dolt sql -q "drop table test2" + dolt commit -am "drop table test2" + + dolt sql << SQL +INSERT INTO test1 VALUES (2, 2, 2); +INSERT INTO test1 VALUES (3, 3, 3); +SQL + dolt commit -am "add data to test1" + + dolt checkout feature-branch + dolt sql << SQL +INSERT INTO test1 VALUES (0, 0, 0); +INSERT INTO test1 VALUES (1, 1, 1); +SQL + dolt commit -am "add data to test1" + + dolt checkout master + run dolt merge feature-branch + + [ "$status" -eq 0 ] + + run dolt sql -q "show tables" + [ "$status" -eq 0 ] + [[ ! "$output" =~ "test2" ]] || false +} From c794bf0103f9b24f8fbb61aa4e7e68f19434340c Mon Sep 17 00:00:00 2001 From: Zach Musgrave Date: Mon, 2 Aug 2021 14:14:07 -0700 Subject: [PATCH 2/8] Fixed some documentation typos --- go/cmd/dolt/commands/remote.go | 9 +++++---- go/libraries/doltcore/merge/merge.go | 4 ++-- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/go/cmd/dolt/commands/remote.go b/go/cmd/dolt/commands/remote.go index aae3f7587a..2d130354cf 100644 --- a/go/cmd/dolt/commands/remote.go +++ b/go/cmd/dolt/commands/remote.go @@ -49,13 +49,14 @@ AWS cloud remote urls should be of the form {{.EmphasisLeft}}aws://[dynamo-table aws-creds-type specifies the means by which credentials should be retrieved in order to access the specified cloud resources (specifically the dynamo table, and the s3 bucket). Valid values are 'role', 'env', or 'file'. - \trole: Use the credentials installed for the current user - \tenv: Looks for environment variables AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY - \tfile: Uses the credentials file specified by the parameter aws-creds-file + role: Use the credentials installed for the current user + env: Looks for environment variables AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY + file: Uses the credentials file specified by the parameter aws-creds-file GCP remote urls should be of the form gs://gcs-bucket/database and will use the credentials setup using the gcloud command line available from Google + -The local filesystem can be used as a remote by providing a repository url in the format file://absolute path. See https://en.wikipedia.org/wiki/File_URI_schemethi +The local filesystem can be used as a remote by providing a repository url in the format file://absolute path. See https://en.wikipedia.org/wiki/File_URI_scheme + {{.EmphasisLeft}}remove{{.EmphasisRight}}, {{.EmphasisLeft}}rm{{.EmphasisRight}}, Remove the remote named {{.LessThan}}name{{.GreaterThan}}. All remote-tracking branches and configuration settings for the remote are removed.`, diff --git a/go/libraries/doltcore/merge/merge.go b/go/libraries/doltcore/merge/merge.go index 7c3a2379f4..25152a86f1 100644 --- a/go/libraries/doltcore/merge/merge.go +++ b/go/libraries/doltcore/merge/merge.go @@ -877,9 +877,9 @@ func MergeRoots(ctx context.Context, ourRoot, theirRoot, ancRoot *doltdb.RootVal if err != nil { return nil, nil, err } - } else if has, err := newRoot.HasTable(ctx, tblName); err != nil { + } else if newRootHasTable, err := newRoot.HasTable(ctx, tblName); err != nil { return nil, nil, err - } else if has { + } else if newRootHasTable { tblToStats[tblName] = &MergeStats{Operation: TableRemoved} err = tableEditSession.UpdateRoot(ctx, func(ctx context.Context, root *doltdb.RootValue) (*doltdb.RootValue, error) { return root.RemoveTables(ctx, tblName) From cbabc4a136ca6bd598bb3c5851ed59cd2153135b Mon Sep 17 00:00:00 2001 From: Zach Musgrave Date: Mon, 2 Aug 2021 15:29:56 -0700 Subject: [PATCH 3/8] Refactoring merge table method for readability --- go/libraries/doltcore/merge/merge.go | 96 +++++++++++++--------------- 1 file changed, 43 insertions(+), 53 deletions(-) diff --git a/go/libraries/doltcore/merge/merge.go b/go/libraries/doltcore/merge/merge.go index 25152a86f1..e9e6591dde 100644 --- a/go/libraries/doltcore/merge/merge.go +++ b/go/libraries/doltcore/merge/merge.go @@ -52,61 +52,51 @@ func NewMerger(ctx context.Context, root, mergeRoot, ancRoot *doltdb.RootValue, return &Merger{root, mergeRoot, ancRoot, vrw} } +func getTableInfoFromRoot(ctx context.Context, tblName string, root *doltdb.RootValue) ( + ok bool, + table *doltdb.Table, + sch schema.Schema, + h hash.Hash, + err error, +) { + table, ok, err = root.GetTable(ctx, tblName) + if err != nil { + return false, nil, nil, hash.Hash{}, err + } + + if ok { + h, err = table.HashOf() + if err != nil { + return false, nil, nil, hash.Hash{}, err + } + sch, err = table.GetSchema(ctx) + if err != nil { + return false, nil, nil, hash.Hash{}, err + } + } + + return ok, table, sch, h, nil +} + // MergeTable merges schema and table data for the table tblName. func (merger *Merger) MergeTable(ctx context.Context, tblName string, sess *editor.TableEditSession) (*doltdb.Table, *MergeStats, error) { - tbl, ok, err := merger.root.GetTable(ctx, tblName) + rootHasTable, tbl, rootSchema, rootHash, err := getTableInfoFromRoot(ctx, tblName, merger.root) if err != nil { return nil, nil, err } - var h hash.Hash - var tblSchema schema.Schema - if ok { - h, err = tbl.HashOf() - if err != nil { - return nil, nil, err - } - tblSchema, err = tbl.GetSchema(ctx) - if err != nil { - return nil, nil, err - } - } - - mergeTbl, mergeOk, err := merger.mergeRoot.GetTable(ctx, tblName) + mergeHasTable, mergeTbl, mergeSchema, mergeHash, err := getTableInfoFromRoot(ctx, tblName, merger.mergeRoot) if err != nil { return nil, nil, err } - var mh hash.Hash - var mergeTblSchema schema.Schema - if mergeOk { - mh, err = mergeTbl.HashOf() - if err != nil { - return nil, nil, err - } - mergeTblSchema, err = mergeTbl.GetSchema(ctx) - if err != nil { - return nil, nil, err - } - } - - ancTbl, ancOk, err := merger.ancRoot.GetTable(ctx, tblName) + ancHasTable, ancTbl, ancSchema, ancHash, err := getTableInfoFromRoot(ctx, tblName, merger.ancRoot) if err != nil { return nil, nil, err } - var anch hash.Hash - var ancTblSchema schema.Schema var ancRows types.Map - if ancOk { - anch, err = ancTbl.HashOf() - if err != nil { - return nil, nil, err - } - ancTblSchema, err = ancTbl.GetSchema(ctx) - if err != nil { - return nil, nil, err - } + if ancHasTable { ancRows, err = ancTbl.GetRowData(ctx) if err != nil { return nil, nil, err @@ -114,26 +104,26 @@ func (merger *Merger) MergeTable(ctx context.Context, tblName string, sess *edit } { // short-circuit logic - if ancOk && schema.IsKeyless(ancTblSchema) { - if ok && mergeOk && ancOk && h == mh && h == anch { + if ancHasTable && schema.IsKeyless(ancSchema) { + if rootHasTable && mergeHasTable && ancHasTable && rootHash == mergeHash && rootHash == ancHash { return tbl, &MergeStats{Operation: TableUnmodified}, nil } } else { - if ok && mergeOk && h == mh { + if rootHasTable && mergeHasTable && rootHash == mergeHash { return tbl, &MergeStats{Operation: TableUnmodified}, nil } } - if !ancOk { - if mergeOk && ok { - if schema.SchemasAreEqual(tblSchema, mergeTblSchema) { + if !ancHasTable { + if mergeHasTable && rootHasTable { + if schema.SchemasAreEqual(rootSchema, mergeSchema) { // hackity hack - ancTblSchema, ancTbl = tblSchema, tbl + ancSchema, ancTbl = rootSchema, tbl ancRows, _ = types.NewMap(ctx, merger.vrw) } else { return nil, nil, ErrSameTblAddedTwice } - } else if ok { + } else if rootHasTable { // fast-forward return tbl, &MergeStats{Operation: TableUnmodified}, nil } else { @@ -142,10 +132,10 @@ func (merger *Merger) MergeTable(ctx context.Context, tblName string, sess *edit } } - if h == anch { + if rootHash == ancHash { // fast-forward ms := MergeStats{Operation: TableModified} - if h != mh { + if rootHash != mergeHash { ms, err = calcTableMergeStats(ctx, tbl, mergeTbl) if err != nil { return nil, nil, err @@ -157,13 +147,13 @@ func (merger *Merger) MergeTable(ctx context.Context, tblName string, sess *edit return nil, nil, err } return mergeTbl, &ms, nil - } else if mh == anch { + } else if mergeHash == ancHash { // fast-forward return tbl, &MergeStats{Operation: TableUnmodified}, nil } } - postMergeSchema, schConflicts, err := SchemaMerge(tblSchema, mergeTblSchema, ancTblSchema, tblName) + postMergeSchema, schConflicts, err := SchemaMerge(rootSchema, mergeSchema, ancSchema, tblName) if err != nil { return nil, nil, err } @@ -192,7 +182,7 @@ func (merger *Merger) MergeTable(ctx context.Context, tblName string, sess *edit for _, index := range postMergeSchema.Indexes().AllIndexes() { addedIndexesSet[strings.ToLower(index.Name())] = index.Name() } - for _, index := range tblSchema.Indexes().AllIndexes() { + for _, index := range rootSchema.Indexes().AllIndexes() { delete(addedIndexesSet, strings.ToLower(index.Name())) } for _, addedIndex := range addedIndexesSet { From 9ce9d5e0b4afd6b20765ff939c668338d6f5ae6c Mon Sep 17 00:00:00 2001 From: Zach Musgrave Date: Mon, 2 Aug 2021 17:14:29 -0700 Subject: [PATCH 4/8] More tests, and fixed most of the deleted table problems in merge --- go/libraries/doltcore/merge/merge.go | 65 +++++++++++++-------- integration-tests/bats/merge.bats | 87 +++++++++++++++++++++++++++- 2 files changed, 127 insertions(+), 25 deletions(-) diff --git a/go/libraries/doltcore/merge/merge.go b/go/libraries/doltcore/merge/merge.go index e9e6591dde..ced880317d 100644 --- a/go/libraries/doltcore/merge/merge.go +++ b/go/libraries/doltcore/merge/merge.go @@ -39,6 +39,7 @@ import ( var ErrFastForward = errors.New("fast forward") var ErrSameTblAddedTwice = errors.New("table with same name added in 2 commits can't be merged") +var ErrTableDeletedAndModified = errors.New("conflict: table with same name deleted and modified ") type Merger struct { root *doltdb.RootValue @@ -104,20 +105,27 @@ func (merger *Merger) MergeTable(ctx context.Context, tblName string, sess *edit } { // short-circuit logic - if ancHasTable && schema.IsKeyless(ancSchema) { - if rootHasTable && mergeHasTable && ancHasTable && rootHash == mergeHash && rootHash == ancHash { - return tbl, &MergeStats{Operation: TableUnmodified}, nil - } - } else { - if rootHasTable && mergeHasTable && rootHash == mergeHash { - return tbl, &MergeStats{Operation: TableUnmodified}, nil - } + + // Nothing changed + if rootHasTable && mergeHasTable && ancHasTable && rootHash == mergeHash && rootHash == ancHash { + return tbl, &MergeStats{Operation: TableUnmodified}, nil } + // Both made identical changes + if rootHasTable && mergeHasTable && rootHash == mergeHash { + return tbl, &MergeStats{Operation: TableUnmodified}, nil + } + + // Changes only in root, table unmodified + if mergeHash == ancHash { + return tbl, &MergeStats{Operation: TableUnmodified}, nil + } + + // One or both added this table if !ancHasTable { if mergeHasTable && rootHasTable { if schema.SchemasAreEqual(rootSchema, mergeSchema) { - // hackity hack + // if both added the same table, pretend it was in the ancestor all along with no data ancSchema, ancTbl = rootSchema, tbl ancRows, _ = types.NewMap(ctx, merger.vrw) } else { @@ -132,8 +140,23 @@ func (merger *Merger) MergeTable(ctx context.Context, tblName string, sess *edit } } - if rootHash == ancHash { + // Deleted in both, fast-forward + if ancHasTable && !rootHasTable && !mergeHasTable { + return nil, &MergeStats{Operation: TableRemoved}, nil + } + + // Deleted in root or in merge, either a conflict (if any changes in other root) or else a fast-forward + if ancHasTable && (!rootHasTable || !mergeHasTable) { + if (mergeHasTable && mergeHash != ancHash) || + (rootHasTable && rootHash != ancHash) { + return nil, nil, ErrTableDeletedAndModified + } // fast-forward + return nil, &MergeStats{Operation: TableRemoved}, nil + } + + // Changes only in merge root, fast-forward + if rootHash == ancHash { ms := MergeStats{Operation: TableModified} if rootHash != mergeHash { ms, err = calcTableMergeStats(ctx, tbl, mergeTbl) @@ -147,9 +170,6 @@ func (merger *Merger) MergeTable(ctx context.Context, tblName string, sess *edit return nil, nil, err } return mergeTbl, &ms, nil - } else if mergeHash == ancHash { - // fast-forward - return tbl, &MergeStats{Operation: TableUnmodified}, nil } } @@ -162,16 +182,6 @@ func (merger *Merger) MergeTable(ctx context.Context, tblName string, sess *edit return nil, nil, schConflicts.AsError() } - rows, err := tbl.GetRowData(ctx) - if err != nil { - return nil, nil, err - } - - mergeRows, err := mergeTbl.GetRowData(ctx) - if err != nil { - return nil, nil, err - } - updatedTbl, err := tbl.UpdateSchema(ctx, postMergeSchema) if err != nil { return nil, nil, err @@ -208,6 +218,15 @@ func (merger *Merger) MergeTable(ctx context.Context, tblName string, sess *edit return nil, nil, err } + rows, err := tbl.GetRowData(ctx) + if err != nil { + return nil, nil, err + } + + mergeRows, err := mergeTbl.GetRowData(ctx) + if err != nil { + return nil, nil, err + } resultTbl, conflicts, stats, err := mergeTableData(ctx, merger.vrw, tblName, postMergeSchema, rows, mergeRows, ancRows, updatedTblEditor, sess) if err != nil { return nil, nil, err diff --git a/integration-tests/bats/merge.bats b/integration-tests/bats/merge.bats index 3ab15533c1..1bb86db05d 100644 --- a/integration-tests/bats/merge.bats +++ b/integration-tests/bats/merge.bats @@ -456,6 +456,8 @@ SQL run dolt merge feature-branch [ "$status" -eq 0 ] + dolt commit -m "merged feature-branch" + run dolt sql -q "show tables" [ "$status" -eq 0 ] [[ ! "$output" =~ "test2" ]] || false @@ -471,7 +473,7 @@ SQL INSERT INTO test2 VALUES (0, 0, 0); INSERT INTO test2 VALUES (1, 1, 1); SQL - dolt add -A && dolt commit -am "add data to test 2" + dolt add -A && dolt commit -am "add data to test2" dolt branch feature-branch dolt sql -q "drop table test2" @@ -492,10 +494,91 @@ SQL dolt checkout master run dolt merge feature-branch - [ "$status" -eq 0 ] + dolt commit -m "merged feature-branch" + run dolt sql -q "show tables" [ "$status" -eq 0 ] [[ ! "$output" =~ "test2" ]] || false } + +@test "merge: merge a branch that edits a deleted table" { + dolt sql << SQL +INSERT INTO test2 VALUES (0, 0, 0); +INSERT INTO test2 VALUES (1, 1, 1); +SQL + dolt add -A && dolt commit -am "add data to test2" + + dolt branch feature-branch + dolt sql -q "drop table test2" + dolt commit -am "drop table test2" + + dolt checkout feature-branch + dolt sql << SQL +INSERT INTO test2 VALUES (2, 2, 2); +SQL + dolt commit -am "add data to test2" + + dolt checkout master + run dolt merge feature-branch + + [ "$status" -eq 1 ] + [[ "$output" =~ "conflict" ]] || false +} + +@test "merge: merge a branch that deletes an edited table" { + dolt sql << SQL +INSERT INTO test2 VALUES (0, 0, 0); +INSERT INTO test2 VALUES (1, 1, 1); +SQL + dolt add -A && dolt commit -am "add data to test2" + + dolt branch feature-branch + dolt sql << SQL +INSERT INTO test2 VALUES (2, 2, 2); +SQL + dolt commit -am "add data to test2" + + dolt checkout feature-branch + dolt sql -q "drop table test2" + dolt commit -am "drop table test2" + + dolt checkout master + run dolt merge feature-branch + + [ "$status" -eq 1 ] + [[ "$output" =~ "conflict" ]] || false +} + +@test "merge: merge a branch that deletes a deleted table" { + dolt sql << SQL +INSERT INTO test2 VALUES (0, 0, 0); +INSERT INTO test2 VALUES (1, 1, 1); +SQL + dolt add -A && dolt commit -am "add data to test2" + + dolt branch feature-branch + dolt sql << SQL +INSERT INTO test2 VALUES (2, 2, 2); +SQL + dolt commit -am "add data to test2" + dolt sql << SQL +drop table test2; +SQL + dolt commit -am "drop test2" + + dolt checkout feature-branch + dolt sql -q "drop table test2" + dolt commit -am "drop table test2" + + dolt checkout master + run dolt merge feature-branch + [ "$status" -eq 0 ] + dolt commit -m "merged feature-branch" + + run dolt sql -q "show tables" + [ "$status" -eq 0 ] + [[ "$output" =~ "test1" ]] || false + [[ ! "$output" =~ "test2" ]] || false +} From 677b7d5cd71162756dc31c8067c5dcf11cb0345c Mon Sep 17 00:00:00 2001 From: Zach Musgrave Date: Mon, 2 Aug 2021 17:25:35 -0700 Subject: [PATCH 5/8] Fixed panic in merging a branch that still contains a table that was deleted in our root --- go/libraries/doltcore/merge/merge.go | 18 +++++++++++------- integration-tests/bats/merge.bats | 5 +++-- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/go/libraries/doltcore/merge/merge.go b/go/libraries/doltcore/merge/merge.go index ced880317d..91f0e1ad93 100644 --- a/go/libraries/doltcore/merge/merge.go +++ b/go/libraries/doltcore/merge/merge.go @@ -116,11 +116,6 @@ func (merger *Merger) MergeTable(ctx context.Context, tblName string, sess *edit return tbl, &MergeStats{Operation: TableUnmodified}, nil } - // Changes only in root, table unmodified - if mergeHash == ancHash { - return tbl, &MergeStats{Operation: TableUnmodified}, nil - } - // One or both added this table if !ancHasTable { if mergeHasTable && rootHasTable { @@ -155,6 +150,11 @@ func (merger *Merger) MergeTable(ctx context.Context, tblName string, sess *edit return nil, &MergeStats{Operation: TableRemoved}, nil } + // Changes only in root, table unmodified + if mergeHash == ancHash { + return tbl, &MergeStats{Operation: TableUnmodified}, nil + } + // Changes only in merge root, fast-forward if rootHash == ancHash { ms := MergeStats{Operation: TableModified} @@ -868,7 +868,6 @@ func MergeRoots(ctx context.Context, ourRoot, theirRoot, ancRoot *doltdb.RootVal // need to validate merges can be done on all tables before starting the actual merges. for _, tblName := range tblNames { mergedTable, stats, err := merger.MergeTable(ctx, tblName, tableEditSession) - if err != nil { return nil, nil, err } @@ -889,6 +888,7 @@ func MergeRoots(ctx context.Context, ourRoot, theirRoot, ancRoot *doltdb.RootVal } else if newRootHasTable, err := newRoot.HasTable(ctx, tblName); err != nil { return nil, nil, err } else if newRootHasTable { + // Merge root deleted this table tblToStats[tblName] = &MergeStats{Operation: TableRemoved} err = tableEditSession.UpdateRoot(ctx, func(ctx context.Context, root *doltdb.RootValue) (*doltdb.RootValue, error) { return root.RemoveTables(ctx, tblName) @@ -901,7 +901,11 @@ func MergeRoots(ctx context.Context, ourRoot, theirRoot, ancRoot *doltdb.RootVal return nil, nil, err } } else { - panic("?") + // This is a deleted table that the merge root still has + if stats.Operation != TableRemoved { + panic("Invalid merge state. This is a bug.") + } + // Nothing to update, our root already has the table deleted } } diff --git a/integration-tests/bats/merge.bats b/integration-tests/bats/merge.bats index 1bb86db05d..ed9a53968a 100644 --- a/integration-tests/bats/merge.bats +++ b/integration-tests/bats/merge.bats @@ -462,9 +462,10 @@ SQL [ "$status" -eq 0 ] [[ ! "$output" =~ "test2" ]] || false - run dolt sql -q "select * from test1" + run dolt sql -q "select * from test1" -r csv [ "$status" -eq 0 ] - [[ "$output" =~ 2 ]] || false + [[ "$output" =~ "1,1,1" ]] || false + [[ "$output" =~ "2,2,2" ]] || false } From 1ab4f68db64d9a5b2516272cc147092d239a3fbe Mon Sep 17 00:00:00 2001 From: Zach Musgrave Date: Mon, 2 Aug 2021 17:31:33 -0700 Subject: [PATCH 6/8] Slightly more robust test --- integration-tests/bats/merge.bats | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/integration-tests/bats/merge.bats b/integration-tests/bats/merge.bats index ed9a53968a..a8b2de61a8 100644 --- a/integration-tests/bats/merge.bats +++ b/integration-tests/bats/merge.bats @@ -502,6 +502,11 @@ SQL run dolt sql -q "show tables" [ "$status" -eq 0 ] [[ ! "$output" =~ "test2" ]] || false + + run dolt sql -q "select * from test1" -r csv + [ "$status" -eq 0 ] + [[ "$output" =~ "1,1,1" ]] || false + [[ "$output" =~ "2,2,2" ]] || false } @test "merge: merge a branch that edits a deleted table" { From 3a674eee6d04a11250c1ec75e4459ee9eafcd808 Mon Sep 17 00:00:00 2001 From: Zach Musgrave Date: Mon, 2 Aug 2021 17:34:39 -0700 Subject: [PATCH 7/8] Moved a function --- go/libraries/doltcore/merge/merge.go | 52 ++++++++++++++-------------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/go/libraries/doltcore/merge/merge.go b/go/libraries/doltcore/merge/merge.go index 91f0e1ad93..0bf9a042f4 100644 --- a/go/libraries/doltcore/merge/merge.go +++ b/go/libraries/doltcore/merge/merge.go @@ -53,32 +53,6 @@ func NewMerger(ctx context.Context, root, mergeRoot, ancRoot *doltdb.RootValue, return &Merger{root, mergeRoot, ancRoot, vrw} } -func getTableInfoFromRoot(ctx context.Context, tblName string, root *doltdb.RootValue) ( - ok bool, - table *doltdb.Table, - sch schema.Schema, - h hash.Hash, - err error, -) { - table, ok, err = root.GetTable(ctx, tblName) - if err != nil { - return false, nil, nil, hash.Hash{}, err - } - - if ok { - h, err = table.HashOf() - if err != nil { - return false, nil, nil, hash.Hash{}, err - } - sch, err = table.GetSchema(ctx) - if err != nil { - return false, nil, nil, hash.Hash{}, err - } - } - - return ok, table, sch, h, nil -} - // MergeTable merges schema and table data for the table tblName. func (merger *Merger) MergeTable(ctx context.Context, tblName string, sess *editor.TableEditSession) (*doltdb.Table, *MergeStats, error) { rootHasTable, tbl, rootSchema, rootHash, err := getTableInfoFromRoot(ctx, tblName, merger.root) @@ -264,6 +238,32 @@ func (merger *Merger) MergeTable(ctx context.Context, tblName string, sess *edit return resultTbl, stats, nil } +func getTableInfoFromRoot(ctx context.Context, tblName string, root *doltdb.RootValue) ( + ok bool, + table *doltdb.Table, + sch schema.Schema, + h hash.Hash, + err error, +) { + table, ok, err = root.GetTable(ctx, tblName) + if err != nil { + return false, nil, nil, hash.Hash{}, err + } + + if ok { + h, err = table.HashOf() + if err != nil { + return false, nil, nil, hash.Hash{}, err + } + sch, err = table.GetSchema(ctx) + if err != nil { + return false, nil, nil, hash.Hash{}, err + } + } + + return ok, table, sch, h, nil +} + func calcTableMergeStats(ctx context.Context, tbl *doltdb.Table, mergeTbl *doltdb.Table) (MergeStats, error) { rows, err := tbl.GetRowData(ctx) From 6667a78ed777b0d409d023939d3a674ed84a68a0 Mon Sep 17 00:00:00 2001 From: Zach Musgrave Date: Tue, 3 Aug 2021 15:05:57 -0700 Subject: [PATCH 8/8] PR feedback --- go/libraries/doltcore/merge/merge.go | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/go/libraries/doltcore/merge/merge.go b/go/libraries/doltcore/merge/merge.go index 0bf9a042f4..1d71bd6f52 100644 --- a/go/libraries/doltcore/merge/merge.go +++ b/go/libraries/doltcore/merge/merge.go @@ -86,7 +86,8 @@ func (merger *Merger) MergeTable(ctx context.Context, tblName string, sess *edit } // Both made identical changes - if rootHasTable && mergeHasTable && rootHash == mergeHash { + // For keyless tables, this counts as a conflict + if rootHasTable && mergeHasTable && rootHash == mergeHash && !schema.IsKeyless(rootSchema) { return tbl, &MergeStats{Operation: TableUnmodified}, nil } @@ -94,8 +95,9 @@ func (merger *Merger) MergeTable(ctx context.Context, tblName string, sess *edit if !ancHasTable { if mergeHasTable && rootHasTable { if schema.SchemasAreEqual(rootSchema, mergeSchema) { - // if both added the same table, pretend it was in the ancestor all along with no data - ancSchema, ancTbl = rootSchema, tbl + // If both added the same table, pretend it was in the ancestor all along with no data + // Don't touch ancHash to avoid triggering other short-circuit logic below + ancHasTable, ancSchema, ancTbl = true, rootSchema, tbl ancRows, _ = types.NewMap(ctx, merger.vrw) } else { return nil, nil, ErrSameTblAddedTwice @@ -851,8 +853,6 @@ func MergeCommits(ctx context.Context, commit, mergeCommit *doltdb.Commit) (*dol } func MergeRoots(ctx context.Context, ourRoot, theirRoot, ancRoot *doltdb.RootValue) (*doltdb.RootValue, map[string]*MergeStats, error) { - merger := NewMerger(ctx, ourRoot, theirRoot, ancRoot, ourRoot.VRW()) - tblNames, err := doltdb.UnionTableNames(ctx, ourRoot, theirRoot) if err != nil { @@ -865,7 +865,10 @@ func MergeRoots(ctx context.Context, ourRoot, theirRoot, ancRoot *doltdb.RootVal tableEditSession := editor.CreateTableEditSession(ourRoot, editor.TableEditSessionProps{ ForeignKeyChecksDisabled: true, }) - // need to validate merges can be done on all tables before starting the actual merges. + + // Merge tables one at a time. This is done based on name, so will work badly for things like table renames. + // TODO: merge based on a more durable table identity that persists across renames + merger := NewMerger(ctx, ourRoot, theirRoot, ancRoot, ourRoot.VRW()) for _, tblName := range tblNames { mergedTable, stats, err := merger.MergeTable(ctx, tblName, tableEditSession) if err != nil { @@ -903,7 +906,7 @@ func MergeRoots(ctx context.Context, ourRoot, theirRoot, ancRoot *doltdb.RootVal } else { // This is a deleted table that the merge root still has if stats.Operation != TableRemoved { - panic("Invalid merge state. This is a bug.") + panic(fmt.Sprintf("Invalid merge state for table %s. This is a bug.", tblName)) } // Nothing to update, our root already has the table deleted }