From 71d681521242105616c442839a8bd506f6d2c19e Mon Sep 17 00:00:00 2001 From: Martin Dobias Date: Wed, 10 Nov 2021 12:57:05 +0100 Subject: [PATCH] Fix handling of rebase when editors change the same value (fixes #152) When the final value (of a given row in a given column) would end up the same after two concurrent edits, geodiff would still produce conflicts, confusing users why would they appear. This commit adds handling of that case. --- geodiff/src/geodiffrebase.cpp | 51 +++++--- geodiff/tests/test_concurrent_commits.cpp | 120 ++++++++++++++++++ .../rebase_conflict/case1a-rebased.diff | Bin 0 -> 51 bytes .../testdata/rebase_conflict/case1a.diff | Bin 0 -> 51 bytes .../testdata/rebase_conflict/case1b.diff | Bin 0 -> 47 bytes .../rebase_conflict/case2a-rebased.diff | 0 .../testdata/rebase_conflict/case2a.diff | Bin 0 -> 67 bytes .../testdata/rebase_conflict/case2b.diff | Bin 0 -> 67 bytes .../rebase_conflict/case3a-rebased.conflicts | 23 ++++ .../rebase_conflict/case3a-rebased.diff | Bin 0 -> 67 bytes .../testdata/rebase_conflict/case3a.diff | Bin 0 -> 66 bytes .../testdata/rebase_conflict/case3b.diff | Bin 0 -> 66 bytes .../rebase_conflict/case4a-rebased.conflicts | 17 +++ .../rebase_conflict/case4a-rebased.diff | Bin 0 -> 51 bytes .../testdata/rebase_conflict/case4a.diff | Bin 0 -> 66 bytes .../testdata/rebase_conflict/case4b.diff | Bin 0 -> 66 bytes 16 files changed, 194 insertions(+), 17 deletions(-) create mode 100644 geodiff/tests/testdata/rebase_conflict/case1a-rebased.diff create mode 100644 geodiff/tests/testdata/rebase_conflict/case1a.diff create mode 100644 geodiff/tests/testdata/rebase_conflict/case1b.diff create mode 100644 geodiff/tests/testdata/rebase_conflict/case2a-rebased.diff create mode 100644 geodiff/tests/testdata/rebase_conflict/case2a.diff create mode 100644 geodiff/tests/testdata/rebase_conflict/case2b.diff create mode 100644 geodiff/tests/testdata/rebase_conflict/case3a-rebased.conflicts create mode 100644 geodiff/tests/testdata/rebase_conflict/case3a-rebased.diff create mode 100644 geodiff/tests/testdata/rebase_conflict/case3a.diff create mode 100644 geodiff/tests/testdata/rebase_conflict/case3b.diff create mode 100644 geodiff/tests/testdata/rebase_conflict/case4a-rebased.conflicts create mode 100644 geodiff/tests/testdata/rebase_conflict/case4a-rebased.diff create mode 100644 geodiff/tests/testdata/rebase_conflict/case4a.diff create mode 100644 geodiff/tests/testdata/rebase_conflict/case4b.diff diff --git a/geodiff/src/geodiffrebase.cpp b/geodiff/src/geodiffrebase.cpp index fc64147f..01621f83 100644 --- a/geodiff/src/geodiffrebase.cpp +++ b/geodiff/src/geodiffrebase.cpp @@ -463,31 +463,46 @@ bool _handle_update( const ChangesetEntry &entry, const RebaseMapping &mapping, ConflictFeature conflictFeature( pk, entry.table->name ); + bool entryHasChanges = false; for ( size_t i = 0; i < numColumns; i++ ) { Value patchedVal = patchedVals[i]; if ( patchedVal.type() != Value::TypeUndefined && entry.newValues[i].type() != Value::TypeUndefined ) { - // we have edit conflict here: both "old" changeset and the "new" changeset modify the same - // column of the same row. Rebased changeset will get the "old" value updated to the new (patched) - // value of the older changeset - outEntry.oldValues[i] = patchedVal; - _addConflictItem( conflictFeature, i, entry.oldValues[i], patchedVal, entry.newValues[i] ); + if ( patchedVal == entry.newValues[i] ) + { + // both "old" and "new" changeset modify the column's value to the same value - that + // means that in our rebased changeset there's no further change and there's no conflict + outEntry.oldValues[i].setUndefined(); + outEntry.newValues[i].setUndefined(); + } + else + { + // we have edit conflict here: both "old" changeset and the "new" changeset modify the same + // column of the same row. Rebased changeset will get the "old" value updated to the new (patched) + // value of the older changeset + outEntry.oldValues[i] = patchedVal; + outEntry.newValues[i] = entry.newValues[i]; + entryHasChanges = true; + _addConflictItem( conflictFeature, i, entry.oldValues[i], patchedVal, entry.newValues[i] ); + } } else { - // otherwise the value is same for both patched and this, so use base value + // the "new" changeset stays as is without modifications outEntry.oldValues[i] = entry.oldValues[i]; + outEntry.newValues[i] = entry.newValues[i]; + // if a column is pkey, it would have "new" value undefined in the entry and that's not an actual change + if ( entry.newValues[i].type() != Value::TypeUndefined ) + entryHasChanges = true; } } - outEntry.newValues = entry.newValues; - if ( conflictFeature.isValid() ) { conflicts.push_back( conflictFeature ); } - return true; + return entryHasChanges; } int _prepare_new_changeset( ChangesetReader &reader, const std::string &changesetNew, @@ -546,16 +561,18 @@ int _prepare_new_changeset( ChangesetReader &reader, const std::string &changese for ( auto it : tableDefinitions ) { - writer.beginTable( it.second ); - auto chit = tableChanges.find( it.first ); - if ( chit != tableChanges.end() ) + if ( chit == tableChanges.end() ) + continue; + + const std::vector &changes = chit->second; + if ( changes.empty() ) + continue; + + writer.beginTable( it.second ); + for ( const ChangesetEntry &entry : changes ) { - const std::vector &changes = chit->second; - for ( const ChangesetEntry &entry : changes ) - { - writer.writeEntry( entry ); - } + writer.writeEntry( entry ); } } diff --git a/geodiff/tests/test_concurrent_commits.cpp b/geodiff/tests/test_concurrent_commits.cpp index 1a40ea1a..8a9563b4 100644 --- a/geodiff/tests/test_concurrent_commits.cpp +++ b/geodiff/tests/test_concurrent_commits.cpp @@ -129,6 +129,55 @@ bool _test( return equals( patchedAB_2, expected_patchedAB, ignore_timestamp_change ); } +bool _test_createRebasedChangesetEx( + const std::string &testName, + const std::string &testBaseDb, + const std::string &diffOur, + const std::string &diffTheir, + const std::string &expectedDiffOurRebased, + const std::string &expectedConflictFile +) +{ + makedir( pathjoin( tmpdir(), testName ) ); + + std::string diffOurRebased = pathjoin( tmpdir(), testName, "rebased.diff" ); + std::string conflictFile = pathjoin( tmpdir(), testName, "conflicts.json" ); + int res = GEODIFF_createRebasedChangesetEx( "sqlite", "", testBaseDb.c_str(), diffOur.c_str(), diffTheir.c_str(), diffOurRebased.c_str(), conflictFile.c_str() ); + if ( res != GEODIFF_SUCCESS ) + { + std::cerr << "err GEODIFF_createRebasedChangesetEx" << std::endl; + return false; + } + + // check rebased diff equality + if ( !compareDiffsByContent( diffOurRebased, expectedDiffOurRebased ) ) + { + std::cerr << "err rebased diff is not equal to the expected diff" << std::endl; + return false; + } + + // check conflict equality + if ( !expectedConflictFile.empty() ) + { + if ( !fileContentEquals( conflictFile, expectedConflictFile ) ) + { + std::cerr << "err conflict file is not equal to the expected conflict file" << std::endl; + return false; + } + } + else + { + // it is expected that no conflict file would be created... + if ( fileExists( conflictFile ) ) + { + std::cerr << "err conflict file should not be created, but it got created" << std::endl; + return false; + } + } + + return true; +} + bool _test_expect_not_implemented( const std::string &baseX, const std::string &testname, @@ -449,6 +498,77 @@ TEST( ConcurrentCommitsSqlite3Test, test_conflict ) ASSERT_TRUE( GEODIFF_applyChangeset( baseB.c_str(), changesetbaseA.c_str() ) != GEODIFF_SUCCESS ); } +TEST( ConcurrentCommitsSqlite3Test, test_rebase_conflict ) +{ + // Check various scenarios where a single row of a table is updated by different users + // and verify that rebased diffs and conflict files are produced correctly. + // All tests are done on a single row of table "simple" at fid=2 + + // CASE 1: change of one column, each diff different column: + // input: + // - A: column "name": "feature2" -> "feature222" + // - B: column "rating": 2 -> 222 + // output: + // - A rebased on top of B: column "name": "feature2" -> "feature222" + // - no conflict file + + bool res1 = _test_createRebasedChangesetEx( "test_rebase_conflict_case1", + pathjoin( testdir(), "base.gpkg" ), + pathjoin( testdir(), "rebase_conflict", "case1a.diff" ), + pathjoin( testdir(), "rebase_conflict", "case1b.diff" ), + pathjoin( testdir(), "rebase_conflict", "case1a-rebased.diff" ), + std::string() ); // no conflict file + ASSERT_TRUE( res1 ); + + // CASE 2: change of two columns, both to the same value + // input: + // - A: column "name": "feature2" -> "feature222" and column "rating": 2 -> 222 + // - B: column "name": "feature2" -> "feature222" and column "rating": 2 -> 222 + // output: + // - A rebased on top of B: empty changeset + // - no conflict file + + bool res2 = _test_createRebasedChangesetEx( "test_rebase_conflict_case2", + pathjoin( testdir(), "base.gpkg" ), + pathjoin( testdir(), "rebase_conflict", "case2a.diff" ), + pathjoin( testdir(), "rebase_conflict", "case2b.diff" ), + pathjoin( testdir(), "rebase_conflict", "case2a-rebased.diff" ), + std::string() ); // no conflict file + ASSERT_TRUE( res2 ); + + // CASE 3: change of two columns, both to different values + // input: + // - A: column "name": "feature2" -> "feature2A" and column "rating": 2 -> 20 + // - B: column "name": "feature2" -> "feature2B" and column "rating": 2 -> 21 + // output: + // - A rebased on top of B: column "name": "feature2B" -> "feature2A" and column "rating": 21 -> 20 + // - conflict file with an entry for both "name" and "rating" columns + + bool res3 = _test_createRebasedChangesetEx( "test_rebase_conflict_case3", + pathjoin( testdir(), "base.gpkg" ), + pathjoin( testdir(), "rebase_conflict", "case3a.diff" ), + pathjoin( testdir(), "rebase_conflict", "case3b.diff" ), + pathjoin( testdir(), "rebase_conflict", "case3a-rebased.diff" ), + pathjoin( testdir(), "rebase_conflict", "case3a-rebased.conflicts" ) ); + ASSERT_TRUE( res3 ); + + // CASE 4: change of two columns, one to the same value, one to other value + // input: + // - A: column "name": "feature2" -> "feature2A" and column "rating": 2 -> 222 + // - B: column "name": "feature2" -> "feature2B" and column "rating": 2 -> 222 + // output: + // - A rebased on top of B: column "name": "feature2B" -> "feature2A" + // - conflict file with an entry for column "name" + + bool res4 = _test_createRebasedChangesetEx( "test_rebase_conflict_case4", + pathjoin( testdir(), "base.gpkg" ), + pathjoin( testdir(), "rebase_conflict", "case4a.diff" ), + pathjoin( testdir(), "rebase_conflict", "case4b.diff" ), + pathjoin( testdir(), "rebase_conflict", "case4a-rebased.diff" ), + pathjoin( testdir(), "rebase_conflict", "case4a-rebased.conflicts" ) ); + ASSERT_TRUE( res4 ); +} + int main( int argc, char **argv ) { testing::InitGoogleTest( &argc, argv ); diff --git a/geodiff/tests/testdata/rebase_conflict/case1a-rebased.diff b/geodiff/tests/testdata/rebase_conflict/case1a-rebased.diff new file mode 100644 index 0000000000000000000000000000000000000000..229ce21f77420867ba62a6c9fdb64888bb681635 GIT binary patch literal 51 tcmWGxVPs%nD9+3+$Vp`oX8>_PfQf;bBP}(tq_im22*_pTf^m$D7y#Vk3AO+L literal 0 HcmV?d00001 diff --git a/geodiff/tests/testdata/rebase_conflict/case1a.diff b/geodiff/tests/testdata/rebase_conflict/case1a.diff new file mode 100644 index 0000000000000000000000000000000000000000..229ce21f77420867ba62a6c9fdb64888bb681635 GIT binary patch literal 51 tcmWGxVPs%nD9+3+$Vp`oX8>_PfQf;bBP}(tq_im22*_pTf^m$D7y#Vk3AO+L literal 0 HcmV?d00001 diff --git a/geodiff/tests/testdata/rebase_conflict/case1b.diff b/geodiff/tests/testdata/rebase_conflict/case1b.diff new file mode 100644 index 0000000000000000000000000000000000000000..084cbbcc4ad298090873a9750f4628cde655239d GIT binary patch literal 47 gcmWGxVPs%nD9+3+$Vp`oX8>_PfQbQ_31{B}0Cv&?-~a#s literal 0 HcmV?d00001 diff --git a/geodiff/tests/testdata/rebase_conflict/case2a-rebased.diff b/geodiff/tests/testdata/rebase_conflict/case2a-rebased.diff new file mode 100644 index 00000000..e69de29b diff --git a/geodiff/tests/testdata/rebase_conflict/case2a.diff b/geodiff/tests/testdata/rebase_conflict/case2a.diff new file mode 100644 index 0000000000000000000000000000000000000000..8e9c68cf9eb1ae6199a89eee2d8e37bb0ee52669 GIT binary patch literal 67 wcmWGxVPs%nD9+3+$Vp`oX8>_PfQf;bBP}(tq_im22qDJ6%mouPf(zdR09_FZm;e9( literal 0 HcmV?d00001 diff --git a/geodiff/tests/testdata/rebase_conflict/case2b.diff b/geodiff/tests/testdata/rebase_conflict/case2b.diff new file mode 100644 index 0000000000000000000000000000000000000000..8e9c68cf9eb1ae6199a89eee2d8e37bb0ee52669 GIT binary patch literal 67 wcmWGxVPs%nD9+3+$Vp`oX8>_PfQf;bBP}(tq_im22qDJ6%mouPf(zdR09_FZm;e9( literal 0 HcmV?d00001 diff --git a/geodiff/tests/testdata/rebase_conflict/case3a-rebased.conflicts b/geodiff/tests/testdata/rebase_conflict/case3a-rebased.conflicts new file mode 100644 index 00000000..6aabeeac --- /dev/null +++ b/geodiff/tests/testdata/rebase_conflict/case3a-rebased.conflicts @@ -0,0 +1,23 @@ +{ + "geodiff": [ + { + "table": "simple", + "type": "conflict", + "fid": "2", + "changes": [ + { + "column": 2, + "base": "feature2", + "old": "feature2B", + "new": "feature2A" + }, + { + "column": 3, + "base": 2, + "old": 21, + "new": 20 + } + ] + } + ] +} \ No newline at end of file diff --git a/geodiff/tests/testdata/rebase_conflict/case3a-rebased.diff b/geodiff/tests/testdata/rebase_conflict/case3a-rebased.diff new file mode 100644 index 0000000000000000000000000000000000000000..70fd33f76e378eabca8247084408fc7c176e1fb9 GIT binary patch literal 67 wcmWGxVPs%nD9+3+$Vp`oX8>_PfQf;bGc7f-q_im2$O$GW%77&52on_n0AZ9000000 literal 0 HcmV?d00001 diff --git a/geodiff/tests/testdata/rebase_conflict/case3a.diff b/geodiff/tests/testdata/rebase_conflict/case3a.diff new file mode 100644 index 0000000000000000000000000000000000000000..f592de60863950f7e2331019e5f3f436fd7e5697 GIT binary patch literal 66 wcmWGxVPs%nD9+3+$Vp`oX8>_PfQf;bBP}(tq_im22qDJ6%n1{8go%m(08vi~ssI20 literal 0 HcmV?d00001 diff --git a/geodiff/tests/testdata/rebase_conflict/case3b.diff b/geodiff/tests/testdata/rebase_conflict/case3b.diff new file mode 100644 index 0000000000000000000000000000000000000000..f7209f166b989bbe51c278adc9b7853bef10d4ef GIT binary patch literal 66 wcmWGxVPs%nD9+3+$Vp`oX8>_PfQf;bBP}(tq_im22qDJ6%n1{8f{BU(08v^AtN;K2 literal 0 HcmV?d00001 diff --git a/geodiff/tests/testdata/rebase_conflict/case4a-rebased.conflicts b/geodiff/tests/testdata/rebase_conflict/case4a-rebased.conflicts new file mode 100644 index 00000000..567ae26c --- /dev/null +++ b/geodiff/tests/testdata/rebase_conflict/case4a-rebased.conflicts @@ -0,0 +1,17 @@ +{ + "geodiff": [ + { + "table": "simple", + "type": "conflict", + "fid": "2", + "changes": [ + { + "column": 2, + "base": "feature2", + "old": "feature2B", + "new": "feature2A" + } + ] + } + ] +} \ No newline at end of file diff --git a/geodiff/tests/testdata/rebase_conflict/case4a-rebased.diff b/geodiff/tests/testdata/rebase_conflict/case4a-rebased.diff new file mode 100644 index 0000000000000000000000000000000000000000..101f0413ccf2a929dd3be0661a569cacee99b12f GIT binary patch literal 51 rcmWGxVPs%nD9+3+$Vp`oX8>_PfQf;bGc7f-q_im2$O*_t@EjQc-EIlh literal 0 HcmV?d00001 diff --git a/geodiff/tests/testdata/rebase_conflict/case4a.diff b/geodiff/tests/testdata/rebase_conflict/case4a.diff new file mode 100644 index 0000000000000000000000000000000000000000..426d6b847fea8038c0fad5a39944462410d4b28d GIT binary patch literal 66 wcmWGxVPs%nD9+3+$Vp`oX8>_PfQf;bBP}(tq_im22qDJ6%n1{8go)k*08$wWbN~PV literal 0 HcmV?d00001 diff --git a/geodiff/tests/testdata/rebase_conflict/case4b.diff b/geodiff/tests/testdata/rebase_conflict/case4b.diff new file mode 100644 index 0000000000000000000000000000000000000000..3ccc88179160f9cc93fae39af40b3e1f3198d884 GIT binary patch literal 66 wcmWGxVPs%nD9+3+$Vp`oX8>_PfQf;bBP}(tq_im22qDJ6%n1{8f{ES(08%3gbpQYW literal 0 HcmV?d00001