* [RFC PATCH 0/5] Improve path collision conflict resolutions
@ 2018-03-05 17:11 Elijah Newren
2018-03-05 17:11 ` [RFC PATCH 1/5] Add testcases for improved file collision conflict handling Elijah Newren
` (5 more replies)
0 siblings, 6 replies; 9+ messages in thread
From: Elijah Newren @ 2018-03-05 17:11 UTC (permalink / raw)
To: git; +Cc: Somebody, Elijah Newren
This series improves conflict resolutions for conflict types that involve
two (possibly entirely unrelated) files colliding at the same path. These
conflict types are:
* add/add
* rename/add
* rename/rename(2to1)
Improving these conflict types has some subtle (though significant)
performance ramifications, but for now, I am just concentrating on
providing the user with better information for their conflict resolution.
Performance considerations will be addressed in a future patch series.
Before mentioning the improvements, it may be worth noting that these three
types are actually more similar than might at first be apparent: for the
cases involving renames, any trace of the rename-source path is deleted
from both the index and the working copy (modulo a small technicality
mentioned in patch 2), leaving just the question of how to represent two
colliding files in both the index and the working copy for all three cases.
There are three important changes this patch series introduces:
1) Consolidating the code for these three types of conflict resolutions
into a single function that all three code paths can call.
2) Doing content merges for a rename before looking at the path collision
between a rename and some other file. In particular (in what I most
suspect others might have an objection to from this series), record
that content-merged file -- which may have conflict markers -- in the
index at the appropriate higher stage.
3) Smarter behavior for recording the conflict in the working tree: first
checking whether the two colliding files are similar, and then based
on that, deciding whether to handle the path collision via a two-way
merge of the different files or to instead record the two different
files at separate temporary paths.
In more detail:
1)
The consolidation seems fairly self-explanatory, but it had a bigger effect
than you'd expect: it made it clear that the rename/add conflict resolution
is broken in multiple ways, and it also made it easier to reason about what
_should_ be done for rename/add conflicts (something I had struggled with
when I looked at that particular conflict type in the past). See patch 3
for more details.
Sidenote: I was kind of surprised that rename/add could have been broken
for this long, unnoticed. Does no one ever hit that conflict in real life?
It looks like we did not have very good test coverage for rename/add
conflicts; a brief search seems to show that we only had a few testcases
triggering that conflict type, and all of them were added by me. Patch 1
tries to address the testcase problem by adding some tests that try to
check the index and working copy more strictly for all three conflict
types.
2)
Previously, rename/rename(2to1) conflict resolution for the colliding path
would just accept the index changes made by unpack_trees(), meaning that
each of the higher order stages in the index for the path collision would
implicitly ignore any changes to each renamed file from the other side of
history. Since, as noted above, all traces of the rename-source path were
removed from both the index and the working tree, this meant that the index
was missing information about changes to such files. If the user tried to
resolve the conflict using the index rather than the working copy, they
would end up with a silent loss of changes.
I "fixed" this by doing the three-way content merge for each renamed-file,
and then recorded THAT in the index at either stage 2 or 3 as appropriate.
Since that merge might have conflict markers, that could mean recording in
the index a file with conflict markers as though it were a given side.
(See patch 2 for a more detailed explanation.) I figure this might be the
most controversial change I made. I can think of a few alternatives, but I
liked all of them less. Opinions?
This change did not require any significant changes to the testsuite; the
difference between the old and new behavior was essentially untested.
(rename/add was even worse: not recording _any_ higher order stages in the
index, and thus partially hiding the fact that the path was involved in a
conflict at all.)
3)
Given the similarity between the conflict types, the big question for
handling the conflict in the working tree was whether the two colliding
files should be two-way merged and recorded in place (as add/add does,
which seems to be preferable if the two files are similar), or whether the
two files should be recorded into separate files (as rename/add and
rename/rename(2to1) do, which seems to be preferable if the two files are
dissimilar). The code handling the different types of conflicts appear to
have been written with different assumptions about whether the colliding
files would be similar.
But, rather than make an assumption about whether the two files will be
similar, why not just check and then make the best choice based on that?
Thus, this code makes use of estimate_similarity(), and uses that to decide
whether to do a two-way content merge or writing unrelated files out to
differently named temporary files.
This logical change did require changing one to two dozen testcases in the
testsuite; I think this is more logical behavior and that the testcases
were toy examples utilized to test other things, but maybe someone else has
an argument for why add/add conflicts should always be two-way merged
regardless of file dissimilarity?
Other notes:
This series builds on en/rename-directory-detection; there are too many
conflicts to resolve if I tried to just base on master.
Elijah Newren (5):
Add testcases for improved file collision conflict handling
merge-recursive: new function for better colliding conflict
resolutions
merge-recursive: fix rename/add conflict handling
merge-recursive: improve handling for rename/rename(2to1) conflicts
merge-recursive: improve handling for add/add conflicts
diff.h | 4 +
diffcore-rename.c | 6 +-
merge-recursive.c | 383 +++++++++++++++++++++++------------
t/t2023-checkout-m.sh | 2 +-
t/t3418-rebase-continue.sh | 27 ++-
t/t3504-cherry-pick-rerere.sh | 19 +-
t/t4200-rerere.sh | 12 +-
t/t6020-merge-df.sh | 4 +-
t/t6024-recursive-merge.sh | 35 ++--
t/t6025-merge-symlinks.sh | 9 +-
t/t6031-merge-filemode.sh | 4 +-
t/t6036-recursive-corner-cases.sh | 19 +-
t/t6042-merge-rename-corner-cases.sh | 212 ++++++++++++++++++-
t/t6043-merge-rename-directories.sh | 15 +-
t/t7060-wtstatus.sh | 1 +
t/t7064-wtstatus-pv2.sh | 4 +-
t/t7506-status-submodule.sh | 11 +-
t/t7610-mergetool.sh | 28 +--
18 files changed, 588 insertions(+), 207 deletions(-)
--
2.16.0.41.g7fdc8a0834
^ permalink raw reply [flat|nested] 9+ messages in thread
* [RFC PATCH 1/5] Add testcases for improved file collision conflict handling
2018-03-05 17:11 [RFC PATCH 0/5] Improve path collision conflict resolutions Elijah Newren
@ 2018-03-05 17:11 ` Elijah Newren
2018-03-08 12:25 ` SZEDER Gábor
2018-03-05 17:11 ` [RFC PATCH 2/5] merge-recursive: new function for better colliding conflict resolutions Elijah Newren
` (4 subsequent siblings)
5 siblings, 1 reply; 9+ messages in thread
From: Elijah Newren @ 2018-03-05 17:11 UTC (permalink / raw)
To: git; +Cc: Somebody, Elijah Newren
Adds testcases dealing with file collisions for the following types of
conflicts:
* add/add
* rename/add
* rename/rename(2to1)
These tests include expectations for proposed smarter behavior which has
not yet been implemented and thus are currently expected to fail.
Subsequent commits will correct that and explain the new behavior.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
t/t6042-merge-rename-corner-cases.sh | 220 +++++++++++++++++++++++++++++++++++
1 file changed, 220 insertions(+)
diff --git a/t/t6042-merge-rename-corner-cases.sh b/t/t6042-merge-rename-corner-cases.sh
index 411550d2b6..a6c151ef95 100755
--- a/t/t6042-merge-rename-corner-cases.sh
+++ b/t/t6042-merge-rename-corner-cases.sh
@@ -575,4 +575,224 @@ test_expect_success 'rename/rename/add-dest merge still knows about conflicting
test ! -f c
'
+test_conflicts_with_adds_and_renames() {
+ test $1 != 0 && side1=rename || side1=add
+ test $2 != 0 && side2=rename || side2=add
+
+ # Setup:
+ # L
+ # / \
+ # master ?
+ # \ /
+ # R
+ #
+ # Where:
+ # Both L and R have files named 'three-unrelated' and
+ # 'three-related' which collide (i.e. 4 files colliding at two
+ # pathnames). Each of the colliding files could have been
+ # involved in a rename, in which case there was a file named
+ # 'one-[un]related' or 'two-[un]related' that was modified on the
+ # opposite side of history and renamed into the collision on this
+ # side of history.
+ #
+ # Questions for both sets of collisions:
+ # 1) The index should contain both a stage 2 and stage 3 entry
+ # for the colliding file. Does it?
+ # 2) When renames are involved, the content merges are clean, so
+ # the index should reflect the content merges, not merely the
+ # version of the colliding file from the prior commit. Does
+ # it?
+ #
+ # Questions for three-unrelated:
+ # 3) There should be files in the worktree named
+ # 'three-unrelated~HEAD' and 'three-unrelated~R^0' with the
+ # (content-merged) version of 'three-unrelated' from the
+ # appropriate side of the merge. Are they present?
+ # 4) There should be no file named 'three-unrelated' in the
+ # working tree. That'd make it too likely that users would
+ # use it instead of carefully looking at both
+ # three-unrelated~HEAD and three-unrelated~R^0. Is it
+ # correctly missing?
+ #
+ # Questions for three-related:
+ # 3) There should be a file in the worktree named three-related
+ # containing the two-way merged contents of the content-merged
+ # versions of three-related from each of the two colliding
+ # files. Is it present?
+ # 4) There should not be any three-related~* files in the working
+ # tree.
+ test_expect_success "setup simple $side1/$side2 conflict" '
+ test_create_repo simple_${side1}_${side2} &&
+ (
+ cd simple_${side1}_${side2} &&
+
+ # Create a simple file with 10 lines
+ ten="0 1 2 3 4 5 6 7 8 9" &&
+ for i in $ten
+ do
+ echo line $i in a sample file
+ done >unrelated1_v1 &&
+ # Create a 2nd version of same file with one more line
+ cat unrelated1_v1 >unrelated1_v2 &&
+ echo another line >>unrelated1_v2 &&
+
+ # Create an unrelated simple file with 10 lines
+ for i in $ten
+ do
+ echo line $i in another sample file
+ done >unrelated2_v1 &&
+ # Create a 2nd version of same file with one more line
+ cat unrelated2_v1 >unrelated2_v2 &&
+ echo another line >>unrelated2_v2 &&
+
+ # Create some related files now
+ for i in $ten
+ do
+ echo Random base content line $i
+ done >related1_v1 &&
+ cp -a related1_v1 related1_v2 &&
+ echo modification >>related1_v2 &&
+
+ cp -a related1_v1 related2_v1 &&
+ echo more stuff >>related2_v1 &&
+ cp -a related2_v1 related2_v2 &&
+ echo yet more stuff >>related2_v2 &&
+
+ # Use a tag to record both these files for simple
+ # access, and clean out these untracked files
+ git tag unrelated1_v1 `git hash-object -w unrelated1_v1` &&
+ git tag unrelated1_v2 `git hash-object -w unrelated1_v2` &&
+ git tag unrelated2_v1 `git hash-object -w unrelated2_v1` &&
+ git tag unrelated2_v2 `git hash-object -w unrelated2_v2` &&
+ git tag related1_v1 `git hash-object -w related1_v1` &&
+ git tag related1_v2 `git hash-object -w related1_v2` &&
+ git tag related2_v1 `git hash-object -w related2_v1` &&
+ git tag related2_v2 `git hash-object -w related2_v2` &&
+ git clean -f &&
+
+ # Setup merge-base, consisting of files named "one-*"
+ # and "two-*" if renames were involved.
+ touch irrelevant_file &&
+ git add irrelevant_file &&
+ if [ $side1 == "rename" ]; then
+ git show unrelated1_v1 >one-unrelated &&
+ git add one-unrelated
+ git show related1_v1 >one-related &&
+ git add one-related
+ fi &&
+ if [ $side2 == "rename" ]; then
+ git show unrelated2_v1 >two-unrelated &&
+ git add two-unrelated
+ git show related2_v1 >two-related &&
+ git add two-related
+ fi &&
+ test_tick && git commit -m initial &&
+
+ git branch L &&
+ git branch R &&
+
+ # Handle the left side
+ git checkout L &&
+ if [ $side1 == "rename" ]; then
+ git mv one-unrelated three-unrelated
+ git mv one-related three-related
+ else
+ git show unrelated1_v2 >three-unrelated &&
+ git add three-unrelated
+ git show related1_v2 >three-related &&
+ git add three-related
+ fi &&
+ if [ $side2 == "rename" ]; then
+ git show unrelated2_v2 >two-unrelated &&
+ git add two-unrelated
+ git show related2_v2 >two-related &&
+ git add two-related
+ fi &&
+ test_tick && git commit -m L &&
+
+ # Handle the right side
+ git checkout R &&
+ if [ $side1 == "rename" ]; then
+ git show unrelated1_v2 >one-unrelated &&
+ git add one-unrelated
+ git show related1_v2 >one-related &&
+ git add one-related
+ fi &&
+ if [ $side2 == "rename" ]; then
+ git mv two-unrelated three-unrelated
+ git mv two-related three-related
+ else
+ git show unrelated2_v2 >three-unrelated &&
+ git add three-unrelated
+ git show related2_v2 >three-related &&
+ git add three-related
+ fi &&
+ test_tick && git commit -m R
+ )
+ '
+
+ test_expect_failure "check simple $side1/$side2 conflict" '
+ (
+ cd simple_${side1}_${side2} &&
+
+ git checkout L^0 &&
+
+ # Merge must fail; there is a conflict
+ test_must_fail git merge -s recursive R^0 &&
+
+ # Make sure the index has the right number of entries
+ git ls-files -s >out &&
+ test_line_count = 5 out &&
+ git ls-files -u >out &&
+ test_line_count = 4 out &&
+
+ # Nothing should have touched irrelevant_file
+ git rev-parse >actual \
+ :0:irrelevant_file \
+ :2:three-unrelated :3:three-unrelated \
+ :2:three-related :3:three-related &&
+ git rev-parse >expected \
+ master:irrelevant_file \
+ unrelated1_v2 unrelated2_v2 \
+ related1_v2 related2_v2 &&
+
+ # Ensure we have the correct number of untracked files
+ git ls-files -o >out &&
+ test_line_count = 5 out &&
+
+ # Make sure each file (with merging if rename
+ # involved) is present in the working tree for the
+ # user to work with.
+ git hash-object >actual \
+ three-unrelated~HEAD three-unrelated~R^0 &&
+ git rev-parse >expected \
+ unrelated1_v2 unrelated2_v2 &&
+
+ # "three-unrelated" should not exist because there is
+ # no reason to give preference to either
+ # three-unrelated~HEAD or three-unrelated~R^0
+ test_path_is_missing three-unrelated &&
+
+ # Make sure we have the correct merged contents for
+ # three-related
+ git show related1_v1 >expected &&
+ cat <<EOF >>expected &&
+<<<<<<< HEAD
+modification
+=======
+more stuff
+yet more stuff
+>>>>>>> R^0
+EOF
+
+ test_cmp expected three-related
+ )
+ '
+}
+
+test_conflicts_with_adds_and_renames 1 1
+test_conflicts_with_adds_and_renames 1 0
+test_conflicts_with_adds_and_renames 0 1
+test_conflicts_with_adds_and_renames 0 0
+
test_done
--
2.16.0.41.g6a66043158
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [RFC PATCH 2/5] merge-recursive: new function for better colliding conflict resolutions
2018-03-05 17:11 [RFC PATCH 0/5] Improve path collision conflict resolutions Elijah Newren
2018-03-05 17:11 ` [RFC PATCH 1/5] Add testcases for improved file collision conflict handling Elijah Newren
@ 2018-03-05 17:11 ` Elijah Newren
2018-03-05 17:11 ` [RFC PATCH 3/5] merge-recursive: fix rename/add conflict handling Elijah Newren
` (3 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Elijah Newren @ 2018-03-05 17:11 UTC (permalink / raw)
To: git; +Cc: Somebody, Elijah Newren
There are three conflict types that represent two (possibly entirely
unrelated) files colliding at the same location:
* add/add
* rename/add
* rename/rename(2to1)
These three conflict types already share more similarity than might be
immediately apparent from their description: (1) the handling of the
rename variants already involves removing any entries from the index
corresponding to the original file names[*], thus only leaving entries
in the index for the colliding path; (2) likewise, any trace of the
original file name in the working tree is also removed. So, in all
three cases we're left with how to represent two colliding files in both
the index and the working copy.
[*] Technically, this isn't quite true because rename/rename(2to1)
conflicts in the recursive (o->call_depth > 0) case do an "unrename"
since about seven years ago. But even in that case, Junio felt
compelled to explain that my decision to "unrename" wasn't necessarily
the only or right answer -- search for "Comment from Junio" in t6036 for
details.
My initial motivation for looking at these three conflict types was that
if the handling of these three conflict types is the same, at least in
the limited set of cases where a renamed file is unmodified on the side
of history where the file is not renamed, then a significant performance
improvement for rename detection during merges is possible. However,
while that served as motivation to look at these three types of
conflicts, the actual goal of this new function is to try to improve the
handling for all three cases, not to merely make them the same as each
other in that special circumstance.
=== Handling the working tree ===
The previous behavior for these conflict types in regards to the
working tree (assuming the file collision occurs at 'foo') was:
* add/add does a two-way merge of the two files and records it as 'foo'.
* rename/rename(2to1) records the two different files into two new
uniquely named files (foo~HEAD and foo~$MERGE), while removing 'foo'
from the working tree.
* rename/add records the two different files into two different
locations, recording the add at foo~$SIDE and, oddly, recording
the rename at foo (why is the rename more important than the add?)
So, the question for what to write to the working tree boils down to
whether the two colliding files should be two-way merged and recorded in
place, or recorded into separate files. If the files are similar enough,
the two-way merge is probably preferable, but if they're not similar,
recording as separate files is probably preferable. (The same logic that
applies for the working directory here would also apply to the recursive
case, i.e. the o->call_depth > 0 case, as well.) The code handling the
different types of conflicts appear to have been written with different
assumptions about whether the colliding files would be similar.
But, rather than make an assumption about whether the two files will be
similar, why not just check? If we simply call estimate_similarity(),
we can two-way merge the files if they are similar, and otherwise record
the two files at different locations.
=== Handling of the index ===
For a typical rename, unpack_trees() would set up the index in the
following fashion:
old_path new_path
stage1: 5ca1ab1e 00000000
stage2: f005ba11 00000000
stage3: 00000000 b0a710ad
And merge-recursive would rewrite this to
new_path
stage1: 5ca1ab1e
stage2: f005ba11
stage3: b0a710ad
Removing old_path from the index means the user won't have to `git rm
old_path` manually every time a renamed path has a content conflict.
It also means they can use `git checkout [--ours|--theirs|--conflict|-m]
new_path`, `git diff [--ours|--theirs]` and various other commands that
would be difficult otherwise.
This strategy becomes a problem when we have a rename/add or
rename/rename(2to1) conflict, however, because then we have only three
slots to store blob sha1s and we need either four or six. Previously,
this was handled by continuing to delete old_path from the index, and
just outright ignoring any blob shas from old_path. That had the
downside of deleting any trace of changes made to old_path on the other
side of history. This function instead does a three-way content merge of
the renamed file, and stores the blob sha1 for that at either stage2 or
stage3 for new_path (depending on which side the rename came from). That
has the advantage of bringing information about changes on both sides and
still allows for easy resolution (no need to git rm old_path, etc.), but
does have the downside that if the content merge had conflict markers,
then what we store in the index is the sha1 of a blob with conflict
markers. While that is a downside, it seems less problematic than the
downsides of any obvious alternatives, and certainly makes more sense
than the previous handling.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
diff.h | 4 ++
diffcore-rename.c | 6 +--
merge-recursive.c | 124 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 131 insertions(+), 3 deletions(-)
diff --git a/diff.h b/diff.h
index 7cf276f077..810944247c 100644
--- a/diff.h
+++ b/diff.h
@@ -442,4 +442,8 @@ extern void print_stat_summary(FILE *fp, int files,
int insertions, int deletions);
extern void setup_diff_pager(struct diff_options *);
+extern int estimate_similarity(struct diff_filespec *src,
+ struct diff_filespec *dst,
+ int minimum_score);
+
#endif /* DIFF_H */
diff --git a/diffcore-rename.c b/diffcore-rename.c
index 245e999fe5..0f4388a9e0 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -127,9 +127,9 @@ struct diff_score {
short name_score;
};
-static int estimate_similarity(struct diff_filespec *src,
- struct diff_filespec *dst,
- int minimum_score)
+int estimate_similarity(struct diff_filespec *src,
+ struct diff_filespec *dst,
+ int minimum_score)
{
/* src points at a file that existed in the original tree (or
* optionally a file in the destination tree) and dst points
diff --git a/merge-recursive.c b/merge-recursive.c
index 5f42c677d5..c54b918dc8 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1290,6 +1290,130 @@ static struct diff_filespec *filespec_from_entry(struct diff_filespec *target,
return target;
}
+#if 0 // #if-0-ing avoids unused function warning; will make live in next commit
+static int handle_file_collision(struct merge_options *o,
+ const char *collide_path,
+ const char *prev_path1,
+ const char *prev_path2,
+ const char *branch1, const char *branch2,
+ const struct object_id *a_oid,
+ unsigned int a_mode,
+ const struct object_id *b_oid,
+ unsigned int b_mode,
+ unsigned int conflict_markers_already_present)
+{
+ struct merge_file_info mfi;
+ struct diff_filespec null, a, b;
+ int minimum_score;
+ char *new_path1, *new_path2;
+
+ /* Remove rename sources if rename/add or rename/rename(2to1) */
+ if (prev_path1)
+ remove_file(o, 1, prev_path1,
+ o->call_depth || would_lose_untracked(prev_path1));
+ if (prev_path2)
+ remove_file(o, 1, prev_path2,
+ o->call_depth || would_lose_untracked(prev_path2));
+
+ /*
+ * Remove the collision path, if it wouldn't cause dirty contents
+ * or an untracked file to get lost. We'll either overwrite with
+ * merged contents, or just write out to differently named files.
+ */
+ if (was_dirty(o, collide_path))
+ output(o, 1, _("Refusing to lose dirty file at %s"),
+ collide_path);
+ else if (would_lose_untracked(collide_path))
+ /*
+ * Only way we get here is if both renames were from
+ * a directory rename AND user had an untracked file
+ * at the location where both files end up after the
+ * two directory renames. See testcase 10d of t6043.
+ */
+ output(o, 1, _("Refusing to lose untracked file at "
+ "%s, even though it's in the way."),
+ collide_path);
+ else
+ /*
+ * FIXME: It's possible that neither of the two files have
+ * conflict markers already present, and that they're
+ * identical, and that the current working copy happens to
+ * match, in which case we are unnecessarily touching the
+ * working tree file. It's not a likely enough scenario
+ * that I want to code up the checks for it and a better
+ * fix is available if we restructure how unpack_trees()
+ * and merge-recursive interoperate anyway, so punting for
+ * now...
+ */
+ remove_file(o, 0, collide_path, 0);
+
+ /* Store things in diff_filespecs for functions that need it */
+ memset(&a, 0, sizeof(struct diff_filespec));
+ memset(&b, 0, sizeof(struct diff_filespec));
+ null.path = a.path = b.path = (char *)collide_path;
+ oidcpy(&null.oid, &null_oid);
+ null.mode = 0;
+ oidcpy(&a.oid, a_oid);
+ a.mode = a_mode;
+ a.oid_valid = 1;
+ oidcpy(&b.oid, b_oid);
+ b.mode = b_mode;
+ b.oid_valid = 1;
+
+ /*
+ * If the colliding files are similar enough, we can simply merge
+ * them. But we don't want to merge files that have conflict
+ * markers in them already, because nested conflict markers are
+ * too confusing.
+ */
+ minimum_score = o->rename_score ? o->rename_score
+ : DEFAULT_RENAME_SCORE;
+ if (!conflict_markers_already_present && minimum_score <=
+ estimate_similarity(&a, &b, o->rename_score)) {
+ if (merge_file_1(o, &null, &a, &b, branch1, branch2, &mfi))
+ return -1;
+ if (update_file(o, mfi.clean, &mfi.oid, mfi.mode, collide_path))
+ return -1;
+ if (!mfi.clean && !o->call_depth &&
+ update_stages(o, collide_path, NULL, &a, &b))
+ return -1;
+ return mfi.clean;
+ }
+
+ /*
+ * Put the colliding files into different paths, and record the
+ * updated sha1sums in the index
+ */
+ new_path1 = (o->call_depth && prev_path1) ? strdup(prev_path1) :
+ unique_path(o, collide_path, branch1);
+ new_path2 = (o->call_depth && prev_path2) ? strdup(prev_path2) :
+ unique_path(o, collide_path, branch2);
+ output(o, 1, _("Renaming collisions at %s to %s and %s instead"),
+ collide_path, new_path1, new_path2);
+
+ if (update_file(o, 0, a_oid, a_mode, new_path1))
+ return -1;
+ if (update_file(o, 0, b_oid, b_mode, new_path2))
+ return -1;
+
+ /* Update index too, making sure to get stage order correct. */
+ if (!o->call_depth) {
+ if (o->branch1 == branch1) {
+ if (update_stages(o, collide_path, NULL, &a, &b))
+ return -1;
+ } else {
+ if (update_stages(o, collide_path, NULL, &b, &a))
+ return -1;
+ }
+ }
+
+ free(new_path2);
+ free(new_path1);
+
+ return 0; /* not clean */
+}
+#endif
+
static int handle_file(struct merge_options *o,
struct diff_filespec *rename,
int stage,
--
2.16.0.41.g6a66043158
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [RFC PATCH 3/5] merge-recursive: fix rename/add conflict handling
2018-03-05 17:11 [RFC PATCH 0/5] Improve path collision conflict resolutions Elijah Newren
2018-03-05 17:11 ` [RFC PATCH 1/5] Add testcases for improved file collision conflict handling Elijah Newren
2018-03-05 17:11 ` [RFC PATCH 2/5] merge-recursive: new function for better colliding conflict resolutions Elijah Newren
@ 2018-03-05 17:11 ` Elijah Newren
2018-03-05 17:11 ` [RFC PATCH 4/5] merge-recursive: improve handling for rename/rename(2to1) conflicts Elijah Newren
` (2 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Elijah Newren @ 2018-03-05 17:11 UTC (permalink / raw)
To: git; +Cc: Somebody, Elijah Newren
This makes the rename/add conflict handling make use of the new
handle_file_collision() function, which fixes several bugs and improves
things for the rename/add case significantly. Previously, rename/add
would:
* Not leave any higher order stage entries in the index, making it
appear as if there were no conflict.
* Would place the rename file at the colliding path, and move the
added file elsewhere, which combined with the lack of higher order
stage entries felt really odd. It's not clear to me why the
rename should take precedence over the add; if one should be moved
out of the way, they both probably should.
* In the recursive case, it would do a two way merge of the added
file and the version of the renamed file on the renamed side,
completely excluding modifications to the renamed file on the
unrenamed side of history.
Using the new handle_file_collision() fixes all of these issues, and
adds smarts to allow two-way merge OR move colliding files to separate
paths depending on the similarity of the colliding files.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
merge-recursive.c | 137 +++++++++++++++++++++++++++-----------
t/t6036-recursive-corner-cases.sh | 19 +++---
2 files changed, 108 insertions(+), 48 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index c54b918dc8..403c0006dc 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -181,6 +181,7 @@ static int oid_eq(const struct object_id *a, const struct object_id *b)
enum rename_type {
RENAME_NORMAL = 0,
RENAME_DIR,
+ RENAME_ADD,
RENAME_DELETE,
RENAME_ONE_FILE_TO_ONE,
RENAME_ONE_FILE_TO_TWO,
@@ -223,6 +224,7 @@ static inline void setup_rename_conflict_info(enum rename_type rename_type,
struct stage_data *src_entry1,
struct stage_data *src_entry2)
{
+ int ostage1, ostage2;
struct rename_conflict_info *ci = xcalloc(1, sizeof(struct rename_conflict_info));
ci->rename_type = rename_type;
ci->pair1 = pair1;
@@ -240,18 +242,22 @@ static inline void setup_rename_conflict_info(enum rename_type rename_type,
dst_entry2->rename_conflict_info = ci;
}
- if (rename_type == RENAME_TWO_FILES_TO_ONE) {
- /*
- * For each rename, there could have been
- * modifications on the side of history where that
- * file was not renamed.
- */
- int ostage1 = o->branch1 == branch1 ? 3 : 2;
- int ostage2 = ostage1 ^ 1;
+ /*
+ * For each rename, there could have been
+ * modifications on the side of history where that
+ * file was not renamed.
+ */
+ if (rename_type == RENAME_ADD ||
+ rename_type == RENAME_TWO_FILES_TO_ONE) {
+ ostage1 = o->branch1 == branch1 ? 3 : 2;
ci->ren1_other.path = pair1->one->path;
oidcpy(&ci->ren1_other.oid, &src_entry1->stages[ostage1].oid);
ci->ren1_other.mode = src_entry1->stages[ostage1].mode;
+ }
+
+ if (rename_type == RENAME_TWO_FILES_TO_ONE) {
+ ostage2 = ostage1 ^ 1;
ci->ren2_other.path = pair2->one->path;
oidcpy(&ci->ren2_other.oid, &src_entry2->stages[ostage2].oid);
@@ -1119,6 +1125,18 @@ static int merge_file_special_markers(struct merge_options *o,
char *side2 = NULL;
int ret;
+ if (o->branch1 != branch1) {
+ /*
+ * It's weird getting a reverse merge with HEAD on the bottom
+ * and the other branch on the top. Fix that.
+ */
+ return merge_file_special_markers(o,
+ one, b, a,
+ branch2, filename2,
+ branch1, filename1,
+ mfi);
+ }
+
if (filename1)
side1 = xstrfmt("%s:%s", branch1, filename1);
if (filename2)
@@ -1290,7 +1308,6 @@ static struct diff_filespec *filespec_from_entry(struct diff_filespec *target,
return target;
}
-#if 0 // #if-0-ing avoids unused function warning; will make live in next commit
static int handle_file_collision(struct merge_options *o,
const char *collide_path,
const char *prev_path1,
@@ -1307,6 +1324,21 @@ static int handle_file_collision(struct merge_options *o,
int minimum_score;
char *new_path1, *new_path2;
+ /*
+ * It's easiest to get the correct things into stage 2 and 3, and
+ * to make sure that the content merge puts HEAD before the other
+ * branch if we just ensure that branch1 == o->branch1. So, simply
+ * flip arguments around if we don't have that.
+ */
+ if (branch1 != o->branch1) {
+ return handle_file_collision(o, collide_path,
+ prev_path2, prev_path1,
+ branch2, branch1,
+ b_oid, b_mode,
+ a_oid, a_mode,
+ conflict_markers_already_present);
+ }
+
/* Remove rename sources if rename/add or rename/rename(2to1) */
if (prev_path1)
remove_file(o, 1, prev_path1,
@@ -1412,7 +1444,36 @@ static int handle_file_collision(struct merge_options *o,
return 0; /* not clean */
}
-#endif
+
+static int conflict_rename_add(struct merge_options *o,
+ struct rename_conflict_info *ci)
+{
+ /* a was renamed to c, and a separate c was added. */
+ struct diff_filespec *a = ci->pair1->one;
+ struct diff_filespec *c = ci->pair1->two;
+ char *path = c->path;
+ struct merge_file_info mfi;
+
+ int other_stage = (ci->branch1 == o->branch1 ? 3 : 2);
+
+ output(o, 1, _("CONFLICT (rename/add): "
+ "Rename %s->%s in %s. Added %s in %s"),
+ a->path, c->path, ci->branch1,
+ c->path, ci->branch2);
+
+ if (merge_file_special_markers(o, a, c, &ci->ren1_other,
+ o->branch1, path,
+ o->branch2, ci->ren1_other.path, &mfi))
+ return -1;
+
+ return handle_file_collision(o,
+ c->path, a->path, NULL,
+ ci->branch1, ci->branch2,
+ &mfi.oid, mfi.mode,
+ &ci->dst_entry1->stages[other_stage].oid,
+ ci->dst_entry1->stages[other_stage].mode,
+ !mfi.clean);
+}
static int handle_file(struct merge_options *o,
struct diff_filespec *rename,
@@ -2582,36 +2643,23 @@ static int process_renames(struct merge_options *o,
0 /* update_wd */))
clean_merge = -1;
} else if (!oid_eq(&dst_other.oid, &null_oid)) {
- clean_merge = 0;
- try_merge = 1;
- output(o, 1, _("CONFLICT (rename/add): Rename %s->%s in %s. "
- "%s added in %s"),
- ren1_src, ren1_dst, branch1,
- ren1_dst, branch2);
- if (o->call_depth) {
- struct merge_file_info mfi;
- if (merge_file_one(o, ren1_dst, &null_oid, 0,
- &ren1->pair->two->oid,
- ren1->pair->two->mode,
- &dst_other.oid,
- dst_other.mode,
- branch1, branch2, &mfi)) {
- clean_merge = -1;
- goto cleanup_and_return;
- }
- output(o, 1, _("Adding merged %s"), ren1_dst);
- if (update_file(o, 0, &mfi.oid,
- mfi.mode, ren1_dst))
- clean_merge = -1;
- try_merge = 0;
- } else {
- char *new_path = unique_path(o, ren1_dst, branch2);
- output(o, 1, _("Adding as %s instead"), new_path);
- if (update_file(o, 0, &dst_other.oid,
- dst_other.mode, new_path))
- clean_merge = -1;
- free(new_path);
- }
+ /*
+ * Probably not a clean merge, but it's
+ * premature to set clean_merge to 0 here,
+ * because if the rename merges cleanly and
+ * the merge exactly matches the newly added
+ * file, then the merge will be clean.
+ */
+ setup_rename_conflict_info(RENAME_ADD,
+ ren1->pair,
+ NULL,
+ branch1,
+ branch2,
+ ren1->dst_entry,
+ NULL,
+ o,
+ ren1->src_entry,
+ NULL);
} else
try_merge = 1;
@@ -3004,6 +3052,15 @@ static int process_entry(struct merge_options *o,
conflict_info->branch2))
clean_merge = -1;
break;
+ case RENAME_ADD:
+ /*
+ * Probably unclean merge, but if the renamed file
+ * merges cleanly and the result can then be
+ * two-way merged cleanly with the added file, I
+ * guess it's a clean merge?
+ */
+ clean_merge = conflict_rename_add(o, conflict_info);
+ break;
case RENAME_DELETE:
clean_merge = 0;
if (conflict_rename_delete(o,
diff --git a/t/t6036-recursive-corner-cases.sh b/t/t6036-recursive-corner-cases.sh
index 18aa88b5c0..09accbc62a 100755
--- a/t/t6036-recursive-corner-cases.sh
+++ b/t/t6036-recursive-corner-cases.sh
@@ -168,7 +168,7 @@ test_expect_success 'setup differently handled merges of rename/add conflict' '
git branch B &&
git checkout -b C &&
echo 10 >>a &&
- echo "other content" >>new_a &&
+ printf "0\n1\n2\n3\n4\n5\n6\n7\nfoobar" >new_a &&
git add a new_a &&
test_tick && git commit -m C &&
@@ -179,13 +179,13 @@ test_expect_success 'setup differently handled merges of rename/add conflict' '
git checkout B^0 &&
test_must_fail git merge C &&
git clean -f &&
+ git add new_a &&
test_tick && git commit -m D &&
git tag D &&
git checkout C^0 &&
test_must_fail git merge B &&
- rm new_a~HEAD new_a &&
- printf "Incorrectly merged content" >>new_a &&
+ printf "0\n1\n2\n3\n4\n5\n6\n7\nbad merge" >new_a &&
git add -u &&
test_tick && git commit -m E &&
git tag E
@@ -204,15 +204,18 @@ test_expect_success 'git detects differently handled merges conflict' '
test $(git rev-parse :2:new_a) = $(git rev-parse D:new_a) &&
test $(git rev-parse :3:new_a) = $(git rev-parse E:new_a) &&
- git cat-file -p B:new_a >>merged &&
+ # Since A:a == B:new_a, the three-way merge of A:a, B:new_a, and
+ # C:a is just C:a. Then we do a two-way merge of that with
+ # C:new_a.
+ git cat-file -p C:a >>merged &&
git cat-file -p C:new_a >>merge-me &&
>empty &&
test_must_fail git merge-file \
- -L "Temporary merge branch 2" \
- -L "" \
-L "Temporary merge branch 1" \
- merged empty merge-me &&
- sed -e "s/^\([<=>]\)/\1\1\1/" merged >merged-internal &&
+ -L "" \
+ -L "Temporary merge branch 2" \
+ merge-me empty merged &&
+ sed -e "s/^\([<=>]\)/\1\1\1/" merge-me >merged-internal &&
test $(git rev-parse :1:new_a) = $(git hash-object merged-internal)
'
--
2.16.0.41.g6a66043158
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [RFC PATCH 4/5] merge-recursive: improve handling for rename/rename(2to1) conflicts
2018-03-05 17:11 [RFC PATCH 0/5] Improve path collision conflict resolutions Elijah Newren
` (2 preceding siblings ...)
2018-03-05 17:11 ` [RFC PATCH 3/5] merge-recursive: fix rename/add conflict handling Elijah Newren
@ 2018-03-05 17:11 ` Elijah Newren
2018-03-05 17:11 ` [RFC PATCH 5/5] merge-recursive: improve handling for add/add conflicts Elijah Newren
2018-03-12 18:19 ` [RFC PATCH 0/5] Improve path collision conflict resolutions Elijah Newren
5 siblings, 0 replies; 9+ messages in thread
From: Elijah Newren @ 2018-03-05 17:11 UTC (permalink / raw)
To: git; +Cc: Somebody, Elijah Newren
This makes the rename/rename(2to1) conflicts use the new
handle_file_collision() function. Since that function was based
originally on the rename/rename(2to1) handling code, the main
differences here are in what was added. In particular:
* If the two colliding files are similar, instead of being stored
at collide_path~HEAD and collide_path~MERGE, the files are two-way
merged and recorded at collide_path.
* Instead of recording the version of the renamed file that existed
on the renamed side in the index (thus ignoring any changes that
were made to the file on the side of history without the rename),
we do a three-way content merge on the renamed path, then store
that at either stage 2 or stage 3.
* Note that if either of the three-way content merges done for each
rename have conflicts, we do NOT try to estimate the similarity of
the resulting two files and just automatically consider them to be
dissimilar. This is done to avoid foisting conflicts-of-conflicts
on the user.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
merge-recursive.c | 101 +++++------------------------------
t/t6042-merge-rename-corner-cases.sh | 2 +-
2 files changed, 14 insertions(+), 89 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index 403c0006dc..96f0e9cee2 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -657,27 +657,6 @@ static int update_stages(struct merge_options *opt, const char *path,
return 0;
}
-static int update_stages_for_stage_data(struct merge_options *opt,
- const char *path,
- const struct stage_data *stage_data)
-{
- struct diff_filespec o, a, b;
-
- o.mode = stage_data->stages[1].mode;
- oidcpy(&o.oid, &stage_data->stages[1].oid);
-
- a.mode = stage_data->stages[2].mode;
- oidcpy(&a.oid, &stage_data->stages[2].oid);
-
- b.mode = stage_data->stages[3].mode;
- oidcpy(&b.oid, &stage_data->stages[3].oid);
-
- return update_stages(opt, path,
- is_null_oid(&o.oid) ? NULL : &o,
- is_null_oid(&a.oid) ? NULL : &a,
- is_null_oid(&b.oid) ? NULL : &b);
-}
-
static void update_entry(struct stage_data *entry,
struct diff_filespec *o,
struct diff_filespec *a,
@@ -1615,7 +1594,6 @@ static int conflict_rename_rename_2to1(struct merge_options *o,
char *path = c1->path; /* == c2->path */
struct merge_file_info mfi_c1;
struct merge_file_info mfi_c2;
- int ret;
output(o, 1, _("CONFLICT (rename/rename): "
"Rename %s->%s in %s. "
@@ -1623,9 +1601,6 @@ static int conflict_rename_rename_2to1(struct merge_options *o,
a->path, c1->path, ci->branch1,
b->path, c2->path, ci->branch2);
- remove_file(o, 1, a->path, o->call_depth || would_lose_untracked(a->path));
- remove_file(o, 1, b->path, o->call_depth || would_lose_untracked(b->path));
-
if (merge_file_special_markers(o, a, c1, &ci->ren1_other,
o->branch1, c1->path,
o->branch2, ci->ren1_other.path, &mfi_c1) ||
@@ -1634,66 +1609,11 @@ static int conflict_rename_rename_2to1(struct merge_options *o,
o->branch2, c2->path, &mfi_c2))
return -1;
- if (o->call_depth) {
- /*
- * If mfi_c1.clean && mfi_c2.clean, then it might make
- * sense to do a two-way merge of those results. But, I
- * think in all cases, it makes sense to have the virtual
- * merge base just undo the renames; they can be detected
- * again later for the non-recursive merge.
- */
- remove_file(o, 0, path, 0);
- ret = update_file(o, 0, &mfi_c1.oid, mfi_c1.mode, a->path);
- if (!ret)
- ret = update_file(o, 0, &mfi_c2.oid, mfi_c2.mode,
- b->path);
- } else {
- char *new_path1 = unique_path(o, path, ci->branch1);
- char *new_path2 = unique_path(o, path, ci->branch2);
- output(o, 1, _("Renaming %s to %s and %s to %s instead"),
- a->path, new_path1, b->path, new_path2);
- if (was_dirty(o, path))
- output(o, 1, _("Refusing to lose dirty file at %s"),
- path);
- else if (would_lose_untracked(path))
- /*
- * Only way we get here is if both renames were from
- * a directory rename AND user had an untracked file
- * at the location where both files end up after the
- * two directory renames. See testcase 10d of t6043.
- */
- output(o, 1, _("Refusing to lose untracked file at "
- "%s, even though it's in the way."),
- path);
- else
- remove_file(o, 0, path, 0);
- ret = update_file(o, 0, &mfi_c1.oid, mfi_c1.mode, new_path1);
- if (!ret)
- ret = update_file(o, 0, &mfi_c2.oid, mfi_c2.mode,
- new_path2);
- /*
- * unpack_trees() actually populates the index for us for
- * "normal" rename/rename(2to1) situtations so that the
- * correct entries are at the higher stages, which would
- * make the call below to update_stages_for_stage_data
- * unnecessary. However, if either of the renames came
- * from a directory rename, then unpack_trees() will not
- * have gotten the right data loaded into the index, so we
- * need to do so now. (While it'd be tempting to move this
- * call to update_stages_for_stage_data() to
- * apply_directory_rename_modifications(), that would break
- * our intermediate calls to would_lose_untracked() since
- * those rely on the current in-memory index. See also the
- * big "NOTE" in update_stages()).
- */
- if (update_stages_for_stage_data(o, path, ci->dst_entry1))
- ret = -1;
-
- free(new_path2);
- free(new_path1);
- }
-
- return ret;
+ return handle_file_collision(o, path, a->path, b->path,
+ ci->branch1, ci->branch2,
+ &mfi_c1.oid, mfi_c1.mode,
+ &mfi_c2.oid, mfi_c2.mode,
+ !mfi_c1.clean || !mfi_c2.clean);
}
/*
@@ -3075,9 +2995,14 @@ static int process_entry(struct merge_options *o,
clean_merge = -1;
break;
case RENAME_TWO_FILES_TO_ONE:
- clean_merge = 0;
- if (conflict_rename_rename_2to1(o, conflict_info))
- clean_merge = -1;
+ /*
+ * Probably unclean merge, but if the two renamed
+ * files merge cleanly and the two resulting files
+ * can then be two-way merged cleanly, I guess it's
+ * a clean merge?
+ */
+ clean_merge = conflict_rename_rename_2to1(o,
+ conflict_info);
break;
default:
entry->processed = 0;
diff --git a/t/t6042-merge-rename-corner-cases.sh b/t/t6042-merge-rename-corner-cases.sh
index a6c151ef95..cf5ea5a0f9 100755
--- a/t/t6042-merge-rename-corner-cases.sh
+++ b/t/t6042-merge-rename-corner-cases.sh
@@ -358,7 +358,7 @@ test_expect_success 'setup rename/rename (2to1) + modify/modify' '
git init &&
printf "1\n2\n3\n4\n5\n" >a &&
- printf "5\n4\n3\n2\n1\n" >b &&
+ printf "9\n8\n7\n6\n5\n" >b &&
git add a b &&
git commit -m A &&
git tag A &&
--
2.16.0.41.g6a66043158
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [RFC PATCH 5/5] merge-recursive: improve handling for add/add conflicts
2018-03-05 17:11 [RFC PATCH 0/5] Improve path collision conflict resolutions Elijah Newren
` (3 preceding siblings ...)
2018-03-05 17:11 ` [RFC PATCH 4/5] merge-recursive: improve handling for rename/rename(2to1) conflicts Elijah Newren
@ 2018-03-05 17:11 ` Elijah Newren
2018-03-12 18:19 ` [RFC PATCH 0/5] Improve path collision conflict resolutions Elijah Newren
5 siblings, 0 replies; 9+ messages in thread
From: Elijah Newren @ 2018-03-05 17:11 UTC (permalink / raw)
To: git; +Cc: Somebody, Elijah Newren
This makes add/add conflicts use the new handle_file_collision()
function. This leaves the handling of the index the same, but
modifies how the working tree is handled: instead of always doing
a two-way merge of the file contents and recording them at the
collision path name, we instead first estimate the similarity of the
two files involved. If they are not similar, we instead record the
file contents into two separate files for the user to inspect.
Several testcases had to be modified to either expect files to be
written to different locations, or for the two test colliding files
to be modified so that they were similar.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
merge-recursive.c | 25 ++++++++++++++++++++-----
t/t2023-checkout-m.sh | 2 +-
t/t3418-rebase-continue.sh | 27 +++++++++++++++++++++++----
t/t3504-cherry-pick-rerere.sh | 19 ++++++++++++++-----
t/t4200-rerere.sh | 12 ++++++------
t/t6020-merge-df.sh | 4 ++--
t/t6024-recursive-merge.sh | 35 +++++++++++++++++++++--------------
t/t6025-merge-symlinks.sh | 9 ++++++---
t/t6031-merge-filemode.sh | 4 ++--
t/t6042-merge-rename-corner-cases.sh | 2 +-
t/t6043-merge-rename-directories.sh | 15 ++++++++-------
t/t7060-wtstatus.sh | 1 +
t/t7064-wtstatus-pv2.sh | 4 ++--
t/t7506-status-submodule.sh | 11 +++++++----
t/t7610-mergetool.sh | 28 ++++++++++++++--------------
15 files changed, 128 insertions(+), 70 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index 96f0e9cee2..edba4fb11c 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -3070,11 +3070,26 @@ static int process_entry(struct merge_options *o,
clean_merge = -1;
}
} else if (a_oid && b_oid) {
- /* Case C: Added in both (check for same permissions) and */
- /* case D: Modified in both, but differently. */
- clean_merge = merge_content(o, path, 0 /* file_in_way */,
- o_oid, o_mode, a_oid, a_mode, b_oid, b_mode,
- NULL);
+ if (!o_oid) {
+ /* Case C: Added in both (check for same permissions) */
+ output(o, 1,
+ _("CONFLICT (add/add): Merge conflict in %s"),
+ path);
+ clean_merge = handle_file_collision(o,
+ path, NULL, NULL,
+ o->branch1,
+ o->branch2,
+ a_oid, a_mode,
+ b_oid, b_mode,
+ 0);
+ } else
+ /* case D: Modified in both, but differently. */
+ clean_merge = merge_content(o, path,
+ 0 /* file_in_way */,
+ o_oid, o_mode,
+ a_oid, a_mode,
+ b_oid, b_mode,
+ NULL);
} else if (!o_oid && !a_oid && !b_oid) {
/*
* this entry was deleted altogether. a_mode == 0 means
diff --git a/t/t2023-checkout-m.sh b/t/t2023-checkout-m.sh
index 7e18985134..2f8ea52318 100755
--- a/t/t2023-checkout-m.sh
+++ b/t/t2023-checkout-m.sh
@@ -27,7 +27,7 @@ clean_branchnames () {
}
test_expect_success '-m restores 2-way conflicted+resolved file' '
- cp each.txt each.txt.conflicted &&
+ test_must_fail git merge-file -p each.txt~HEAD /dev/null each.txt~master >each.txt.conflicted &&
echo resolved >each.txt &&
git add each.txt &&
git checkout -m -- each.txt &&
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index 7c91a85f43..cf3a82d512 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -10,10 +10,17 @@ set_fake_editor
test_expect_success 'setup' '
test_commit "commit-new-file-F1" F1 1 &&
- test_commit "commit-new-file-F2" F2 2 &&
+ printf "1\n2\n2\n" >F2 &&
+ git add F2 &&
+ test_tick &&
+ git commit -m "commit-new-file-F2" &&
git checkout -b topic HEAD^ &&
- test_commit "commit-new-file-F2-on-topic-branch" F2 22 &&
+ printf "1\n2\n22\n" >F2 &&
+ git add F2 &&
+ test_tick &&
+ git commit -m "commit-new-file-F2-on-topic-branch" &&
+ git tag "commit-new-file-F2-on-topic-branch" &&
git checkout master
'
@@ -48,7 +55,13 @@ test_expect_success 'rebase --continue can not be used with other options' '
test_expect_success 'rebase --continue remembers merge strategy and options' '
rm -fr .git/rebase-* &&
git reset --hard commit-new-file-F2-on-topic-branch &&
- test_commit "commit-new-file-F3-on-topic-branch" F3 32 &&
+
+ printf "1\n2\n32\n" >F3 &&
+ git add F3 &&
+ test_tick &&
+ git commit -m "commit-new-file-F3-on-topic-branch" &&
+ git tag "commit-new-file-F3-on-topic-branch" &&
+
test_when_finished "rm -fr test-bin funny.was.run" &&
mkdir test-bin &&
cat >test-bin/git-merge-funny <<-EOF &&
@@ -92,7 +105,13 @@ test_expect_success 'setup rerere database' '
rm -fr .git/rebase-* &&
git reset --hard commit-new-file-F3-on-topic-branch &&
git checkout master &&
- test_commit "commit-new-file-F3" F3 3 &&
+
+ printf "1\n2\n3\n" >F3 &&
+ git add F3 &&
+ test_tick &&
+ git commit -m "commit-new-file-F3" &&
+ git tag "commit-new-file-F3" &&
+
test_config rerere.enabled true &&
test_must_fail git rebase -m master topic &&
echo "Resolved" >F2 &&
diff --git a/t/t3504-cherry-pick-rerere.sh b/t/t3504-cherry-pick-rerere.sh
index a267b2d144..444ab06ad1 100755
--- a/t/t3504-cherry-pick-rerere.sh
+++ b/t/t3504-cherry-pick-rerere.sh
@@ -4,14 +4,23 @@ test_description='cherry-pick should rerere for conflicts'
. ./test-lib.sh
+test_basic_commit () {
+ file=$2 &&
+ printf "1\n2\n3\n4\n5\n6\n7\n8\n9\n10\n$1" >"$2" &&
+ git add "$file" &&
+ test_tick
+ git commit -m "$1" &&
+ git tag "$1"
+}
+
test_expect_success setup '
- test_commit foo &&
- test_commit foo-master foo &&
- test_commit bar-master bar &&
+ test_basic_commit foo foo &&
+ test_basic_commit foo-master foo &&
+ test_basic_commit bar-master bar &&
git checkout -b dev foo &&
- test_commit foo-dev foo &&
- test_commit bar-dev bar &&
+ test_basic_commit foo-dev foo &&
+ test_basic_commit bar-dev bar &&
git config rerere.enabled true
'
diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
index d97d2bebc9..80451907ce 100755
--- a/t/t4200-rerere.sh
+++ b/t/t4200-rerere.sh
@@ -280,36 +280,36 @@ test_expect_success 'setup: file2 added differently in two branches' '
git reset --hard &&
git checkout -b fourth &&
- echo Hallo >file2 &&
+ printf "1\n2\n3\n4\n5\n6\n7\n8\n9\nHallo" >file2 &&
git add file2 &&
test_tick &&
git commit -m version1 &&
git checkout third &&
- echo Bello >file2 &&
+ printf "1\n2\n3\n4\n5\n6\n7\n8\n9\nBello" >file2 &&
git add file2 &&
test_tick &&
git commit -m version2 &&
test_must_fail git merge fourth &&
- echo Cello >file2 &&
+ printf "1\n2\n3\n4\n5\n6\n7\n8\n9\nCello" >file2 &&
git add file2 &&
git commit -m resolution
'
test_expect_success 'resolution was recorded properly' '
- echo Cello >expected &&
+ printf "1\n2\n3\n4\n5\n6\n7\n8\n9\nCello" >expected &&
git reset --hard HEAD~2 &&
git checkout -b fifth &&
- echo Hallo >file3 &&
+ printf "1\n2\n3\n4\n5\n6\n7\n8\n9\nHallo" >file3 &&
git add file3 &&
test_tick &&
git commit -m version1 &&
git checkout third &&
- echo Bello >file3 &&
+ printf "1\n2\n3\n4\n5\n6\n7\n8\n9\nBello" >file3 &&
git add file3 &&
test_tick &&
git commit -m version2 &&
diff --git a/t/t6020-merge-df.sh b/t/t6020-merge-df.sh
index 2af1beec5f..4ee27d2e20 100755
--- a/t/t6020-merge-df.sh
+++ b/t/t6020-merge-df.sh
@@ -62,7 +62,7 @@ test_expect_success 'setup modify/delete + directory/file conflict' '
# Throw in letters.txt for sorting order fun
# ("letters.txt" sorts between "letters" and "letters/file")
echo i >>letters &&
- echo "version 2" >letters.txt &&
+ printf "1\n2\n3\n4\n5\nversion 2" >letters.txt &&
git add letters letters.txt &&
git commit -m modified &&
@@ -70,7 +70,7 @@ test_expect_success 'setup modify/delete + directory/file conflict' '
git rm letters &&
mkdir letters &&
>letters/file &&
- echo "version 1" >letters.txt &&
+ printf "1\n2\n3\n4\n5\nversion 1" >letters.txt &&
git add letters letters.txt &&
git commit -m deleted
'
diff --git a/t/t6024-recursive-merge.sh b/t/t6024-recursive-merge.sh
index 3f59e58dfb..354f70a66f 100755
--- a/t/t6024-recursive-merge.sh
+++ b/t/t6024-recursive-merge.sh
@@ -15,47 +15,47 @@ GIT_COMMITTER_DATE="2006-12-12 23:28:00 +0100"
export GIT_COMMITTER_DATE
test_expect_success "setup tests" '
-echo 1 > a1 &&
+printf "1\n2\n3\n4\n1\n" > a1 &&
git add a1 &&
GIT_AUTHOR_DATE="2006-12-12 23:00:00" git commit -m 1 a1 &&
git checkout -b A master &&
-echo A > a1 &&
+printf "1\n2\n3\n4\nA\n" > a1 &&
GIT_AUTHOR_DATE="2006-12-12 23:00:01" git commit -m A a1 &&
git checkout -b B master &&
-echo B > a1 &&
+printf "1\n2\n3\n4\nB\n" > a1 &&
GIT_AUTHOR_DATE="2006-12-12 23:00:02" git commit -m B a1 &&
git checkout -b D A &&
git rev-parse B > .git/MERGE_HEAD &&
-echo D > a1 &&
+printf "1\n2\n3\n4\nD\n" > a1 &&
git update-index a1 &&
GIT_AUTHOR_DATE="2006-12-12 23:00:03" git commit -m D &&
git symbolic-ref HEAD refs/heads/other &&
-echo 2 > a1 &&
+printf "1\n2\n3\n4\n2\n" > a1 &&
GIT_AUTHOR_DATE="2006-12-12 23:00:04" git commit -m 2 a1 &&
git checkout -b C &&
-echo C > a1 &&
+printf "1\n2\n3\n4\nC\n" > a1 &&
GIT_AUTHOR_DATE="2006-12-12 23:00:05" git commit -m C a1 &&
git checkout -b E C &&
git rev-parse B > .git/MERGE_HEAD &&
-echo E > a1 &&
+printf "1\n2\n3\n4\nE\n" > a1 &&
git update-index a1 &&
GIT_AUTHOR_DATE="2006-12-12 23:00:06" git commit -m E &&
git checkout -b G E &&
git rev-parse A > .git/MERGE_HEAD &&
-echo G > a1 &&
+printf "1\n2\n3\n4\nG\n" > a1 &&
git update-index a1 &&
GIT_AUTHOR_DATE="2006-12-12 23:00:07" git commit -m G &&
git checkout -b F D &&
git rev-parse C > .git/MERGE_HEAD &&
-echo F > a1 &&
+printf "1\n2\n3\n4\nF\n" > a1 &&
git update-index a1 &&
GIT_AUTHOR_DATE="2006-12-12 23:00:08" git commit -m F
'
@@ -65,6 +65,10 @@ test_expect_success "combined merge conflicts" "
"
cat > expect << EOF
+1
+2
+3
+4
<<<<<<< HEAD
F
=======
@@ -76,9 +80,9 @@ test_expect_success "result contains a conflict" "test_cmp expect a1"
git ls-files --stage > out
cat > expect << EOF
-100644 ec3fe2a791706733f2d8fa7ad45d9a9672031f5e 1 a1
-100644 cf84443e49e1b366fac938711ddf4be2d4d1d9e9 2 a1
-100644 fd7923529855d0b274795ae3349c5e0438333979 3 a1
+100644 a2849e152d46797b83870a479775e980d9a138b1 1 a1
+100644 b1a886a67c3f471cec4d4112a91dc6caa6e3b709 2 a1
+100644 e9d143c719e446df2bafb4b6aba8ed511ecd63b1 3 a1
EOF
test_expect_success "virtual trees were processed" "test_cmp expect out"
@@ -87,10 +91,13 @@ test_expect_success 'refuse to merge binary files' '
git reset --hard &&
printf "\0" > binary-file &&
git add binary-file &&
- git commit -m binary &&
- git checkout G &&
+ git commit -m initial-binary &&
printf "\0\0" > binary-file &&
git add binary-file &&
+ git commit -m binary1 &&
+ git checkout -b H HEAD~1 &&
+ printf "\0\0\0" > binary-file &&
+ git add binary-file &&
git commit -m binary2 &&
test_must_fail git merge F > merge.out 2> merge.err &&
grep "Cannot merge binary files: binary-file (HEAD vs. F)" merge.err
diff --git a/t/t6025-merge-symlinks.sh b/t/t6025-merge-symlinks.sh
index 433c4de08f..08eb85e703 100755
--- a/t/t6025-merge-symlinks.sh
+++ b/t/t6025-merge-symlinks.sh
@@ -37,7 +37,8 @@ test_must_fail git merge master'
test_expect_success \
'the merge result must be a file' '
-test -f symlink'
+test -f symlink~HEAD &&
+test -f symlink~master'
test_expect_success \
'merge master into b-file, which has a file instead of a symbolic link' '
@@ -46,7 +47,8 @@ test_must_fail git merge master'
test_expect_success \
'the merge result must be a file' '
-test -f symlink'
+test -f symlink~HEAD &&
+test -f symlink~master'
test_expect_success \
'merge b-file, which has a file instead of a symbolic link, into master' '
@@ -56,6 +58,7 @@ test_must_fail git merge b-file'
test_expect_success \
'the merge result must be a file' '
-test -f symlink'
+test -f symlink~HEAD &&
+test -f symlink~master'
test_done
diff --git a/t/t6031-merge-filemode.sh b/t/t6031-merge-filemode.sh
index 7d06461f13..79e77f391f 100755
--- a/t/t6031-merge-filemode.sh
+++ b/t/t6031-merge-filemode.sh
@@ -40,12 +40,12 @@ do_one_mode resolve b1 a1
test_expect_success 'set up mode change in both branches' '
git reset --hard HEAD &&
git checkout -b a2 master &&
- : >file2 &&
+ echo true >file2 &&
H=$(git hash-object file2) &&
test_chmod +x file2 &&
git commit -m a2 &&
git checkout -b b2 master &&
- : >file2 &&
+ echo true >file2 &&
git add file2 &&
git commit -m b2 &&
{
diff --git a/t/t6042-merge-rename-corner-cases.sh b/t/t6042-merge-rename-corner-cases.sh
index cf5ea5a0f9..7bd6d64671 100755
--- a/t/t6042-merge-rename-corner-cases.sh
+++ b/t/t6042-merge-rename-corner-cases.sh
@@ -731,7 +731,7 @@ test_conflicts_with_adds_and_renames() {
)
'
- test_expect_failure "check simple $side1/$side2 conflict" '
+ test_expect_success "check simple $side1/$side2 conflict" '
(
cd simple_${side1}_${side2} &&
diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh
index 2e28f2908d..a5560f7d94 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -996,7 +996,7 @@ test_expect_success '5b-check: Rename/delete in order to get add/add/add conflic
git ls-files -u >out &&
test_line_count = 2 out &&
git ls-files -o >out &&
- test_line_count = 1 out &&
+ test_line_count = 3 out &&
git rev-parse >actual \
:0:y/b :0:y/c :0:y/e :2:y/d :3:y/d &&
@@ -1005,7 +1005,9 @@ test_expect_success '5b-check: Rename/delete in order to get add/add/add conflic
test_cmp expect actual &&
test_must_fail git rev-parse :1:y/d &&
- test_path_is_file y/d
+ test_path_is_file y/d~HEAD &&
+ test_path_is_file y/d~B^0 &&
+ test_path_is_missing y/d
)
'
@@ -1077,7 +1079,7 @@ test_expect_success '5c-check: Transitive rename would cause rename/rename/renam
git ls-files -u >out &&
test_line_count = 6 out &&
git ls-files -o >out &&
- test_line_count = 3 out &&
+ test_line_count = 5 out &&
git rev-parse >actual \
:0:y/b :0:y/c :0:y/e &&
@@ -1093,13 +1095,12 @@ test_expect_success '5c-check: Transitive rename would cause rename/rename/renam
test_cmp expect actual &&
git hash-object >actual \
- w/d~HEAD w/d~B^0 z/d &&
+ w/d~HEAD w/d~B^0 y/d~HEAD y/d~B^0 z/d &&
git rev-parse >expect \
- O:x/d B:w/d O:x/d &&
+ O:x/d B:w/d A:y/d B:y/d O:x/d &&
test_cmp expect actual &&
test_path_is_missing x/d &&
- test_path_is_file y/d &&
- grep -q "<<<<" y/d # conflict markers should be present
+ test_path_is_missing y/d
)
'
diff --git a/t/t7060-wtstatus.sh b/t/t7060-wtstatus.sh
index 53cf42fac1..a7888f9fc3 100755
--- a/t/t7060-wtstatus.sh
+++ b/t/t7060-wtstatus.sh
@@ -214,6 +214,7 @@ EOF
git status --untracked-files=no >actual &&
test_i18ncmp expected actual &&
git reset --hard &&
+ git clean -f *~* &&
git checkout master
'
diff --git a/t/t7064-wtstatus-pv2.sh b/t/t7064-wtstatus-pv2.sh
index e319fa2e84..c4d3b1d7e4 100755
--- a/t/t7064-wtstatus-pv2.sh
+++ b/t/t7064-wtstatus-pv2.sh
@@ -259,14 +259,14 @@ test_expect_success 'verify AA (add-add) conflict' '
git branch AA_A master &&
git checkout AA_A &&
- echo "Branch AA_A" >conflict.txt &&
+ printf "1\n2\n3\n4\n5\n6\nBranch AA_A" >conflict.txt &&
OID_AA_A=$(git hash-object -t blob -- conflict.txt) &&
git add conflict.txt &&
git commit -m "branch aa_a" &&
git branch AA_B master &&
git checkout AA_B &&
- echo "Branch AA_B" >conflict.txt &&
+ printf "1\n2\n3\n4\n5\n6\nBranch AA_B" >conflict.txt &&
OID_AA_B=$(git hash-object -t blob -- conflict.txt) &&
git add conflict.txt &&
git commit -m "branch aa_b" &&
diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index 9edf6572ed..0514a019be 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -240,6 +240,8 @@ test_expect_success 'status -a clean (empty submodule dir)' '
cat >status_expect <<\EOF
AA .gitmodules
A sub1
+?? .gitmodules~HEAD
+?? .gitmodules~add_sub1
EOF
test_expect_success 'status with merge conflict in .gitmodules' '
@@ -273,7 +275,7 @@ index badaa4c,44f999a..0000000
--- a/.gitmodules
+++ b/.gitmodules
@@@ -1,3 -1,3 +1,9 @@@
-++<<<<<<< HEAD
+++<<<<<<< ours
+[submodule "sub2"]
+ path = sub2
+ url = ../sub2
@@ -281,7 +283,7 @@ index badaa4c,44f999a..0000000
+ [submodule "sub1"]
+ path = sub1
+ url = ../sub1
-++>>>>>>> add_sub1
+++>>>>>>> theirs
EOF
cat >diff_submodule_expect <<\EOF
@@ -290,7 +292,7 @@ index badaa4c,44f999a..0000000
--- a/.gitmodules
+++ b/.gitmodules
@@@ -1,3 -1,3 +1,9 @@@
-++<<<<<<< HEAD
+++<<<<<<< ours
+[submodule "sub2"]
+ path = sub2
+ url = ../sub2
@@ -298,12 +300,13 @@ index badaa4c,44f999a..0000000
+ [submodule "sub1"]
+ path = sub1
+ url = ../sub1
-++>>>>>>> add_sub1
+++>>>>>>> theirs
EOF
test_expect_success 'diff with merge conflict in .gitmodules' '
(
cd super &&
+ git checkout -m .gitmodules &&
git diff >../diff_actual 2>&1
) &&
test_cmp diff_expect diff_actual
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 1a430b9c40..71f41fc9a8 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -37,9 +37,9 @@ test_expect_success 'setup' '
git checkout -b branch1 master &&
git submodule update -N &&
echo branch1 change >file1 &&
- echo branch1 newfile >file2 &&
+ printf "1\n2\n3\n4\n5\n6\n7\n8\n9\nbranch1 newfile\n" >file2 &&
echo branch1 spaced >"spaced name" &&
- echo branch1 both added >both &&
+ printf "1\n2\n3\n4\n5\n6\n7\n8\n9\n10\nbranch1 both added\n" >both &&
echo branch1 change file11 >file11 &&
echo branch1 change file13 >file13 &&
echo branch1 sub >subdir/file3 &&
@@ -84,9 +84,9 @@ test_expect_success 'setup' '
git checkout master &&
git submodule update -N &&
echo master updated >file1 &&
- echo master new >file2 &&
+ printf "1\n2\n3\n4\n5\n6\n7\n8\n9\nmaster new\n" >file2 &&
echo master updated spaced >"spaced name" &&
- echo master both added >both &&
+ printf "1\n2\n3\n4\n5\n6\n7\n8\n9\n10\nmaster both added\n" >both &&
echo master updated file12 >file12 &&
echo master updated file14 >file14 &&
echo master new sub >subdir/file3 &&
@@ -139,7 +139,7 @@ test_expect_success 'custom mergetool' '
( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
( yes "l" | git mergetool submod >/dev/null 2>&1 ) &&
test "$(cat file1)" = "master updated" &&
- test "$(cat file2)" = "master new" &&
+ test "$(git hash-object file2)" = "$(git rev-parse master:file2)" &&
test "$(cat subdir/file3)" = "master new sub" &&
test "$(cat submod/bar)" = "branch1 submodule" &&
git commit -m "branch1 resolved with mergetool"
@@ -163,7 +163,7 @@ test_expect_success 'mergetool crlf' '
( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
( yes "r" | git mergetool submod >/dev/null 2>&1 ) &&
test "$(printf x | cat file1 -)" = "$(printf "master updated\r\nx")" &&
- test "$(printf x | cat file2 -)" = "$(printf "master new\r\nx")" &&
+ test "$(printf x | cat file2 -)" = "$(printf "1\r\n2\r\n3\r\n4\r\n5\r\n6\r\n7\r\n8\r\n9\r\nmaster new\r\nx")" &&
test "$(printf x | cat subdir/file3 -)" = "$(printf "master new sub\r\nx")" &&
git submodule update -N &&
test "$(cat submod/bar)" = "master submodule" &&
@@ -171,7 +171,7 @@ test_expect_success 'mergetool crlf' '
'
test_expect_success 'mergetool in subdir' '
- test_when_finished "git reset --hard" &&
+ test_when_finished "git reset --hard && git clean -f" &&
git checkout -b test$test_count branch1 &&
git submodule update -N &&
(
@@ -183,7 +183,7 @@ test_expect_success 'mergetool in subdir' '
'
test_expect_success 'mergetool on file in parent dir' '
- test_when_finished "git reset --hard" &&
+ test_when_finished "git reset --hard && git clean -f" &&
git checkout -b test$test_count branch1 &&
git submodule update -N &&
(
@@ -197,14 +197,14 @@ test_expect_success 'mergetool on file in parent dir' '
( yes "d" | git mergetool ../file12 >/dev/null 2>&1 ) &&
( yes "l" | git mergetool ../submod >/dev/null 2>&1 ) &&
test "$(cat ../file1)" = "master updated" &&
- test "$(cat ../file2)" = "master new" &&
+ test "$(git hash-object ../file2)" = "$(git rev-parse master:file2)" &&
test "$(cat ../submod/bar)" = "branch1 submodule" &&
git commit -m "branch1 resolved with mergetool - subdir"
)
'
test_expect_success 'mergetool skips autoresolved' '
- test_when_finished "git reset --hard" &&
+ test_when_finished "git reset --hard && git clean -f" &&
git checkout -b test$test_count branch1 &&
git submodule update -N &&
test_must_fail git merge master &&
@@ -217,7 +217,7 @@ test_expect_success 'mergetool skips autoresolved' '
'
test_expect_success 'mergetool merges all from subdir (rerere disabled)' '
- test_when_finished "git reset --hard" &&
+ test_when_finished "git reset --hard && git clean -f" &&
git checkout -b test$test_count branch1 &&
test_config rerere.enabled false &&
(
@@ -226,7 +226,7 @@ test_expect_success 'mergetool merges all from subdir (rerere disabled)' '
( yes "r" | git mergetool ../submod ) &&
( yes "d" "d" | git mergetool --no-prompt ) &&
test "$(cat ../file1)" = "master updated" &&
- test "$(cat ../file2)" = "master new" &&
+ test "$(git hash-object ../file2)" = "$(git rev-parse master:file2)" &&
test "$(cat file3)" = "master new sub" &&
( cd .. && git submodule update -N ) &&
test "$(cat ../submod/bar)" = "master submodule" &&
@@ -245,7 +245,7 @@ test_expect_success 'mergetool merges all from subdir (rerere enabled)' '
( yes "r" | git mergetool ../submod ) &&
( yes "d" "d" | git mergetool --no-prompt ) &&
test "$(cat ../file1)" = "master updated" &&
- test "$(cat ../file2)" = "master new" &&
+ test "$(git hash-object ../file2)" = "$(git rev-parse master:file2)" &&
test "$(cat file3)" = "master new sub" &&
( cd .. && git submodule update -N ) &&
test "$(cat ../submod/bar)" = "master submodule" &&
@@ -631,7 +631,7 @@ test_expect_success 'custom commands override built-ins' '
test_config mergetool.defaults.trustExitCode true &&
test_must_fail git merge master &&
git mergetool --no-prompt --tool defaults -- both &&
- echo master both added >expected &&
+ git cat-file -p master:both >expected &&
test_cmp expected both
'
--
2.16.0.41.g6a66043158
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [RFC PATCH 1/5] Add testcases for improved file collision conflict handling
2018-03-05 17:11 ` [RFC PATCH 1/5] Add testcases for improved file collision conflict handling Elijah Newren
@ 2018-03-08 12:25 ` SZEDER Gábor
2018-03-08 17:51 ` Elijah Newren
0 siblings, 1 reply; 9+ messages in thread
From: SZEDER Gábor @ 2018-03-08 12:25 UTC (permalink / raw)
To: Elijah Newren; +Cc: SZEDER Gábor, git
> Adds testcases dealing with file collisions for the following types of
> conflicts:
> * add/add
> * rename/add
> * rename/rename(2to1)
> These tests include expectations for proposed smarter behavior which has
> not yet been implemented and thus are currently expected to fail.
> Subsequent commits will correct that and explain the new behavior.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
> t/t6042-merge-rename-corner-cases.sh | 220 +++++++++++++++++++++++++++++++++++
> 1 file changed, 220 insertions(+)
>
> diff --git a/t/t6042-merge-rename-corner-cases.sh b/t/t6042-merge-rename-corner-cases.sh
> index 411550d2b6..a6c151ef95 100755
> --- a/t/t6042-merge-rename-corner-cases.sh
> +++ b/t/t6042-merge-rename-corner-cases.sh
> @@ -575,4 +575,224 @@ test_expect_success 'rename/rename/add-dest merge still knows about conflicting
> test ! -f c
> '
>
> +test_conflicts_with_adds_and_renames() {
> + test $1 != 0 && side1=rename || side1=add
> + test $2 != 0 && side2=rename || side2=add
For additonal context I'm going to quote the callsites of this
function from the end of the test script:
> +test_conflicts_with_adds_and_renames 1 1
> +test_conflicts_with_adds_and_renames 1 0
> +test_conflicts_with_adds_and_renames 0 1
> +test_conflicts_with_adds_and_renames 0 0
Instead of the two conditions at the beginning of the function and the
1 and 0 sort-of magic numbers at the callsites, you could just pass
the words "rename" and "add" as parameters to the function. The
callsites would be clearer and the function could start with two
simple variable assignments side1=$1 ; side2=$2.
Please feel free to dismiss this as bikeshedding: since the branches
are called 'L' and 'R', maybe calling the variables $sideL and $sideR
would match better the rest of the test? Dunno.
> + # Setup:
> + # L
> + # / \
> + # master ?
> + # \ /
> + # R
> + #
> + # Where:
> + # Both L and R have files named 'three-unrelated' and
> + # 'three-related' which collide (i.e. 4 files colliding at two
> + # pathnames). Each of the colliding files could have been
> + # involved in a rename, in which case there was a file named
> + # 'one-[un]related' or 'two-[un]related' that was modified on the
> + # opposite side of history and renamed into the collision on this
> + # side of history.
> + #
> + # Questions for both sets of collisions:
> + # 1) The index should contain both a stage 2 and stage 3 entry
> + # for the colliding file. Does it?
> + # 2) When renames are involved, the content merges are clean, so
> + # the index should reflect the content merges, not merely the
> + # version of the colliding file from the prior commit. Does
> + # it?
> + #
> + # Questions for three-unrelated:
> + # 3) There should be files in the worktree named
> + # 'three-unrelated~HEAD' and 'three-unrelated~R^0' with the
> + # (content-merged) version of 'three-unrelated' from the
> + # appropriate side of the merge. Are they present?
> + # 4) There should be no file named 'three-unrelated' in the
> + # working tree. That'd make it too likely that users would
> + # use it instead of carefully looking at both
> + # three-unrelated~HEAD and three-unrelated~R^0. Is it
> + # correctly missing?
> + #
> + # Questions for three-related:
> + # 3) There should be a file in the worktree named three-related
> + # containing the two-way merged contents of the content-merged
> + # versions of three-related from each of the two colliding
> + # files. Is it present?
> + # 4) There should not be any three-related~* files in the working
> + # tree.
> + test_expect_success "setup simple $side1/$side2 conflict" '
> + test_create_repo simple_${side1}_${side2} &&
> + (
> + cd simple_${side1}_${side2} &&
> +
> + # Create a simple file with 10 lines
> + ten="0 1 2 3 4 5 6 7 8 9" &&
> + for i in $ten
> + do
> + echo line $i in a sample file
> + done >unrelated1_v1 &&
> + # Create a 2nd version of same file with one more line
> + cat unrelated1_v1 >unrelated1_v2 &&
'cp unrelated1_v1 unrelated1_v2', perhaps?
> + echo another line >>unrelated1_v2 &&
> +
> + # Create an unrelated simple file with 10 lines
> + for i in $ten
> + do
> + echo line $i in another sample file
> + done >unrelated2_v1 &&
> + # Create a 2nd version of same file with one more line
> + cat unrelated2_v1 >unrelated2_v2 &&
Likewise.
> + echo another line >>unrelated2_v2 &&
> +
> + # Create some related files now
> + for i in $ten
> + do
> + echo Random base content line $i
> + done >related1_v1 &&
> + cp -a related1_v1 related1_v2 &&
Wouldn't a "plain" 'cp', i.e. without '-a', be sufficient?
> + echo modification >>related1_v2 &&
> +
> + cp -a related1_v1 related2_v1 &&
> + echo more stuff >>related2_v1 &&
> + cp -a related2_v1 related2_v2 &&
> + echo yet more stuff >>related2_v2 &&
> +
> + # Use a tag to record both these files for simple
> + # access, and clean out these untracked files
> + git tag unrelated1_v1 `git hash-object -w unrelated1_v1` &&
> + git tag unrelated1_v2 `git hash-object -w unrelated1_v2` &&
> + git tag unrelated2_v1 `git hash-object -w unrelated2_v1` &&
> + git tag unrelated2_v2 `git hash-object -w unrelated2_v2` &&
> + git tag related1_v1 `git hash-object -w related1_v1` &&
> + git tag related1_v2 `git hash-object -w related1_v2` &&
> + git tag related2_v1 `git hash-object -w related2_v1` &&
> + git tag related2_v2 `git hash-object -w related2_v2` &&
Style nit: please use $() for command substitutions instead of
backticks.
> + git clean -f &&
> +
> + # Setup merge-base, consisting of files named "one-*"
> + # and "two-*" if renames were involved.
> + touch irrelevant_file &&
> + git add irrelevant_file &&
> + if [ $side1 == "rename" ]; then
Another style nit:
if test $side1 = "rename"
then
...
> + git show unrelated1_v1 >one-unrelated &&
> + git add one-unrelated
Broken && chain.
Please check the subsequent if statements as well, there are more
places where the && is missing.
Also note that you can run 'git add one-unrelated one-related', i.e.
add more than one file at once, sparing a couple of lines and git
executions.
> + git show related1_v1 >one-related &&
> + git add one-related
> + fi &&
> + if [ $side2 == "rename" ]; then
> + git show unrelated2_v1 >two-unrelated &&
> + git add two-unrelated
> + git show related2_v1 >two-related &&
> + git add two-related
> + fi &&
> + test_tick && git commit -m initial &&
> +
> + git branch L &&
> + git branch R &&
> +
> + # Handle the left side
> + git checkout L &&
> + if [ $side1 == "rename" ]; then
> + git mv one-unrelated three-unrelated
> + git mv one-related three-related
> + else
> + git show unrelated1_v2 >three-unrelated &&
> + git add three-unrelated
> + git show related1_v2 >three-related &&
> + git add three-related
> + fi &&
> + if [ $side2 == "rename" ]; then
> + git show unrelated2_v2 >two-unrelated &&
> + git add two-unrelated
> + git show related2_v2 >two-related &&
> + git add two-related
> + fi &&
> + test_tick && git commit -m L &&
> +
> + # Handle the right side
> + git checkout R &&
> + if [ $side1 == "rename" ]; then
> + git show unrelated1_v2 >one-unrelated &&
> + git add one-unrelated
> + git show related1_v2 >one-related &&
> + git add one-related
> + fi &&
> + if [ $side2 == "rename" ]; then
> + git mv two-unrelated three-unrelated
> + git mv two-related three-related
> + else
> + git show unrelated2_v2 >three-unrelated &&
> + git add three-unrelated
> + git show related2_v2 >three-related &&
> + git add three-related
> + fi &&
> + test_tick && git commit -m R
> + )
> + '
This setup test is enormous, and the conditions for the combination of
the two sides and the add/rename conflicts are somewhat distracting.
I don't know how it could be structured better/shorter/clearer... I
couldn't come up with anything useful during lunch.
> +
> + test_expect_failure "check simple $side1/$side2 conflict" '
> + (
> + cd simple_${side1}_${side2} &&
> +
> + git checkout L^0 &&
> +
> + # Merge must fail; there is a conflict
> + test_must_fail git merge -s recursive R^0 &&
> +
> + # Make sure the index has the right number of entries
> + git ls-files -s >out &&
> + test_line_count = 5 out &&
> + git ls-files -u >out &&
> + test_line_count = 4 out &&
> +
> + # Nothing should have touched irrelevant_file
> + git rev-parse >actual \
> + :0:irrelevant_file \
> + :2:three-unrelated :3:three-unrelated \
> + :2:three-related :3:three-related &&
> + git rev-parse >expected \
> + master:irrelevant_file \
> + unrelated1_v2 unrelated2_v2 \
> + related1_v2 related2_v2 &&
Missing 'test_cmp'? The above lines write the files 'actual' and
'expected', but they are never looked at.
> + # Ensure we have the correct number of untracked files
> + git ls-files -o >out &&
> + test_line_count = 5 out &&
> +
> + # Make sure each file (with merging if rename
> + # involved) is present in the working tree for the
> + # user to work with.
> + git hash-object >actual \
> + three-unrelated~HEAD three-unrelated~R^0 &&
> + git rev-parse >expected \
> + unrelated1_v2 unrelated2_v2 &&
Again, missing 'test_cmp'?
> + # "three-unrelated" should not exist because there is
> + # no reason to give preference to either
> + # three-unrelated~HEAD or three-unrelated~R^0
> + test_path_is_missing three-unrelated &&
> +
> + # Make sure we have the correct merged contents for
> + # three-related
> + git show related1_v1 >expected &&
> + cat <<EOF >>expected &&
> +<<<<<<< HEAD
> +modification
> +=======
> +more stuff
> +yet more stuff
> +>>>>>>> R^0
> +EOF
You could use 'cat <<-EOF ....' and indent the here doc with tabs, so
it won't look so out-of-place. Or even '<<-\EOF' to indicate that
there is nothing to be expanded in the here doc.
> +
> + test_cmp expected three-related
> + )
> + '
> +}
> +
> +test_conflicts_with_adds_and_renames 1 1
> +test_conflicts_with_adds_and_renames 1 0
> +test_conflicts_with_adds_and_renames 0 1
> +test_conflicts_with_adds_and_renames 0 0
> +
> test_done
> --
> 2.16.0.41.g6a66043158
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH 1/5] Add testcases for improved file collision conflict handling
2018-03-08 12:25 ` SZEDER Gábor
@ 2018-03-08 17:51 ` Elijah Newren
0 siblings, 0 replies; 9+ messages in thread
From: Elijah Newren @ 2018-03-08 17:51 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: Git Mailing List
Sweet! Thanks for taking a look, and for spotting lots of
improvements (and some really embarrassing errors). I'll keep the
fixes queued up while waiting for other feedback. A few comments...
On Thu, Mar 8, 2018 at 4:25 AM, SZEDER Gábor <szeder.dev@gmail.com> wrote:
> This setup test is enormous, and the conditions for the combination of
> the two sides and the add/rename conflicts are somewhat distracting.
> I don't know how it could be structured better/shorter/clearer... I
> couldn't come up with anything useful during lunch.
Yeah. :-( It's part attempt to test these conflict types much more
thoroughly than other tests in the testsuite do, and part attempt to
keep the test setup consistent between the types to reflect the fact
that I'm consolidating the conflict resolution into a single function
as well.
Two possible ideas:
* Split the tests for "*_unrelated" files and "*_related" files into
separate tests (doubling the number of tests, but making each only
deal with half the files. That would make each setup function about
half the size, though both check functions would be about as big as
the original.
* Instead of programatically generated tests, just manually write
out the tests for each of the four combinations (rename/rename,
rename/add, add/rename, add/add). That means four "copies" of fairly
similar functions (and possible greater difficulty keeping things
consistent if changes are requested), but does allow removal of the
three different if-statements and thus makes each one easier to
understand in isolation.
Doing both would be possible as well.
Personally, I'm much more in favor of the first idea than the second.
I'm still kind of borderline about whether to make the change
mentioned in the first idea, but if others feel that splitting would
help a lot, I'm happy to look into either or both ideas.
>> + cat <<EOF >>expected &&
>> +<<<<<<< HEAD
>> +modification
>> +=======
>> +more stuff
>> +yet more stuff
>> +>>>>>>> R^0
>> +EOF
>
> You could use 'cat <<-EOF ....' and indent the here doc with tabs, so
> it won't look so out-of-place. Or even '<<-\EOF' to indicate that
> there is nothing to be expanded in the here doc.
I just learned two new things about heredocs; I've wanted both of
those things in the past, but didn't even think to check if they were
possible. Thanks for enlightening me.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH 0/5] Improve path collision conflict resolutions
2018-03-05 17:11 [RFC PATCH 0/5] Improve path collision conflict resolutions Elijah Newren
` (4 preceding siblings ...)
2018-03-05 17:11 ` [RFC PATCH 5/5] merge-recursive: improve handling for add/add conflicts Elijah Newren
@ 2018-03-12 18:19 ` Elijah Newren
5 siblings, 0 replies; 9+ messages in thread
From: Elijah Newren @ 2018-03-12 18:19 UTC (permalink / raw)
To: Git Mailing List; +Cc: Elijah Newren
On Mon, Mar 5, 2018 at 9:11 AM, Elijah Newren <newren@gmail.com> wrote:
> 2) Doing content merges for a rename before looking at the path collision
> between a rename and some other file. In particular (in what I most
> suspect others might have an objection to from this series), record
> that content-merged file -- which may have conflict markers -- in the
> index at the appropriate higher stage.
>
> 2)
>
> Previously, rename/rename(2to1) conflict resolution for the colliding path
> would just accept the index changes made by unpack_trees(), meaning that
> each of the higher order stages in the index for the path collision would
> implicitly ignore any changes to each renamed file from the other side of
> history. Since, as noted above, all traces of the rename-source path were
> removed from both the index and the working tree, this meant that the index
> was missing information about changes to such files. If the user tried to
> resolve the conflict using the index rather than the working copy, they
> would end up with a silent loss of changes.
>
> I "fixed" this by doing the three-way content merge for each renamed-file,
> and then recorded THAT in the index at either stage 2 or 3 as appropriate.
> Since that merge might have conflict markers, that could mean recording in
> the index a file with conflict markers as though it were a given side.
> (See patch 2 for a more detailed explanation.) I figure this might be the
> most controversial change I made. I can think of a few alternatives, but I
> liked all of them less. Opinions?
Actually, I realized this last weekend that this wasn't unprecedented
after all, and thus likely not at all controversial. In particular,
there is already a case where git stores the sha1 of a file with
conflict markers from a "provisional content merge" at a non-zero
stage in the index: If a recursive merge is needed and there is a file
with content conflicts when creating the virtual merge base, then the
file with all the conflict markers will be accepted as part of the
virtual merge base and, depending on how the outer merge goes, the
sha1 of this file with conflict markers can appear in the index at
stage 1.
So, that really only leaves the changes to the working tree files.
And based on a few factors including things mentioned in the cover
letter, I suspect most would only have an opinion about how this patch
series affects add/add conflicts. I'll send a separate email to ask
about that, to make it clear I want folks opinions on that issue even
if they don't have enough time to review the patch series or even read
my really long cover letter.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-03-12 18:19 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-05 17:11 [RFC PATCH 0/5] Improve path collision conflict resolutions Elijah Newren
2018-03-05 17:11 ` [RFC PATCH 1/5] Add testcases for improved file collision conflict handling Elijah Newren
2018-03-08 12:25 ` SZEDER Gábor
2018-03-08 17:51 ` Elijah Newren
2018-03-05 17:11 ` [RFC PATCH 2/5] merge-recursive: new function for better colliding conflict resolutions Elijah Newren
2018-03-05 17:11 ` [RFC PATCH 3/5] merge-recursive: fix rename/add conflict handling Elijah Newren
2018-03-05 17:11 ` [RFC PATCH 4/5] merge-recursive: improve handling for rename/rename(2to1) conflicts Elijah Newren
2018-03-05 17:11 ` [RFC PATCH 5/5] merge-recursive: improve handling for add/add conflicts Elijah Newren
2018-03-12 18:19 ` [RFC PATCH 0/5] Improve path collision conflict resolutions Elijah Newren
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).