Start preparing merge-related tests to work with multiple merge backends #827
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
newren
force-pushed
the
merge-test-rename
branch
2 times, most recently
from
88f4897
to
3a2b7e9
Compare
|
/submit |
|
There is an issue in commit 3e3f8e0: |
newren
changed the title
|
/submit |
|
There is an issue in commit 3e3f8e0: |
newren
force-pushed
the
merge-test-rename
branch
from
598a24d
to
38f6573
Compare
|
/submit |
![gitgitgadget-git[bot]](https://avatars.githubusercontent.com/in/46807?s=60&v=4){kind=small}
newren
force-pushed
the
merge-test-rename
branch
from
38f6573
to
dad06d5
Compare
|
libgit2 must have some issue with its rename detection; there are no conflicts with this branch and master, nor even with next or seen. All merge automatically and cleanly at the command line. |
|
/submit |
|
Error: Refusing to submit a patch series that does not merge cleanly. |
|
gitgitgadget: you idiot bot, it does merge cleanly. It's just libgit2 has broken rename detection or something like that making GitHub think it doesn't merge cleanly. grumble, grumble... |
The tests for the merge machinery are spread over several places. Collect them into t64xx for simplicity. Some notes: t60[234]*.sh: Merge tests started in t602*, overgrew bisect and remote tracking tests in t6030, t6040, and t6041, and nearly overtook replace tests in t6050. This made picking out relevant tests that I wanted to run in a tighter loop slightly more annoying for years. t303*.sh: These started out as tests for the 'merge-recursive' toplevel command, but did not restrict to that and had lots of overlap with the underlying merge machinery. t7405, t7613: submodule-specific merge logic started out in submodule.c but was moved to merge-recursive.c in commit 18cfc08 ("submodule.c: move submodule merging to merge-recursive.c", 2018-05-15). Since these tests are about the logic found in the merge machinery, moving these tests to be with the merge tests makes sense. t7607, t7609: Having tests spread all over the place makes it more likely that additional tests related to a certain piece of logic grow in all those other places. Much like t303*.sh, these two tests were about the underlying merge machinery rather than outer levels. Tests that were NOT moved: t76[01]*.sh: Other than the four tests mentioned above, the remaining tests in t76[01]*.sh are related to non-recursive merge strategies, parameter parsing, and other stuff associated with the highlevel builtin/merge.c rather than the recursive merge machinery. t3[45]*.sh: The rebase testcases in t34*.sh also test the merge logic pretty heavily; sometimes changes I make only trigger failures in the rebase tests. The rebase tests are already nicely coupled together, though, and I didn't want to mess that up. Similar comments apply for the cherry-pick tests in t35*.sh. Signed-off-by: Elijah Newren <newren@gmail.com>
The testcase only required that the merge complete without conflict, without specifying what the correct resolution was. Since normalization changed this from a modify/delete to a not-modified/delete, the correct resolution is to have the file be removed at the end. Add a check for this resolution. Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
Apparently I don't know how to count untracked files, and since the
tests in question were marked as test_expect_failure, no one ever
noticed it until now. Correct the count, as these tests clearly create
three untracked files ('out', 'err', and 'file_count').
(I believe this problem arose because earlier incarnations counted lines
via a pipe to 'wc -l'. Reviewers asked that it be replaced by writing
the output to a file and using test_line_count, but when the temporary
output was added to a separate file, the count of untracked files should
have increased.)
Signed-off-by: Elijah Newren <newren@gmail.com>
Commit da1e295 ("t604[236]: do not run setup in separate tests", 2019-10-22) removed approximately half the tests (which were setup-only tests) in t6043 by turning them into functions that the subsequent test would call as their first step. This ensured that any test from this file could be run entirely independently of all the other tests in the file. Unfortunately, the call to the new setup function was missed in two of the test_expect_failure cases. Add them in. Signed-off-by: Elijah Newren <newren@gmail.com>
This test had multiple issues causing it to fail for the wrong
reason(s):
* rename/rename(1to2) conflicts have always left the original source
path present in the working directory and index (at stage 1). Thus,
the triple rename/rename(1to2) should result in 9 unstaged files,
not 6.
* It messed up the three-way content merge for checking the results of
merging for one of the renames, accidentally turning it into a
two-way merge.
* It got the contents of the base files it was using to compare
against wrong, due to an off-by-one error, and overwrite-redirection
('>') instead of append-redirection ('>>').
* It used slightly too-long conflict markers
* It didn't include filenames in the conflict marker hunks (granted,
that was a shortcoming of the merge-recursive backend for rename/add
and rename/rename(2to1) conflicts, but since it's
test_expect_failure anyway we might as well make it expect our
preferred behavior rather than some compromise that we can't yet
reach anyway).
Fix these issues so that a merge backend which correctly handles these
kinds of nested conflicts will pass the test.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
I had long since forgotten the idea behind this test and why it failed, and took a little while to figure it out. To prevent others from having to spend a similar time on it, add an explanation in the comments. However, the reasoning in the explanation makes me question why I considered it a failure at all. I'm not sure if I had a better reason when I originally wrote it, but for now just add commentary about the possible expectations and why it behaves the way it does right now. Signed-off-by: Elijah Newren <newren@gmail.com>
merge-recursive treats an add/add conflict where one of the adds came from a rename as a separate 'rename/add' type of conflict. However, if there is not content conflict after the content merge(s), then the file is not considered to be conflicted. That suggests the conflict type is really just add/add. Other merge engines might choose to print messages to the console that just refer to these as add/add conflicts; accept both types of output. Note: it could help to notify users if the three-way content merge of the rename had content conflicts, because when we then go to two-way merge THAT with the conflicting add we can get nested conflict markers. merge-recursive, unfortunately, doesn't do that, but other merge engines could. Signed-off-by: Elijah Newren <newren@gmail.com>
Much like the last commit accepted 'add/add' and 'rename/add' interchangably, we also want to do the same for 'add/add' and 'rename/rename'. This also allows us to avoid the ambiguity in meaning with 'rename/rename' (is it two separate files renamed to the same location, or one file renamed on both sides but differently)? Signed-off-by: Elijah Newren <newren@gmail.com>
t6425 was very picky about the exact output message produced by a
rename/delete conflict, in a way that just scratches the surface of the
mess that was built into merge-recursive. The idea was that it would
try to find the possible combinations of different conflict types, and
when more than one was present for one path, it would try to provide a
combined message that covered all the cases.
There's a lot to unravel here...
First, there's a basic conflict type known as modify/delete, which is a
content conflict. It occurs when one side deletes a file, but the other
modifies it.
There is also a path conflict known as a rename/delete. This occurs
when one side deletes a path, and the other renames it. This is not a
content conflict, it is a path conflict. It will often occur in
combination with a content conflict, though, namely a modify/delete. As
such, these two were often combined.
Another type of conflict that can exist is a directory/file conflict.
For example, one side adds a new file at some path, and the other side
of history adds a directory at the same path. The path that was "added"
could have been put there by a rename, though. Thus, we have the
possibility of a single path being affected by a modify/delete, a
rename/delete, and a directory/file conflict.
In part, this was a natural by-product of merge-recursive's design.
Since it was doing a four way merge with the contents of the working
tree being the fourth factor it had to consider, it had working tree
handling spread all over the code. It also had directory/file conflict
handling spread everywhere through all the other types of conflicts.
And our testsuite has a huge number of directory/file conflict tests
because trying to get them right required modifying so many different
codepaths. A natural outgrowth of this kind of structure is conflict
messages that combine all the different types that the current codepath
is considering.
However, if we want to make the different conflict types orthogonal and
avoid repeating ourselves and getting very brittle code, then we need to
split the messages from these different conflict types apart. Besides,
trying to determine all possible permutations is a _royal_ mess. The
code to handle the rename/delete/directory/file conflict output is
already somewhat hard to parse, and is somewhat brittle. But if we
really wanted to go that route, then we'd have to have special handling
for the following types of combinations:
* rename/add/delete:
on side of history that didn't rename the given file, remove the file
instead and place an unrelated file in the way of the rename
* rename/rename(2to1)/mode conflict/delete/delete:
two different files, one executable and the other not, are renamed
to the same location, each side deletes the source file that the
other side renames
* rename/rename(1to2)/add/add:
file renamed differently on each side of history, with each side
placing an unrelated file in the way of the other
* rename/rename(1to2)/content conflict/file location/(D/F)/(D/F)/:
both sides modify a file in conflicting way, both rename that file
but to different paths, one side renames the directory which the
other side had renamed that file into causing it to possibly need a
transitive rename, and each side puts a directory in the way of the
other's path.
Let's back away from this path of insanity, and allow the different
types of conflicts to be handled by separate pieces of non-repeated code
by allowing the conflict messages to be split into their separate types.
(If multiple conflict types affect a single path, the conflict messages
can be printed sequentially.) Start this path with a simple change:
modify this test to be more flexible and accept the output either merge
backend (recursive or the new ort) will produce.
Signed-off-by: Elijah Newren <newren@gmail.com>
newren
force-pushed
the
merge-test-rename
branch
from
dad06d5
to
0c8dcbf
Compare
|
The osx-clang failures look like infra failures to me; a bunch of git-p4 test failures which is highly suspect given that I didn't modify any product code -- I only modified tests, and none of the git-p4 ones. So I'm going to ignore that one. |
|
/submit |
|
This branch is now known as |
|
This patch series was integrated into seen via 8f25967. |
|
This patch series was integrated into seen via 89a4c4b. |
|
This patch series was integrated into seen via bb33add. |
|
This patch series was integrated into next via eab9523. |
|
This patch series was integrated into seen via b0d7b27. |
|
This patch series was integrated into seen via 610a70e. |
|
This patch series was integrated into seen via 36d225c. |
|
This patch series was integrated into next via 36d225c. |
|
This patch series was integrated into master via 36d225c. |
|
Closed via 36d225c. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This series starts preparing the merge related tests to work with multiple backends: merge-recursive or merge-ort. Further work will be coming.
Changes since v2:
The greater flexibility patches at the end of the series also previews some of the design changes being brought by merge-ort.
Merges cleanly with next and seen, despite the pile of testcase renames.