Skip to content

Conversation

herrerog
Copy link
Contributor

When merging trees containing multiple empty files, make sure a rename
is not detected between each empty files.

For example
Ancestor has files:

  • foo.c with content
  • bar.c empty

Ours has:

  • foo.c with content
  • boo.c empty

Theirs has:

  • foo.c with other content
  • bar.c with content

merge_trees() will incorrectly apply the content of theirs's bar.c in
ours's boo.c thinking bar.c has been renamed to boo.c.
This happens because both are empty and their sha1 are the same. Thus
merge_diff_mark_similarity_exact() incorrectly mark it as renamed.

When merging trees containing multiple empty files, make sure a rename
is not detected between each empty files.

For example
Ancestor has files:
 * foo.c with content
 * bar.c empty

Ours has:
 * foo.c with content
 * boo.c empty

Theirs has:
 * foo.c with other content
 * bar.c with content

merge_trees() will incorrectly apply the content of theirs's 'bar.c' in
ours's boo.c' thinking 'bar.c' has been renamed to 'boo.c'.
This happens because both are empty and their sha1 are the same. Thus
merge_diff_mark_similarity_exact() incorrectly mark it as renamed.

Signed-off-by: Gregory Herrero <gregory.herrero@oracle.com>
Add a boolean option to merge_trivial() so that it can use theirs
parent instead of merge base as ancestor. This will be needed in the
following commit.

Signed-off-by: Gregory Herrero <gregory.herrero@oracle.com>
This test follow "merge: fix incorrect rename detection for empty
files." commit.

Signed-off-by: Gregory Herrero <gregory.herrero@oracle.com>
Don't munge the "trivial" tests, those are specifically about the
"trivial" resolutions for git's tree merge. (For example, adding a file
in a new branch and making no changes in the HEAD branch is something
that can be handled _trivially_.)

For tests of rename functionality, put them in the trees::rename tests.
@ethomson
Copy link
Member

👋 Thanks for the fix - sorry it's taken so long to come back to it. I did make one change, which was just to move the test out of the merge::trivial class and into merge::renames. The rationale is that this covers rename functionality and is not related to the trivial resolution steps that git itself defines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants