Fix bug in merge

[johnnyaug]

When merging from a branch that made no changes, Graveler would create a new commit with an empty metarange ID, causing all the objects to be deleted on the merge destination.
Instead, we should return a "no changes" error.

Test changes

Changed the behavior of the unit tests to take into account the metarange IDs, which until now were ignored.

This required changes to the test cases themselves: before this change, metaranges always got fixed IDs. After this change, IDs are generated according to the metarange's content. In some cases this causes the ID of the base metarange to be equal to the ID of the source or dest metaranges.

In order for them to get different IDs, I added some content to the end of some of the bases. Since all addition are only to the merge bases, this doesn't affect the merge results.

[nopcoder]

When there are no changes to the source, we want the destination to be the result.
Think that the no change should be returned when both point to base.

[nopcoder]

can be []testRange this way we don't need special construction for all the test code.

type testMetaRange []testRange

[arielshaqed]

When there are no changes to the source, we want the destination to be the result. Think that the no change should be returned when both point to base.

I think that this is correct. So: we should start by agreeing on the desired test results, and only then proceed to the actual fix.

Note that we can determine the correct result by ignoring the "base === {source, dest}" optimization. I mean, suppose I took a case where base and source (or dest) have precisely the same diffs, but one range boundary in base is different. Then the metarange IDs are different and the full algorithm runs. But the result should be the same.

For instance, we could consider adding a test-only flag that turns off these optimizations. Now we can run the tests with and without the flag -- the tests should pass regardless!

Or you could add a field to testMetaRange that gets appended to its ID. Now you can copy the tests in disagreement and change that field on base. We should still get the same result!

[arielshaqed]

Thanks! Still want the test with and without changing the base metarange ID, to ensure we have consistent behaviour. But this is not blocking for a bug like this.

[guy-har]

Adding Here This comment just to explain what exactly happened, because it may be a bit confusing

When trying to introduce a merge with no changes an error should* be returned
There was a bug that returned an empty string and no error, which then set the metarangeID of the merge commit to be an empty string

flowchart TD
  A[mid:1] -- Main --> B[mid:2] --> C[mid:""];
  A  -- Branch 1 No Change --> D[mid:1];
  D -- No Change --> C;

Loading

• An error shouldn't really be returned, It was historically returned when commit and merge were implemented together, This behavior should probably change, but this shouldn't be resolved in this PR

[nopcoder]

Approve as keeping the previous behaviour. Opening an issue to address the behaviour that will allow us to merge when no changes from base.