This patchset introduces directory rename detection to merge-recursive. See
https://public-inbox.org/git/20171110190550.27059-1-newren@gmail.com/
for the first series (including design considerations, etc.) This series
continues to depend on en/merge-recursive-fixes in next, at least
contextually. For the curious, follow-up series and comments can also be
found at
https://public-inbox.org/git/20171120220209.15111-1-newren@gmail.com/https://public-inbox.org/git/20171121080059.32304-1-newren@gmail.com/https://public-inbox.org/git/20171129014237.32570-1-newren@gmail.com/https://public-inbox.org/git/20171228041352.27880-1-newren@gmail.com/https://public-inbox.org/git/20180105202711.24311-1-newren@gmail.com/https://public-inbox.org/git/20180130232533.25846-1-newren@gmail.com/
Also, as a reminder, this series fixes a few bugs somewhat as a side effect:
* a bug causing dirty files involved in a rename to be overwritten
* a few memory leaks
Changes since v7 (full tbdiff follows below):
* Added Stefan's Reviewed-by.
* Squashed commits introducing new hash structs and associated functions
into the commit that used them to avoid unused function
warnings/errors.
* Added or clarified a number of comments where things were unclear
* Minor stuff:
* Style (and typo) fixes for commit message and comments
* Avoiding casting with hash initialization function
* s/malloc/xmalloc/
* struct assignment
* s/20/GIT_MAX_RAWSZ/
Elijah Newren (29):
directory rename detection: basic testcases
directory rename detection: directory splitting testcases
directory rename detection: testcases to avoid taking detection too
far
directory rename detection: partially renamed directory
testcase/discussion
directory rename detection: files/directories in the way of some
renames
directory rename detection: testcases checking which side did the
rename
directory rename detection: more involved edge/corner testcases
directory rename detection: testcases exploring possibly suboptimal
merges
directory rename detection: miscellaneous testcases to complete
coverage
directory rename detection: tests for handling overwriting untracked
files
directory rename detection: tests for handling overwriting dirty files
merge-recursive: move the get_renames() function
merge-recursive: introduce new functions to handle rename logic
merge-recursive: fix leaks of allocated renames and diff_filepairs
merge-recursive: make !o->detect_rename codepath more obvious
merge-recursive: split out code for determining diff_filepairs
merge-recursive: make a helper function for cleanup for handle_renames
merge-recursive: add get_directory_renames()
merge-recursive: check for directory level conflicts
merge-recursive: add computation of collisions due to dir rename &
merging
merge-recursive: check for file level conflicts then get new name
merge-recursive: when comparing files, don't include trees
merge-recursive: apply necessary modifications for directory renames
merge-recursive: avoid clobbering untracked files with directory
renames
merge-recursive: fix overwriting dirty files involved in renames
merge-recursive: fix remaining directory rename + dirty overwrite
cases
directory rename detection: new testcases showcasing a pair of bugs
merge-recursive: avoid spurious rename/rename conflict from dir
renames
merge-recursive: ensure we write updates for directory-renamed file
merge-recursive.c | 1243 ++++++++++-
merge-recursive.h | 27 +
strbuf.c | 16 +
strbuf.h | 16 +
t/t3501-revert-cherry-pick.sh | 2 +-
t/t6043-merge-rename-directories.sh | 3998 +++++++++++++++++++++++++++++++++++
t/t7607-merge-overwrite.sh | 2 +-
unpack-trees.c | 4 +-
unpack-trees.h | 4 +
9 files changed, 5197 insertions(+), 115 deletions(-)
create mode 100755 t/t6043-merge-rename-directories.sh
Full tbdiff (the biggest code changes come from commit squashing):
1: 5ba69c9c7b ! 1: 9f1d894d89 directory rename detection: basic testcases
@@ -2,6 +2,7 @@
directory rename detection: basic testcases
+ Reviewed-by: Stefan Beller [off-list ref]
Signed-off-by: Elijah Newren [off-list ref]
diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh
2: e1d23f7f95 ! 2: 36a4b05757 directory rename detection: directory splitting testcases
@@ -2,6 +2,7 @@
directory rename detection: directory splitting testcases
+ Reviewed-by: Stefan Beller [off-list ref]
Signed-off-by: Elijah Newren [off-list ref]
diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh
3: b10cb49cf9 ! 3: 031a835801 directory rename detection: testcases to avoid taking detection too far
@@ -2,6 +2,7 @@
directory rename detection: testcases to avoid taking detection too far
+ Reviewed-by: Stefan Beller [off-list ref]
Signed-off-by: Elijah Newren [off-list ref]
diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh
4: ec3ccf0a95 ! 4: 5a09b80671 directory rename detection: partially renamed directory testcase/discussion
@@ -2,6 +2,10 @@
directory rename detection: partially renamed directory testcase/discussion
+ Add a long note about why we are not considering "partial directory
+ renames" for the current directory rename detection implementation.
+
+ Reviewed-by: Stefan Beller [off-list ref]
Signed-off-by: Elijah Newren [off-list ref]
diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh
@@ -40,7 +44,15 @@
+# path towards crazy corner cases that are far more complex than what we're
+# already dealing with.
+#
-+# This section contains a test for this partially-renamed-directory case.
++# Note that the wording of the rule ("We don't do directory rename
++# detection if the directory still exists on both sides of the merge.")
++# also excludes "renaming" of a directory into a subdirectory of itself
++# (e.g. /some/dir/* -> /some/dir/subdir/*). It may be possible to carve
++# out an exception for "renaming"-beneath-itself cases without opening
++# weird edge/corner cases for other partial directory renames, but for now
++# we are keeping the rule simple.
++#
++# This section contains a test for a partially-renamed-directory case.
+###########################################################################
+
+# Testcase 4a, Directory split, with original directory still present
5: da018f1adb ! 5: f6f7fe21b4 directory rename detection: files/directories in the way of some renames
@@ -2,6 +2,7 @@
directory rename detection: files/directories in the way of some renames
+ Reviewed-by: Stefan Beller [off-list ref]
Signed-off-by: Elijah Newren [off-list ref]
diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh
6: f7ca54e7f2 ! 6: f34670c87a directory rename detection: testcases checking which side did the rename
@@ -2,6 +2,7 @@
directory rename detection: testcases checking which side did the rename
+ Reviewed-by: Stefan Beller [off-list ref]
Signed-off-by: Elijah Newren [off-list ref]
diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh
7: 8ae28f45fe ! 7: 0bb552373e directory rename detection: more involved edge/corner testcases
@@ -2,6 +2,7 @@
directory rename detection: more involved edge/corner testcases
+ Reviewed-by: Stefan Beller [off-list ref]
Signed-off-by: Elijah Newren [off-list ref]
diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh
8: 8d05b8dc10 ! 8: e9c6bcb5bf directory rename detection: testcases exploring possibly suboptimal merges
@@ -2,6 +2,7 @@
directory rename detection: testcases exploring possibly suboptimal merges
+ Reviewed-by: Stefan Beller [off-list ref]
Signed-off-by: Elijah Newren [off-list ref]
diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh
9: 47ffccc86d ! 9: 80d1c2807f directory rename detection: miscellaneous testcases to complete coverage
@@ -8,6 +8,7 @@
into the previous sections because I didn't want to re-label with all the
testcase references. :-)
+ Reviewed-by: Stefan Beller [off-list ref]
Signed-off-by: Elijah Newren [off-list ref]
diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh
10: db7d9850c2 ! 10: 90257851c2 directory rename detection: tests for handling overwriting untracked files
@@ -2,6 +2,7 @@
directory rename detection: tests for handling overwriting untracked files
+ Reviewed-by: Stefan Beller [off-list ref]
Signed-off-by: Elijah Newren [off-list ref]
diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh
11: 0de0a9dfa0 ! 11: e558395fe2 directory rename detection: tests for handling overwriting dirty files
@@ -2,6 +2,7 @@
directory rename detection: tests for handling overwriting dirty files
+ Reviewed-by: Stefan Beller [off-list ref]
Signed-off-by: Elijah Newren [off-list ref]
diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh
12: 9a6777f577 ! 12: f7ce963690 merge-recursive: move the get_renames() function
@@ -2,10 +2,12 @@
merge-recursive: move the get_renames() function
- I want to re-use some other functions in the file without moving those
- other functions or dealing with a handful of annoying split function
- declarations and definitions.
+ Move this function so it can re-use some others (without either
+ moving all of them or adding an annoying split between function
+ declarations and definitions). Cheat slightly by adding a blank line
+ for readability, and in order to silence checkpatch.pl.
+ Reviewed-by: Stefan Beller [off-list ref]
Signed-off-by: Elijah Newren [off-list ref]
diff --git a/merge-recursive.c b/merge-recursive.c
13: ac6a95c7b8 ! 13: 467827818c merge-recursive: introduce new functions to handle rename logic
@@ -16,6 +16,7 @@
which is used later in process_entry(). Thus the reason for a separate
cleanup_renames().
+ Reviewed-by: Stefan Beller [off-list ref]
Signed-off-by: Elijah Newren [off-list ref]
diff --git a/merge-recursive.c b/merge-recursive.c
14: 76b09d49cd ! 14: 2079029a75 merge-recursive: fix leaks of allocated renames and diff_filepairs
@@ -8,6 +8,7 @@
return string_list. Make sure all of these are deallocated when we
are done with them.
+ Reviewed-by: Stefan Beller [off-list ref]
Signed-off-by: Elijah Newren [off-list ref]
diff --git a/merge-recursive.c b/merge-recursive.c
15: e4189f3da2 ! 15: 6b5b10e76f merge-recursive: make !o->detect_rename codepath more obvious
@@ -7,6 +7,7 @@
iterate over. It seems more straightforward to simply avoid calling
either function in that case.
+ Reviewed-by: Stefan Beller [off-list ref]
Signed-off-by: Elijah Newren [off-list ref]
diff --git a/merge-recursive.c b/merge-recursive.c
16: 6bc800e369 ! 16: 02cf55e49e merge-recursive: split out code for determining diff_filepairs
@@ -7,6 +7,7 @@
get_renames(), I want them to be available to some new functions. No
actual logic changes yet.
+ Reviewed-by: Stefan Beller [off-list ref]
Signed-off-by: Elijah Newren [off-list ref]
diff --git a/merge-recursive.c b/merge-recursive.c
@@ -48,10 +49,8 @@
o->needed_rename_limit = opts.needed_rename_limit;
- for (i = 0; i < diff_queued_diff.nr; ++i) {
+
-+ ret = malloc(sizeof(struct diff_queue_struct));
-+ ret->queue = diff_queued_diff.queue;
-+ ret->nr = diff_queued_diff.nr;
-+ /* Ignore diff_queued_diff.alloc; we won't be changing size at all */
++ ret = xmalloc(sizeof(*ret));
++ *ret = diff_queued_diff;
+
+ opts.output_format = DIFF_FORMAT_NO_OUTPUT;
+ diff_queued_diff.nr = 0;
17: 0757c92ca1 < --: ------- merge-recursive: add a new hashmap for storing directory renames
18: f17343fc2c ! 17: 24f31fa43a merge-recursive: make a helper function for cleanup for handle_renames
@@ -8,6 +8,7 @@
helper initial_cleanup_rename(), and leave the big comment in the code
about why we can't do all the cleanup at once.
+ Reviewed-by: Stefan Beller [off-list ref]
Signed-off-by: Elijah Newren [off-list ref]
diff --git a/merge-recursive.c b/merge-recursive.c
19: 9b63e257c8 ! 18: ae89010bec merge-recursive: add get_directory_renames()
@@ -2,15 +2,68 @@
merge-recursive: add get_directory_renames()
- This populates a list of directory renames for us. The list of
- directory renames is not yet used, but will be in subsequent commits.
+ This populates a set of directory renames for us. The set of directory
+ renames is not yet used, but will be in subsequent commits.
+ Note that the use of a string_list for possible_new_dirs in the new
+ dir_rename_entry struct implies an O(n^2) algorithm; however, in practice
+ I expect the number of distinct directories that files were renamed into
+ from a single original directory to be O(1). My guess is that n has a
+ mode of 1 and a mean of less than 2, so, for now, string_list seems good
+ enough for possible_new_dirs.
+
+ Reviewed-by: Stefan Beller [off-list ref]
Signed-off-by: Elijah Newren [off-list ref]
diff --git a/merge-recursive.c b/merge-recursive.c
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@
+ return ignore_case ? strihash(path) : strhash(path);
+ }
+
++static struct dir_rename_entry *dir_rename_find_entry(struct hashmap *hashmap,
++ char *dir)
++{
++ struct dir_rename_entry key;
++
++ if (dir == NULL)
++ return NULL;
++ hashmap_entry_init(&key, strhash(dir));
++ key.dir = dir;
++ return hashmap_get(hashmap, &key, NULL);
++}
++
++static int dir_rename_cmp(const void *unused_cmp_data,
++ const void *entry,
++ const void *entry_or_key,
++ const void *unused_keydata)
++{
++ const struct dir_rename_entry *e1 = entry;
++ const struct dir_rename_entry *e2 = entry_or_key;
++
++ return strcmp(e1->dir, e2->dir);
++}
++
++static void dir_rename_init(struct hashmap *map)
++{
++ hashmap_init(map, dir_rename_cmp, NULL, 0);
++}
++
++static void dir_rename_entry_init(struct dir_rename_entry *entry,
++ char *directory)
++{
++ hashmap_entry_init(entry, strhash(directory));
++ entry->dir = directory;
++ entry->non_unique_new_dir = 0;
++ strbuf_init(&entry->new_dir, 0);
++ string_list_init(&entry->possible_new_dirs, 0);
++}
++
+ static void flush_output(struct merge_options *o)
+ {
+ if (o->buffer_output < 2 && o->obuf.len) {
+@@
return ret;
}
@@ -23,12 +76,13 @@
+ *old_dir = NULL;
+ *new_dir = NULL;
+
-+ /* For
-+ * "a/b/c/d/foo.c" -> "a/b/something-else/d/foo.c"
-+ * the "d/foo.c" part is the same, we just want to know that
-+ * "a/b/c" was renamed to "a/b/something-else"
-+ * so, for this example, this function returns "a/b/c" in
-+ * *old_dir and "a/b/something-else" in *new_dir.
++ /*
++ * For
++ * "a/b/c/d/e/foo.c" -> "a/b/some/thing/else/e/foo.c"
++ * the "e/foo.c" part is the same, we just want to know that
++ * "a/b/c/d" was renamed to "a/b/some/thing/else"
++ * so, for this example, this function returns "a/b/c/d" in
++ * *old_dir and "a/b/some/thing/else" in *new_dir.
+ *
+ * Also, if the basename of the file changed, we don't care. We
+ * want to know which portion of the directory, if any, changed.
@@ -76,7 +130,23 @@
+ struct dir_rename_entry *entry;
+ int i;
+
-+ dir_renames = malloc(sizeof(struct hashmap));
++ /*
++ * Typically, we think of a directory rename as all files from a
++ * certain directory being moved to a target directory. However,
++ * what if someone first moved two files from the original
++ * directory in one commit, and then renamed the directory
++ * somewhere else in a later commit? At merge time, we just know
++ * that files from the original directory went to two different
++ * places, and that the bulk of them ended up in the same place.
++ * We want each directory rename to represent where the bulk of the
++ * files from that directory end up; this function exists to find
++ * where the bulk of the files went.
++ *
++ * The first loop below simply iterates through the list of file
++ * renames, finding out how often each directory rename pair
++ * possibility occurs.
++ */
++ dir_renames = xmalloc(sizeof(struct hashmap));
+ dir_rename_init(dir_renames);
+ for (i = 0; i < pairs->nr; ++i) {
+ struct string_list_item *item;
@@ -114,6 +184,15 @@
+ *count += 1;
+ }
+
++ /*
++ * For each directory with files moved out of it, we find out which
++ * target directory received the most files so we can declare it to
++ * be the "winning" target location for the directory rename. This
++ * winner gets recorded in new_dir. If there is no winner
++ * (multiple target directories received the same number of files),
++ * we set non_unique_new_dir. Once we've determined the winner (or
++ * that there is no winner), we no longer need possible_new_dirs.
++ */
+ hashmap_iter_init(dir_renames, &iter);
+ while ((entry = hashmap_iter_next(&iter))) {
+ int max = 0;
@@ -136,8 +215,13 @@
+ assert(entry->new_dir.len == 0);
+ strbuf_addstr(&entry->new_dir, best);
+ }
-+ /* Strings were xstrndup'ed before inserting into string-list,
-+ * so ask string_list to remove the entries for us.
++ /*
++ * The relevant directory sub-portion of the original full
++ * filepaths were xstrndup'ed before inserting into
++ * possible_new_dirs, and instead of manually iterating the
++ * list and free'ing each, just lie and tell
++ * possible_new_dirs that it did the strdup'ing so that it
++ * will free them for us.
+ */
+ entry->possible_new_dirs.strdup_strings = 1;
+ string_list_clear(&entry->possible_new_dirs, 1);
@@ -201,3 +285,32 @@
return clean;
}
+
+diff --git a/merge-recursive.h b/merge-recursive.h
+--- a/merge-recursive.h
++++ b/merge-recursive.h
+@@
+ struct string_list df_conflict_file_set;
+ };
+
++/*
++ * For dir_rename_entry, directory names are stored as a full path from the
++ * toplevel of the repository and do not include a trailing '/'. Also:
++ *
++ * dir: original name of directory being renamed
++ * non_unique_new_dir: if true, could not determine new_dir
++ * new_dir: final name of directory being renamed
++ * possible_new_dirs: temporary used to help determine new_dir; see comments
++ * in get_directory_renames() for details
++ */
++struct dir_rename_entry {
++ struct hashmap_entry ent; /* must be the first member! */
++ char *dir;
++ unsigned non_unique_new_dir:1;
++ struct strbuf new_dir;
++ struct string_list possible_new_dirs;
++};
++
+ /* merge_trees() but with recursive ancestor consolidation */
+ int merge_recursive(struct merge_options *o,
+ struct commit *h1,
20: 6730d8e7b7 ! 19: 4f36512a02 merge-recursive: check for directory level conflicts
@@ -7,6 +7,7 @@
directory level. There will be additional checks at the individual
file level too, which will be added later.
+ Reviewed-by: Stefan Beller [off-list ref]
Signed-off-by: Elijah Newren [off-list ref]
diff --git a/merge-recursive.c b/merge-recursive.c
@@ -18,7 +19,7 @@
+static int tree_has_path(struct tree *tree, const char *path)
+{
-+ unsigned char hashy[20];
++ unsigned char hashy[GIT_MAX_RAWSZ];
+ unsigned int mode_o;
+
+ return !get_tree_entry(tree->object.oid.hash, path,
21: 178ec9e079 < --: ------- merge-recursive: add a new hashmap for storing file collisions
22: 1f3ff65e82 ! 20: 4a9098fba5 merge-recursive: add computation of collisions due to dir rename & merging
@@ -7,11 +7,42 @@
the same (otherwise vacant) location. Add checking and reporting for such
cases, falling back to no-directory-rename handling for such paths.
+ Reviewed-by: Stefan Beller [off-list ref]
Signed-off-by: Elijah Newren [off-list ref]
diff --git a/merge-recursive.c b/merge-recursive.c
--- a/merge-recursive.c
+++ b/merge-recursive.c
+@@
+ string_list_init(&entry->possible_new_dirs, 0);
+ }
+
++static struct collision_entry *collision_find_entry(struct hashmap *hashmap,
++ char *target_file)
++{
++ struct collision_entry key;
++
++ hashmap_entry_init(&key, strhash(target_file));
++ key.target_file = target_file;
++ return hashmap_get(hashmap, &key, NULL);
++}
++
++static int collision_cmp(void *unused_cmp_data,
++ const struct collision_entry *e1,
++ const struct collision_entry *e2,
++ const void *unused_keydata)
++{
++ return strcmp(e1->target_file, e2->target_file);
++}
++
++static void collision_init(struct hashmap *map)
++{
++ hashmap_init(map, (hashmap_cmp_fn) collision_cmp, NULL, 0);
++}
++
+ static void flush_output(struct merge_options *o)
+ {
+ if (o->buffer_output < 2 && o->obuf.len) {
@@
hashy, &mode_o);
}
@@ -179,3 +210,21 @@
common, head, merge, entries);
clean = process_renames(o, ri->head_renames, ri->merge_renames);
+
+diff --git a/merge-recursive.h b/merge-recursive.h
+--- a/merge-recursive.h
++++ b/merge-recursive.h
+@@
+ struct string_list possible_new_dirs;
+ };
+
++struct collision_entry {
++ struct hashmap_entry ent; /* must be the first member! */
++ char *target_file;
++ struct string_list source_files;
++ unsigned reported_already:1;
++};
++
+ /* merge_trees() but with recursive ancestor consolidation */
+ int merge_recursive(struct merge_options *o,
+ struct commit *h1,
23: d28651aeb0 ! 21: fd9129379f merge-recursive: check for file level conflicts then get new name
@@ -7,6 +7,7 @@
file level either. If there aren't any, then get the new name from
any directory renames.
+ Reviewed-by: Stefan Beller [off-list ref]
Signed-off-by: Elijah Newren [off-list ref]
diff --git a/merge-recursive.c b/merge-recursive.c
24: d6f3d47304 ! 22: 94eaf30851 merge-recursive: when comparing files, don't include trees
@@ -15,6 +15,7 @@
for a given path on the different sides of the merge, so create a
get_tree_entry_if_blob() helper function and use it.
+ Reviewed-by: Stefan Beller [off-list ref]
Signed-off-by: Elijah Newren [off-list ref]
diff --git a/merge-recursive.c b/merge-recursive.c
25: f91509f9df ! 23: 389b0d6bda merge-recursive: apply necessary modifications for directory renames
@@ -6,6 +6,7 @@
necessary changes to the rename struct, it's dst_entry, and the
diff_filepair under consideration.
+ Reviewed-by: Stefan Beller [off-list ref]
Signed-off-by: Elijah Newren [off-list ref]
diff --git a/merge-recursive.c b/merge-recursive.c
26: 9d903c98de ! 24: 5a5f25c6e0 merge-recursive: avoid clobbering untracked files with directory renames
@@ -2,6 +2,7 @@
merge-recursive: avoid clobbering untracked files with directory renames
+ Reviewed-by: Stefan Beller [off-list ref]
Signed-off-by: Elijah Newren [off-list ref]
diff --git a/merge-recursive.c b/merge-recursive.c
27: 2ab61d26a3 ! 25: 45819be1f8 merge-recursive: fix overwriting dirty files involved in renames
@@ -5,9 +5,10 @@
This fixes an issue that existed before my directory rename detection
patches that affects both normal renames and renames implied by
directory rename detection. Additional codepaths that only affect
- overwriting of directy files that are involved in directory rename
+ overwriting of dirty files that are involved in directory rename
detection will be added in a subsequent commit.
+ Reviewed-by: Stefan Beller [off-list ref]
Signed-off-by: Elijah Newren [off-list ref]
diff --git a/merge-recursive.c b/merge-recursive.c
@@ -186,7 +187,7 @@
+ struct unpack_trees_options unpack_opts;
};
- struct dir_rename_entry {
+ /*
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
--- a/t/t3501-revert-cherry-pick.sh
28: d510c260b7 ! 26: b840086726 merge-recursive: fix remaining directory rename + dirty overwrite cases
@@ -2,6 +2,7 @@
merge-recursive: fix remaining directory rename + dirty overwrite cases
+ Reviewed-by: Stefan Beller [off-list ref]
Signed-off-by: Elijah Newren [off-list ref]
diff --git a/merge-recursive.c b/merge-recursive.c
@@ -22,8 +23,7 @@
+ rename->path);
+ }
+ /*
-+ * Stupid double negatives in remove_file; it somehow manages
-+ * to repeatedly mess me up. So, just for myself:
++ * Because the double negatives somehow keep confusing me...
+ * 1) update_wd iff !ren_src_was_dirty.
+ * 2) no_wd iff !update_wd
+ * 3) so, no_wd == !!ren_src_was_dirty == ren_src_was_dirty
29: b59a612e68 ! 27: d320f88ef3 directory rename detection: new testcases showcasing a pair of bugs
@@ -12,6 +12,7 @@
testcases that showed existing bugs in order to make sure we aren't
merely addressing problems in isolation but in general.
+ Reviewed-by: Stefan Beller [off-list ref]
Signed-off-by: Elijah Newren [off-list ref]
diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh
30: d20b759b63 ! 28: e6574d990c merge-recursive: avoid spurious rename/rename conflict from dir renames
@@ -11,6 +11,7 @@
previously reported as a rename/delete conflict will now be reported as a
modify/delete conflict.
+ Reviewed-by: Stefan Beller [off-list ref]
Signed-off-by: Elijah Newren [off-list ref]
diff --git a/merge-recursive.c b/merge-recursive.c
31: f69932adfe ! 29: 32446f2578 merge-recursive: ensure we write updates for directory-renamed file
@@ -10,6 +10,7 @@
Update the code that checks whether we can skip the update to also work in
the presence of directory renames.
+ Reviewed-by: Stefan Beller [off-list ref]
Signed-off-by: Elijah Newren [off-list ref]
diff --git a/merge-recursive.c b/merge-recursive.c
--
2.16.1.232.g28d5be9217
@@ -439,4 +439,147 @@ test_expect_failure '1f-check: Split a directory into two other directories' '# in section 2, plus testcases 3a and 4a.###########################################################################++###########################################################################+# SECTION 2: Split into multiple directories, with equal number of paths+#+# Explore the splitting-a-directory rules a bit; what happens in the+# edge cases?+#+# Note that there is a closely related case of a directory not being+# split on either side of history, but being renamed differently on+# each side. See testcase 8e for that.+###########################################################################++# Testcase 2a, Directory split into two on one side, with equal numbers of paths+# Commit O: z/{b,c}+# Commit A: y/b, w/c+# Commit B: z/{b,c,d}+# Expected: y/b, w/c, z/d, with warning about z/ -> (y/ vs. w/) conflict+test_expect_success'2a-setup: Directory split into two on one side, with equal numbers of paths''+test_create_repo2a&&+(+cd2a&&++mkdirz&&+echob>z/b&&+echoc>z/c&&+gitaddz&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+mkdiry&&+mkdirw&&+gitmvz/by/&&+gitmvz/cw/&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+echod>z/d&&+gitaddz/d&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_failure'2a-check: Directory split into two on one side, with equal numbers of paths''+(+cd2a&&++gitcheckoutA^0&&++test_must_failgitmerge-srecursiveB^0>out&&+test_i18ngrep"CONFLICT.*directory rename split"out&&++gitls-files-s>out&&+test_line_count=3out&&+gitls-files-u>out&&+test_line_count=0out&&+gitls-files-o>out&&+test_line_count=1out&&++gitrev-parse>actual\+:0:y/b:0:w/c:0:z/d&&+gitrev-parse>expect\+O:z/bO:z/cB:z/d&&+test_cmpexpectactual+)+'++# Testcase 2b, Directory split into two on one side, with equal numbers of paths+# Commit O: z/{b,c}+# Commit A: y/b, w/c+# Commit B: z/{b,c}, x/d+# Expected: y/b, w/c, x/d; No warning about z/ -> (y/ vs. w/) conflict+test_expect_success'2b-setup: Directory split into two on one side, with equal numbers of paths''+test_create_repo2b&&+(+cd2b&&++mkdirz&&+echob>z/b&&+echoc>z/c&&+gitaddz&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+mkdiry&&+mkdirw&&+gitmvz/by/&&+gitmvz/cw/&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+mkdirx&&+echod>x/d&&+gitaddx/d&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_success'2b-check: Directory split into two on one side, with equal numbers of paths''+(+cd2b&&++gitcheckoutA^0&&++gitmerge-srecursiveB^0>out&&++gitls-files-s>out&&+test_line_count=3out&&+gitls-files-u>out&&+test_line_count=0out&&+gitls-files-o>out&&+test_line_count=1out&&++gitrev-parse>actual\+:0:y/b:0:w/c:0:x/d&&+gitrev-parse>expect\+O:z/bO:z/cB:x/d&&+test_cmpexpectactual&&+test_i18ngrep!"CONFLICT.*directory rename split"out+)+'++###########################################################################+# Rules suggested by section 2:+#+# None; the rule was already covered in section 1. These testcases are+# here just to make sure the conflict resolution and necessary warning+# messages are handled correctly.+###########################################################################+ test_done
In anticipation of more involved cleanup to come, make a helper function
for doing the cleanup at the end of handle_renames. Rename the already
existing cleanup_rename[s]() to final_cleanup_rename[s](), name the new
helper initial_cleanup_rename(), and leave the big comment in the code
about why we can't do all the cleanup at once.
Reviewed-by: Stefan Beller <redacted>
Signed-off-by: Elijah Newren <redacted>
---
merge-recursive.c | 23 +++++++++++++----------
1 file changed, 13 insertions(+), 10 deletions(-)
@@ -1323,11 +1323,22 @@ static int handle_file(struct merge_options *o,add=filespec_from_entry(&other,dst_entry,stage^1);if(add){+intren_src_was_dirty=was_dirty(o,rename->path);char*add_name=unique_path(o,rename->path,other_branch);if(update_file(o,0,&add->oid,add->mode,add_name))return-1;-remove_file(o,0,rename->path,0);+if(ren_src_was_dirty){+output(o,1,_("Refusing to lose dirty file at %s"),+rename->path);+}+/*+*Becausethedoublenegativessomehowkeepconfusingme...+*1)update_wdiff!ren_src_was_dirty.+*2)no_wdiff!update_wd+*3)so,no_wd==!!ren_src_was_dirty==ren_src_was_dirty+*/+remove_file(o,0,rename->path,ren_src_was_dirty);dst_name=unique_path(o,rename->path,cur_branch);}else{if(dir_in_way(rename->path,!o->call_depth,0)){
@@ -1465,7 +1476,10 @@ static int conflict_rename_rename_2to1(struct merge_options *o,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(would_lose_untracked(path))+if(was_dirty(o,path))+output(o,1,_("Refusing to lose dirty file at %s"),+path);+elseif(would_lose_untracked(path))/**Onlywaywegethereisifbothrenameswerefrom*adirectoryrenameANDuserhadanuntrackedfile
@@ -2084,7 +2099,11 @@ static void apply_directory_rename_modifications(struct merge_options *o,*sayingthefilewouldhavebeenoverwritten),butitmight*bedirty,though.*/-remove_file(o,1,pair->two->path,0/* no_wd */);+update_wd=!was_dirty(o,pair->two->path);+if(!update_wd)+output(o,1,_("Refusing to lose dirty file at %s"),+pair->two->path);+remove_file(o,1,pair->two->path,!update_wd);/* Find or create a new re->dst_entry */item=string_list_lookup(entries,new_path);
@@ -3370,7 +3370,7 @@ test_expect_success '11b-setup: Avoid losing dirty file involved in directory re)'-test_expect_failure'11b-check: Avoid losing dirty file involved in directory rename''+test_expect_success'11b-check: Avoid losing dirty file involved in directory rename''(cd11b&&
@@ -3512,7 +3512,7 @@ test_expect_success '11d-setup: Avoid losing not-uptodate with rename + D/F conf)'-test_expect_failure'11d-check: Avoid losing not-uptodate with rename + D/F conflict''+test_expect_success'11d-check: Avoid losing not-uptodate with rename + D/F conflict''(cd11d&&
@@ -3591,7 +3591,7 @@ test_expect_success '11e-setup: Avoid deleting not-uptodate with dir rename/rena)'-test_expect_failure'11e-check: Avoid deleting not-uptodate with dir rename/rename(1to2)/add''+test_expect_success'11e-check: Avoid deleting not-uptodate with dir rename/rename(1to2)/add''(cd11e&&
@@ -3667,7 +3667,7 @@ test_expect_success '11f-setup: Avoid deleting not-uptodate with dir rename/rena)'-test_expect_failure'11f-check: Avoid deleting not-uptodate with dir rename/rename(2to1)''+test_expect_success'11f-check: Avoid deleting not-uptodate with dir rename/rename(2to1)''(cd11f&&
Before trying to apply directory renames to paths within the given
directories, we want to make sure that there aren't conflicts at the
directory level. There will be additional checks at the individual
file level too, which will be added later.
Reviewed-by: Stefan Beller <redacted>
Signed-off-by: Elijah Newren <redacted>
---
merge-recursive.c | 119 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 119 insertions(+)
@@ -1449,6 +1458,112 @@ static void get_renamed_dir_portion(const char *old_path, const char *new_path,}}+staticvoidremove_hashmap_entries(structhashmap*dir_renames,+structstring_list*items_to_remove)+{+inti;+structdir_rename_entry*entry;++for(i=0;i<items_to_remove->nr;i++){+entry=items_to_remove->items[i].util;+hashmap_remove(dir_renames,entry,NULL);+}+string_list_clear(items_to_remove,0);+}++/*+*Thereareacouplethingswewanttodoatthedirectorylevel:+*1.Checkforbothsidesrenamingtothesamething,inordertoavoid+*implicitrenamingoffilesthatshouldbeleftinplace.(See+*testcase6bint6043fordetails.)+*2.Prunedirectoryrenamesiftherearestillfilesleftinthe+*theoriginaldirectory.Theserepresentapartialdirectoryrename,+*i.e.arenamewhereonlysomeofthefileswithinthedirectory+*wererenamedelsewhere.(Technically,thiscouldbedoneearlier+*inget_directory_renames(),exceptthatwouldpreventusfrom+*doingthepreviouscheckandthusfailingtestcase6b.)+*3.Checkforrename/rename(1to2)conflicts(atthedirectorylevel).+*Inthefuture,wecouldpotentiallyrecordthisinfoaswelland+*omitreportingrename/rename(1to2)conflictsforeachpathwithin+*theaffecteddirectories,thuscleaningupthemergeoutput.+*NOTE:WedoNOTcheckforrename/rename(2to1)conflictsatthe+*directorylevel,becausemergingdirectoriesisfine.Ifit+*causesconflictsforfileswithinthosemergeddirectories,then+*thatshouldbedetectedattheindividualpathlevel.+*/+staticvoidhandle_directory_level_conflicts(structmerge_options*o,+structhashmap*dir_re_head,+structtree*head,+structhashmap*dir_re_merge,+structtree*merge)+{+structhashmap_iteriter;+structdir_rename_entry*head_ent;+structdir_rename_entry*merge_ent;++structstring_listremove_from_head=STRING_LIST_INIT_NODUP;+structstring_listremove_from_merge=STRING_LIST_INIT_NODUP;++hashmap_iter_init(dir_re_head,&iter);+while((head_ent=hashmap_iter_next(&iter))){+merge_ent=dir_rename_find_entry(dir_re_merge,head_ent->dir);+if(merge_ent&&+!head_ent->non_unique_new_dir&&+!merge_ent->non_unique_new_dir&&+!strbuf_cmp(&head_ent->new_dir,&merge_ent->new_dir)){+/* 1. Renamed identically; remove it from both sides */+string_list_append(&remove_from_head,+head_ent->dir)->util=head_ent;+strbuf_release(&head_ent->new_dir);+string_list_append(&remove_from_merge,+merge_ent->dir)->util=merge_ent;+strbuf_release(&merge_ent->new_dir);+}elseif(tree_has_path(head,head_ent->dir)){+/* 2. This wasn't a directory rename after all */+string_list_append(&remove_from_head,+head_ent->dir)->util=head_ent;+strbuf_release(&head_ent->new_dir);+}+}++remove_hashmap_entries(dir_re_head,&remove_from_head);+remove_hashmap_entries(dir_re_merge,&remove_from_merge);++hashmap_iter_init(dir_re_merge,&iter);+while((merge_ent=hashmap_iter_next(&iter))){+head_ent=dir_rename_find_entry(dir_re_head,merge_ent->dir);+if(tree_has_path(merge,merge_ent->dir)){+/* 2. This wasn't a directory rename after all */+string_list_append(&remove_from_merge,+merge_ent->dir)->util=merge_ent;+}elseif(head_ent&&+!head_ent->non_unique_new_dir&&+!merge_ent->non_unique_new_dir){+/* 3. rename/rename(1to2) */+/*+*Wecanassumeit'snotrename/rename(1to1)because+*thatwascase(1),alreadycheckedabove.Sowe+*knowthathead_ent->new_dirandmerge_ent->new_dir+*aredifferentstrings.+*/+output(o,1,_("CONFLICT (rename/rename): "+"Rename directory %s->%s in %s. "+"Rename directory %s->%s in %s"),+head_ent->dir,head_ent->new_dir.buf,o->branch1,+head_ent->dir,merge_ent->new_dir.buf,o->branch2);+string_list_append(&remove_from_head,+head_ent->dir)->util=head_ent;+strbuf_release(&head_ent->new_dir);+string_list_append(&remove_from_merge,+merge_ent->dir)->util=merge_ent;+strbuf_release(&merge_ent->new_dir);+}+}++remove_hashmap_entries(dir_re_head,&remove_from_head);+remove_hashmap_entries(dir_re_merge,&remove_from_merge);+}+staticstructhashmap*get_directory_renames(structdiff_queue_struct*pairs,structtree*tree){
@@ -1910,6 +2025,10 @@ static int handle_renames(struct merge_options *o,dir_re_head=get_directory_renames(head_pairs,head);dir_re_merge=get_directory_renames(merge_pairs,merge);+handle_directory_level_conflicts(o,+dir_re_head,head,+dir_re_merge,merge);+ri->head_renames=get_renames(o,head_pairs,head,common,head,merge,entries);ri->merge_renames=get_renames(o,merge_pairs,merge,
This populates a set of directory renames for us. The set of directory
renames is not yet used, but will be in subsequent commits.
Note that the use of a string_list for possible_new_dirs in the new
dir_rename_entry struct implies an O(n^2) algorithm; however, in practice
I expect the number of distinct directories that files were renamed into
from a single original directory to be O(1). My guess is that n has a
mode of 1 and a mean of less than 2, so, for now, string_list seems good
enough for possible_new_dirs.
Reviewed-by: Stefan Beller <redacted>
Signed-off-by: Elijah Newren <redacted>
---
merge-recursive.c | 224 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
merge-recursive.h | 18 +++++
2 files changed, 239 insertions(+), 3 deletions(-)
@@ -49,6 +49,44 @@ static unsigned int path_hash(const char *path)returnignore_case?strihash(path):strhash(path);}+staticstructdir_rename_entry*dir_rename_find_entry(structhashmap*hashmap,+char*dir)+{+structdir_rename_entrykey;++if(dir==NULL)+returnNULL;+hashmap_entry_init(&key,strhash(dir));+key.dir=dir;+returnhashmap_get(hashmap,&key,NULL);+}++staticintdir_rename_cmp(constvoid*unused_cmp_data,+constvoid*entry,+constvoid*entry_or_key,+constvoid*unused_keydata)+{+conststructdir_rename_entry*e1=entry;+conststructdir_rename_entry*e2=entry_or_key;++returnstrcmp(e1->dir,e2->dir);+}++staticvoiddir_rename_init(structhashmap*map)+{+hashmap_init(map,dir_rename_cmp,NULL,0);+}++staticvoiddir_rename_entry_init(structdir_rename_entry*entry,+char*directory)+{+hashmap_entry_init(entry,strhash(directory));+entry->dir=directory;+entry->non_unique_new_dir=0;+strbuf_init(&entry->new_dir,0);+string_list_init(&entry->possible_new_dirs,0);+}+staticvoidflush_output(structmerge_options*o){if(o->buffer_output<2&&o->obuf.len){
@@ -1356,6 +1394,169 @@ static struct diff_queue_struct *get_diffpairs(struct merge_options *o,returnret;}+staticvoidget_renamed_dir_portion(constchar*old_path,constchar*new_path,+char**old_dir,char**new_dir)+{+char*end_of_old,*end_of_new;+intold_len,new_len;++*old_dir=NULL;+*new_dir=NULL;++/*+*For+*"a/b/c/d/e/foo.c"->"a/b/some/thing/else/e/foo.c"+*the"e/foo.c"partisthesame,wejustwanttoknowthat+*"a/b/c/d"wasrenamedto"a/b/some/thing/else"+*so,forthisexample,thisfunctionreturns"a/b/c/d"in+**old_dirand"a/b/some/thing/else"in*new_dir.+*+*Also,ifthebasenameofthefilechanged,wedon'tcare.We+*wanttoknowwhichportionofthedirectory,ifany,changed.+*/+end_of_old=strrchr(old_path,'/');+end_of_new=strrchr(new_path,'/');++if(end_of_old==NULL||end_of_new==NULL)+return;+while(*--end_of_new==*--end_of_old&&+end_of_old!=old_path&&+end_of_new!=new_path)+;/* Do nothing; all in the while loop */+/*+*We'vefoundthefirstnon-matchingcharacterinthedirectory+*paths.Thatmeansthecurrentdirectorywewerecomparing+*representstherename.Moveend_of_oldandend_of_newback+*tothefulldirectoryname.+*/+if(*end_of_old=='/')+end_of_old++;+if(*end_of_old!='/')+end_of_new++;+end_of_old=strchr(end_of_old,'/');+end_of_new=strchr(end_of_new,'/');++/*+*Itmayhavebeenthecasethatold_pathandnew_pathwerethesame+*directoryallalong.Don'tclaimarenameifthey'rethesame.+*/+old_len=end_of_old-old_path;+new_len=end_of_new-new_path;++if(old_len!=new_len||strncmp(old_path,new_path,old_len)){+*old_dir=xstrndup(old_path,old_len);+*new_dir=xstrndup(new_path,new_len);+}+}++staticstructhashmap*get_directory_renames(structdiff_queue_struct*pairs,+structtree*tree)+{+structhashmap*dir_renames;+structhashmap_iteriter;+structdir_rename_entry*entry;+inti;++/*+*Typically,wethinkofadirectoryrenameasallfilesfroma+*certaindirectorybeingmovedtoatargetdirectory.However,+*whatifsomeonefirstmovedtwofilesfromtheoriginal+*directoryinonecommit,andthenrenamedthedirectory+*somewhereelseinalatercommit?Atmergetime,wejustknow+*thatfilesfromtheoriginaldirectorywenttotwodifferent+*places,andthatthebulkofthemendedupinthesameplace.+*Wewanteachdirectoryrenametorepresentwherethebulkofthe+*filesfromthatdirectoryendup;thisfunctionexiststofind+*wherethebulkofthefileswent.+*+*Thefirstloopbelowsimplyiteratesthroughthelistoffile+*renames,findingouthowofteneachdirectoryrenamepair+*possibilityoccurs.+*/+dir_renames=xmalloc(sizeof(structhashmap));+dir_rename_init(dir_renames);+for(i=0;i<pairs->nr;++i){+structstring_list_item*item;+int*count;+structdiff_filepair*pair=pairs->queue[i];+char*old_dir,*new_dir;++/* File not part of directory rename if it wasn't renamed */+if(pair->status!='R')+continue;++get_renamed_dir_portion(pair->one->path,pair->two->path,+&old_dir,&new_dir);+if(!old_dir)+/* Directory didn't change at all; ignore this one. */+continue;++entry=dir_rename_find_entry(dir_renames,old_dir);+if(!entry){+entry=xmalloc(sizeof(structdir_rename_entry));+dir_rename_entry_init(entry,old_dir);+hashmap_put(dir_renames,entry);+}else{+free(old_dir);+}+item=string_list_lookup(&entry->possible_new_dirs,new_dir);+if(!item){+item=string_list_insert(&entry->possible_new_dirs,+new_dir);+item->util=xcalloc(1,sizeof(int));+}else{+free(new_dir);+}+count=item->util;+*count+=1;+}++/*+*Foreachdirectorywithfilesmovedoutofit,wefindoutwhich+*targetdirectoryreceivedthemostfilessowecandeclareitto+*bethe"winning"targetlocationforthedirectoryrename.This+*winnergetsrecordedinnew_dir.Ifthereisnowinner+*(multipletargetdirectoriesreceivedthesamenumberoffiles),+*wesetnon_unique_new_dir.Oncewe'vedeterminedthewinner(or+*thatthereisnowinner),wenolongerneedpossible_new_dirs.+*/+hashmap_iter_init(dir_renames,&iter);+while((entry=hashmap_iter_next(&iter))){+intmax=0;+intbad_max=0;+char*best=NULL;++for(i=0;i<entry->possible_new_dirs.nr;i++){+int*count=entry->possible_new_dirs.items[i].util;++if(*count==max)+bad_max=max;+elseif(*count>max){+max=*count;+best=entry->possible_new_dirs.items[i].string;+}+}+if(bad_max==max)+entry->non_unique_new_dir=1;+else{+assert(entry->new_dir.len==0);+strbuf_addstr(&entry->new_dir,best);+}+/*+*Therelevantdirectorysub-portionoftheoriginalfull+*filepathswerexstrndup'edbeforeinsertinginto+*possible_new_dirs,andinsteadofmanuallyiteratingthe+*listandfree'ingeach,justlieandtell+*possible_new_dirsthatitdidthestrdup'ingsothatit+*willfreethemforus.+*/+entry->possible_new_dirs.strdup_strings=1;+string_list_clear(&entry->possible_new_dirs,1);+}++returndir_renames;+}+/**Getinformationofallrenameswhichoccurredin'pairs',makinguseof*anyimplicitdirectoryrenamesinferredfromtheothersideofhistory.
@@ -29,6 +29,24 @@ struct merge_options {structstring_listdf_conflict_file_set;};+/*+*Fordir_rename_entry,directorynamesarestoredasafullpathfromthe+*topleveloftherepositoryanddonotincludeatrailing'/'.Also:+*+*dir:originalnameofdirectorybeingrenamed+*non_unique_new_dir:iftrue,couldnotdeterminenew_dir+*new_dir:finalnameofdirectorybeingrenamed+*possible_new_dirs:temporaryusedtohelpdeterminenew_dir;seecomments+*inget_directory_renames()fordetails+*/+structdir_rename_entry{+structhashmap_entryent;/* must be the first member! */+char*dir;+unsignednon_unique_new_dir:1;+structstrbufnew_dir;+structstring_listpossible_new_dirs;+};+/* merge_trees() but with recursive ancestor consolidation */intmerge_recursive(structmerge_options*o,structcommit*h1,
@@ -850,4 +850,334 @@ test_expect_success '4a-check: Directory split, with original directory still pr# detection.) But, sadly, see testcase 8b.###########################################################################++###########################################################################+# SECTION 5: Files/directories in the way of subset of to-be-renamed paths+#+# Implicitly renaming files due to a detected directory rename could run+# into problems if there are files or directories in the way of the paths+# we want to rename. Explore such cases in this section.+###########################################################################++# Testcase 5a, Merge directories, other side adds files to original and target+# Commit O: z/{b,c}, y/d+# Commit A: z/{b,c,e_1,f}, y/{d,e_2}+# Commit B: y/{b,c,d}+# Expected: z/e_1, y/{b,c,d,e_2,f} + CONFLICT warning+# NOTE: While directory rename detection is active here causing z/f to+# become y/f, we did not apply this for z/e_1 because that would+# give us an add/add conflict for y/e_1 vs y/e_2. This problem with+# this add/add, is that both versions of y/e are from the same side+# of history, giving us no way to represent this conflict in the+# index.++test_expect_success'5a-setup: Merge directories, other side adds files to original and target''+test_create_repo5a&&+(+cd5a&&++mkdirz&&+echob>z/b&&+echoc>z/c&&+mkdiry&&+echod>y/d&&+gitaddzy&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+echoe1>z/e&&+echof>z/f&&+echoe2>y/e&&+gitaddz/ez/fy/e&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+gitmvz/by/&&+gitmvz/cy/&&+rmdirz&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_failure'5a-check: Merge directories, other side adds files to original and target''+(+cd5a&&++gitcheckoutA^0&&++test_must_failgitmerge-srecursiveB^0>out&&+test_i18ngrep"CONFLICT.*implicit dir rename"out&&++gitls-files-s>out&&+test_line_count=6out&&+gitls-files-u>out&&+test_line_count=0out&&+gitls-files-o>out&&+test_line_count=1out&&++gitrev-parse>actual\+:0:y/b:0:y/c:0:y/d:0:y/e:0:z/e:0:y/f&&+gitrev-parse>expect\+O:z/bO:z/cO:y/dA:y/eA:z/eA:z/f&&+test_cmpexpectactual+)+'++# Testcase 5b, Rename/delete in order to get add/add/add conflict+# (Related to testcase 8d; these may appear slightly inconsistent to users;+# Also related to testcases 7d and 7e)+# Commit O: z/{b,c,d_1}+# Commit A: y/{b,c,d_2}+# Commit B: z/{b,c,d_1,e}, y/d_3+# Expected: y/{b,c,e}, CONFLICT(add/add: y/d_2 vs. y/d_3)+# NOTE: If z/d_1 in commit B were to be involved in dir rename detection, as+# we normaly would since z/ is being renamed to y/, then this would be+# a rename/delete (z/d_1 -> y/d_1 vs. deleted) AND an add/add/add+# conflict of y/d_1 vs. y/d_2 vs. y/d_3. Add/add/add is not+# representable in the index, so the existence of y/d_3 needs to+# cause us to bail on directory rename detection for that path, falling+# back to git behavior without the directory rename detection.++test_expect_success'5b-setup: Rename/delete in order to get add/add/add conflict''+test_create_repo5b&&+(+cd5b&&++mkdirz&&+echob>z/b&&+echoc>z/c&&+echod1>z/d&&+gitaddz&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+gitrmz/d&&+gitmvzy&&+echod2>y/d&&+gitaddy/d&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+mkdiry&&+echod3>y/d&&+echoe>z/e&&+gitaddy/dz/e&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_failure'5b-check: Rename/delete in order to get add/add/add conflict''+(+cd5b&&++gitcheckoutA^0&&++test_must_failgitmerge-srecursiveB^0>out&&+test_i18ngrep"CONFLICT (add/add).* y/d"out&&++gitls-files-s>out&&+test_line_count=5out&&+gitls-files-u>out&&+test_line_count=2out&&+gitls-files-o>out&&+test_line_count=1out&&++gitrev-parse>actual\+:0:y/b:0:y/c:0:y/e:2:y/d:3:y/d&&+gitrev-parse>expect\+O:z/bO:z/cB:z/eA:y/dB:y/d&&+test_cmpexpectactual&&++test_must_failgitrev-parse:1:y/d&&+test_path_is_filey/d+)+'++# Testcase 5c, Transitive rename would cause rename/rename/rename/add/add/add+# (Directory rename detection would result in transitive rename vs.+# rename/rename(1to2) and turn it into a rename/rename(1to3). Further,+# rename paths conflict with separate adds on the other side)+# (Related to testcases 3b and 7c)+# Commit O: z/{b,c}, x/d_1+# Commit A: y/{b,c,d_2}, w/d_1+# Commit B: z/{b,c,d_1,e}, w/d_3, y/d_4+# Expected: A mess, but only a rename/rename(1to2)/add/add mess. Use the+# presence of y/d_4 in B to avoid doing transitive rename of+# x/d_1 -> z/d_1 -> y/d_1, so that the only paths we have at+# y/d are y/d_2 and y/d_4. We still do the move from z/e to y/e,+# though, because it doesn't have anything in the way.++test_expect_success'5c-setup: Transitive rename would cause rename/rename/rename/add/add/add''+test_create_repo5c&&+(+cd5c&&++mkdirz&&+echob>z/b&&+echoc>z/c&&+mkdirx&&+echod1>x/d&&+gitaddzx&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+gitmvzy&&+echod2>y/d&&+gitaddy/d&&+gitmvxw&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+gitmvx/dz/&&+mkdirw&&+mkdiry&&+echod3>w/d&&+echod4>y/d&&+echoe>z/e&&+gitaddw/y/z/e&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_failure'5c-check: Transitive rename would cause rename/rename/rename/add/add/add''+(+cd5c&&++gitcheckoutA^0&&++test_must_failgitmerge-srecursiveB^0>out&&+test_i18ngrep"CONFLICT (rename/rename).*x/d.*w/d.*z/d"out&&+test_i18ngrep"CONFLICT (add/add).* y/d"out&&++gitls-files-s>out&&+test_line_count=9out&&+gitls-files-u>out&&+test_line_count=6out&&+gitls-files-o>out&&+test_line_count=3out&&++gitrev-parse>actual\+:0:y/b:0:y/c:0:y/e&&+gitrev-parse>expect\+O:z/bO:z/cB:z/e&&+test_cmpexpectactual&&++test_must_failgitrev-parse:1:y/d&&+gitrev-parse>actual\+:2:w/d:3:w/d:1:x/d:2:y/d:3:y/d:3:z/d&&+gitrev-parse>expect\+O:x/dB:w/dO:x/dA:y/dB:y/dO:x/d&&+test_cmpexpectactual&&++githash-object>actual\+w/d~HEADw/d~B^0z/d&&+gitrev-parse>expect\+O:x/dB:w/dO:x/d&&+test_cmpexpectactual&&+test_path_is_missingx/d&&+test_path_is_filey/d&&+grep-q"<<<<"y/d# conflict markers should be present+)+'++# Testcase 5d, Directory/file/file conflict due to directory rename+# Commit O: z/{b,c}+# Commit A: y/{b,c,d_1}+# Commit B: z/{b,c,d_2,f}, y/d/e+# Expected: y/{b,c,d/e,f}, z/d_2, CONFLICT(file/directory), y/d_1~HEAD+# Note: The fact that y/d/ exists in B makes us bail on directory rename+# detection for z/d_2, but that doesn't prevent us from applying the+# directory rename detection for z/f -> y/f.++test_expect_success'5d-setup: Directory/file/file conflict due to directory rename''+test_create_repo5d&&+(+cd5d&&++mkdirz&&+echob>z/b&&+echoc>z/c&&+gitaddz&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+gitmvzy&&+echod1>y/d&&+gitaddy/d&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+mkdir-py/d&&+echoe>y/d/e&&+echod2>z/d&&+echof>z/f&&+gitaddy/d/ez/dz/f&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_failure'5d-check: Directory/file/file conflict due to directory rename''+(+cd5d&&++gitcheckoutA^0&&++test_must_failgitmerge-srecursiveB^0>out&&+test_i18ngrep"CONFLICT (file/directory).*y/d"out&&++gitls-files-s>out&&+test_line_count=6out&&+gitls-files-u>out&&+test_line_count=1out&&+gitls-files-o>out&&+test_line_count=2out&&++gitrev-parse>actual\+:0:y/b:0:y/c:0:z/d:0:y/f:2:y/d:0:y/d/e&&+gitrev-parse>expect\+O:z/bO:z/cB:z/dB:z/fA:y/dB:y/d/e&&+test_cmpexpectactual&&++githash-objecty/d~HEAD>actual&&+gitrev-parseA:y/d>expect&&+test_cmpexpectactual+)+'++###########################################################################+# Rules suggested by section 5:+#+# If a subset of to-be-renamed files have a file or directory in the way,+# "turn off" the directory rename for those specific sub-paths, falling+# back to old handling. But, sadly, see testcases 8a and 8b.+###########################################################################+ test_done
I came up with the testcases in the first eight sections before coding up
the implementation. The testcases in this section were mostly ones I
thought of while coding/debugging, and which I was too lazy to insert
into the previous sections because I didn't want to re-label with all the
testcase references. :-)
Reviewed-by: Stefan Beller <redacted>
Signed-off-by: Elijah Newren <redacted>
---
t/t6043-merge-rename-directories.sh | 565 +++++++++++++++++++++++++++++++++++-
1 file changed, 564 insertions(+), 1 deletion(-)
@@ -305,6 +305,7 @@ test_expect_failure '1d-check: Directory renames cause a rename/rename(2to1) con'# Testcase 1e, Renamed directory, with all filenames being renamed too+# (Related to testcases 9f & 9g)# Commit O: z/{oldb,oldc}# Commit A: y/{newb,newc}# Commit B: z/{oldb,oldc,d}
@@ -593,7 +594,7 @@ test_expect_success '2b-check: Directory split into two on one side, with equal############################################################################ Testcase 3a, Avoid implicit rename if involved as source on other side-# (Related to testcases 1c and 1f)+# (Related to testcases 1c, 1f, and 9h)# Commit O: z/{b,c,d}# Commit A: z/{b,c,d} (no change)# Commit B: y/{b,c}, x/d
@@ -2316,4 +2317,566 @@ test_expect_failure '8e-check: Both sides rename, one side adds to original dire)'+###########################################################################+# SECTION 9: Other testcases+#+# This section consists of miscellaneous testcases I thought of during+# the implementation which round out the testing.+###########################################################################++# Testcase 9a, Inner renamed directory within outer renamed directory+# (Related to testcase 1f)+# Commit O: z/{b,c,d/{e,f,g}}+# Commit A: y/{b,c}, x/w/{e,f,g}+# Commit B: z/{b,c,d/{e,f,g,h},i}+# Expected: y/{b,c,i}, x/w/{e,f,g,h}+# NOTE: The only reason this one is interesting is because when a directory+# is split into multiple other directories, we determine by the weight+# of which one had the most paths going to it. A naive implementation+# of that could take the new file in commit B at z/i to x/w/i or x/i.++test_expect_success'9a-setup: Inner renamed directory within outer renamed directory''+test_create_repo9a&&+(+cd9a&&++mkdir-pz/d&&+echob>z/b&&+echoc>z/c&&+echoe>z/d/e&&+echof>z/d/f&&+echog>z/d/g&&+gitaddz&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+mkdirx&&+gitmvz/dx/w&&+gitmvzy&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+echoh>z/d/h&&+echoi>z/i&&+gitaddz&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_failure'9a-check: Inner renamed directory within outer renamed directory''+(+cd9a&&++gitcheckoutA^0&&++gitmerge-srecursiveB^0&&++gitls-files-s>out&&+test_line_count=7out&&+gitls-files-u>out&&+test_line_count=0out&&+gitls-files-o>out&&+test_line_count=1out&&++gitrev-parse>actual\+HEAD:y/bHEAD:y/cHEAD:y/i&&+gitrev-parse>expect\+O:z/bO:z/cB:z/i&&+test_cmpexpectactual&&++gitrev-parse>actual\+HEAD:x/w/eHEAD:x/w/fHEAD:x/w/gHEAD:x/w/h&&+gitrev-parse>expect\+O:z/d/eO:z/d/fO:z/d/gB:z/d/h&&+test_cmpexpectactual+)+'++# Testcase 9b, Transitive rename with content merge+# (Related to testcase 1c)+# Commit O: z/{b,c}, x/d_1+# Commit A: y/{b,c}, x/d_2+# Commit B: z/{b,c,d_3}+# Expected: y/{b,c,d_merged}++test_expect_success'9b-setup: Transitive rename with content merge''+test_create_repo9b&&+(+cd9b&&++mkdirz&&+echob>z/b&&+echoc>z/c&&+mkdirx&&+test_seq110>x/d&&+gitaddzx&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+gitmvzy&&+test_seq111>x/d&&+gitaddx/d&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+test_seq010>x/d&&+gitmvx/dz/d&&+gitaddz/d&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_failure'9b-check: Transitive rename with content merge''+(+cd9b&&++gitcheckoutA^0&&++gitmerge-srecursiveB^0&&++gitls-files-s>out&&+test_line_count=3out&&++test_seq011>expected&&+test_cmpexpectedy/d&&+gitaddexpected&&+gitrev-parse>actual\+HEAD:y/bHEAD:y/cHEAD:y/d&&+gitrev-parse>expect\+O:z/bO:z/c:0:expected&&+test_cmpexpectactual&&+test_must_failgitrev-parseHEAD:x/d&&+test_must_failgitrev-parseHEAD:z/d&&+test_path_is_missingz/d&&++test$(gitrev-parseHEAD:y/d)!=$(gitrev-parseO:x/d)&&+test$(gitrev-parseHEAD:y/d)!=$(gitrev-parseA:x/d)&&+test$(gitrev-parseHEAD:y/d)!=$(gitrev-parseB:z/d)+)+'++# Testcase 9c, Doubly transitive rename?+# (Related to testcase 1c, 7e, and 9d)+# Commit O: z/{b,c}, x/{d,e}, w/f+# Commit A: y/{b,c}, x/{d,e,f,g}+# Commit B: z/{b,c,d,e}, w/f+# Expected: y/{b,c,d,e}, x/{f,g}+#+# NOTE: x/f and x/g may be slightly confusing here. The rename from w/f to+# x/f is clear. Let's look beyond that. Here's the logic:+# Commit B renamed x/ -> z/+# Commit A renamed z/ -> y/+# So, we could possibly further rename x/f to z/f to y/f, a doubly+# transient rename. However, where does it end? We can chain these+# indefinitely (see testcase 9d). What if there is a D/F conflict+# at z/f/ or y/f/? Or just another file conflict at one of those+# paths? In the case of an N-long chain of transient renamings,+# where do we "abort" the rename at? Can the user make sense of+# the resulting conflict and resolve it?+#+# To avoid this confusion I use the simple rule that if the other side+# of history did a directory rename to a path that your side renamed+# away, then ignore that particular rename from the other side of+# history for any implicit directory renames.++test_expect_success'9c-setup: Doubly transitive rename?''+test_create_repo9c&&+(+cd9c&&++mkdirz&&+echob>z/b&&+echoc>z/c&&+mkdirx&&+echod>x/d&&+echoe>x/e&&+mkdirw&&+echof>w/f&&+gitaddzxw&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+gitmvzy&&+gitmvw/fx/&&+echog>x/g&&+gitaddx/g&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+gitmvx/dz/d&&+gitmvx/ez/e&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_failure'9c-check: Doubly transitive rename?''+(+cd9c&&++gitcheckoutA^0&&++gitmerge-srecursiveB^0>out&&+test_i18ngrep"WARNING: Avoiding applying x -> z rename to x/f"out&&++gitls-files-s>out&&+test_line_count=6out&&+gitls-files-o>out&&+test_line_count=1out&&++gitrev-parse>actual\+HEAD:y/bHEAD:y/cHEAD:y/dHEAD:y/eHEAD:x/fHEAD:x/g&&+gitrev-parse>expect\+O:z/bO:z/cO:x/dO:x/eO:w/fA:x/g&&+test_cmpexpectactual+)+'++# Testcase 9d, N-fold transitive rename?+# (Related to testcase 9c...and 1c and 7e)+# Commit O: z/a, y/b, x/c, w/d, v/e, u/f+# Commit A: y/{a,b}, w/{c,d}, u/{e,f}+# Commit B: z/{a,t}, x/{b,c}, v/{d,e}, u/f+# Expected: <see NOTE first>+#+# NOTE: z/ -> y/ (in commit A)+# y/ -> x/ (in commit B)+# x/ -> w/ (in commit A)+# w/ -> v/ (in commit B)+# v/ -> u/ (in commit A)+# So, if we add a file to z, say z/t, where should it end up? In u?+# What if there's another file or directory named 't' in one of the+# intervening directories and/or in u itself? Also, shouldn't the+# same logic that places 't' in u/ also move ALL other files to u/?+# What if there are file or directory conflicts in any of them? If+# we attempted to do N-way (N-fold? N-ary? N-uple?) transitive renames+# like this, would the user have any hope of understanding any+# conflicts or how their working tree ended up? I think not, so I'm+# ruling out N-ary transitive renames for N>1.+#+# Therefore our expected result is:+# z/t, y/a, x/b, w/c, u/d, u/e, u/f+# The reason that v/d DOES get transitively renamed to u/d is that u/ isn't+# renamed somewhere. A slightly sub-optimal result, but it uses fairly+# simple rules that are consistent with what we need for all the other+# testcases and simplifies things for the user.++test_expect_success'9d-setup: N-way transitive rename?''+test_create_repo9d&&+(+cd9d&&++mkdirzyxwvu&&+echoa>z/a&&+echob>y/b&&+echoc>x/c&&+echod>w/d&&+echoe>v/e&&+echof>u/f&&+gitaddzyxwvu&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+gitmvz/ay/&&+gitmvx/cw/&&+gitmvv/eu/&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+echot>z/t&&+gitmvy/bx/&&+gitmvw/dv/&&+gitaddz/t&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_failure'9d-check: N-way transitive rename?''+(+cd9d&&++gitcheckoutA^0&&++gitmerge-srecursiveB^0>out&&+test_i18ngrep"WARNING: Avoiding applying z -> y rename to z/t"out&&+test_i18ngrep"WARNING: Avoiding applying y -> x rename to y/a"out&&+test_i18ngrep"WARNING: Avoiding applying x -> w rename to x/b"out&&+test_i18ngrep"WARNING: Avoiding applying w -> v rename to w/c"out&&++gitls-files-s>out&&+test_line_count=7out&&+gitls-files-o>out&&+test_line_count=1out&&++gitrev-parse>actual\+HEAD:z/t\+HEAD:y/aHEAD:x/bHEAD:w/c\+HEAD:u/dHEAD:u/eHEAD:u/f&&+gitrev-parse>expect\+B:z/t\+O:z/aO:y/bO:x/c\+O:w/dO:v/eA:u/f&&+test_cmpexpectactual+)+'++# Testcase 9e, N-to-1 whammo+# (Related to testcase 9c...and 1c and 7e)+# Commit O: dir1/{a,b}, dir2/{d,e}, dir3/{g,h}, dirN/{j,k}+# Commit A: dir1/{a,b,c,yo}, dir2/{d,e,f,yo}, dir3/{g,h,i,yo}, dirN/{j,k,l,yo}+# Commit B: combined/{a,b,d,e,g,h,j,k}+# Expected: combined/{a,b,c,d,e,f,g,h,i,j,k,l}, CONFLICT(Nto1) warnings,+# dir1/yo, dir2/yo, dir3/yo, dirN/yo++test_expect_success'9e-setup: N-to-1 whammo''+test_create_repo9e&&+(+cd9e&&++mkdirdir1dir2dir3dirN&&+echoa>dir1/a&&+echob>dir1/b&&+echod>dir2/d&&+echoe>dir2/e&&+echog>dir3/g&&+echoh>dir3/h&&+echoj>dirN/j&&+echok>dirN/k&&+gitadddir*&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+echoc>dir1/c&&+echoyo>dir1/yo&&+echof>dir2/f&&+echoyo>dir2/yo&&+echoi>dir3/i&&+echoyo>dir3/yo&&+echol>dirN/l&&+echoyo>dirN/yo&&+gitadddir*&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+gitmvdir1combined&&+gitmvdir2/*combined/&&+gitmvdir3/*combined/&&+gitmvdirN/*combined/&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_failureC_LOCALE_OUTPUT'9e-check: N-to-1 whammo''+(+cd9e&&++gitcheckoutA^0&&++test_must_failgitmerge-srecursiveB^0>out&&+grep"CONFLICT (implicit dir rename): Cannot map more than one path to combined/yo"out>error_line&&+grep-qdir1/yoerror_line&&+grep-qdir2/yoerror_line&&+grep-qdir3/yoerror_line&&+grep-qdirN/yoerror_line&&++gitls-files-s>out&&+test_line_count=16out&&+gitls-files-u>out&&+test_line_count=0out&&+gitls-files-o>out&&+test_line_count=2out&&++gitrev-parse>actual\+:0:combined/a:0:combined/b:0:combined/c\+:0:combined/d:0:combined/e:0:combined/f\+:0:combined/g:0:combined/h:0:combined/i\+:0:combined/j:0:combined/k:0:combined/l&&+gitrev-parse>expect\+O:dir1/aO:dir1/bA:dir1/c\+O:dir2/dO:dir2/eA:dir2/f\+O:dir3/gO:dir3/hA:dir3/i\+O:dirN/jO:dirN/kA:dirN/l&&+test_cmpexpectactual&&++gitrev-parse>actual\+:0:dir1/yo:0:dir2/yo:0:dir3/yo:0:dirN/yo&&+gitrev-parse>expect\+A:dir1/yoA:dir2/yoA:dir3/yoA:dirN/yo&&+test_cmpexpectactual+)+'++# Testcase 9f, Renamed directory that only contained immediate subdirs+# (Related to testcases 1e & 9g)+# Commit O: goal/{a,b}/$more_files+# Commit A: priority/{a,b}/$more_files+# Commit B: goal/{a,b}/$more_files, goal/c+# Expected: priority/{a,b}/$more_files, priority/c++test_expect_success'9f-setup: Renamed directory that only contained immediate subdirs''+test_create_repo9f&&+(+cd9f&&++mkdir-pgoal/a&&+mkdir-pgoal/b&&+echofoo>goal/a/foo&&+echobar>goal/b/bar&&+echobaz>goal/b/baz&&+gitaddgoal&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+gitmvgoal/priority&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+echoc>goal/c&&+gitaddgoal/c&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_failure'9f-check: Renamed directory that only contained immediate subdirs''+(+cd9f&&++gitcheckoutA^0&&++gitmerge-srecursiveB^0&&++gitls-files-s>out&&+test_line_count=4out&&++gitrev-parse>actual\+HEAD:priority/a/foo\+HEAD:priority/b/bar\+HEAD:priority/b/baz\+HEAD:priority/c&&+gitrev-parse>expect\+O:goal/a/foo\+O:goal/b/bar\+O:goal/b/baz\+B:goal/c&&+test_cmpexpectactual&&+test_must_failgitrev-parseHEAD:goal/c+)+'++# Testcase 9g, Renamed directory that only contained immediate subdirs, immediate subdirs renamed+# (Related to testcases 1e & 9f)+# Commit O: goal/{a,b}/$more_files+# Commit A: priority/{alpha,bravo}/$more_files+# Commit B: goal/{a,b}/$more_files, goal/c+# Expected: priority/{alpha,bravo}/$more_files, priority/c++test_expect_success'9g-setup: Renamed directory that only contained immediate subdirs, immediate subdirs renamed''+test_create_repo9g&&+(+cd9g&&++mkdir-pgoal/a&&+mkdir-pgoal/b&&+echofoo>goal/a/foo&&+echobar>goal/b/bar&&+echobaz>goal/b/baz&&+gitaddgoal&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+mkdirpriority&&+gitmvgoal/a/priority/alpha&&+gitmvgoal/b/priority/beta&&+rmdirgoal/&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+echoc>goal/c&&+gitaddgoal/c&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_failure'9g-check: Renamed directory that only contained immediate subdirs, immediate subdirs renamed''+(+cd9g&&++gitcheckoutA^0&&++gitmerge-srecursiveB^0&&++gitls-files-s>out&&+test_line_count=4out&&++gitrev-parse>actual\+HEAD:priority/alpha/foo\+HEAD:priority/beta/bar\+HEAD:priority/beta/baz\+HEAD:priority/c&&+gitrev-parse>expect\+O:goal/a/foo\+O:goal/b/bar\+O:goal/b/baz\+B:goal/c&&+test_cmpexpectactual&&+test_must_failgitrev-parseHEAD:goal/c+)+'++###########################################################################+# Rules suggested by section 9:+#+# If the other side of history did a directory rename to a path that your+# side renamed away, then ignore that particular rename from the other+# side of history for any implicit directory renames.+###########################################################################+ test_done
Create a new function, get_diffpairs() to compute the diff_filepairs
between two trees. While these are currently only used in
get_renames(), I want them to be available to some new functions. No
actual logic changes yet.
Reviewed-by: Stefan Beller <redacted>
Signed-off-by: Elijah Newren <redacted>
---
merge-recursive.c | 84 ++++++++++++++++++++++++++++++++++++++++---------------
1 file changed, 62 insertions(+), 22 deletions(-)
Move this function so it can re-use some others (without either
moving all of them or adding an annoying split between function
declarations and definitions). Cheat slightly by adding a blank line
for readability, and in order to silence checkpatch.pl.
Reviewed-by: Stefan Beller <redacted>
Signed-off-by: Elijah Newren <redacted>
---
merge-recursive.c | 139 +++++++++++++++++++++++++++---------------------------
1 file changed, 70 insertions(+), 69 deletions(-)
Previously, if !o->detect_rename then get_renames() would return an
empty string_list, and then process_renames() would have nothing to
iterate over. It seems more straightforward to simply avoid calling
either function in that case.
Reviewed-by: Stefan Beller <redacted>
Signed-off-by: Elijah Newren <redacted>
---
merge-recursive.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
@@ -3246,4 +3246,462 @@ test_expect_failure '10e-check: Does git complain about untracked file that is n)'+###########################################################################+# SECTION 11: Handling dirty (not up-to-date) files+#+# unpack_trees(), upon which the recursive merge algorithm is based, aborts+# the operation if untracked or dirty files would be deleted or overwritten+# by the merge. Unfortunately, unpack_trees() does not understand renames,+# and if it doesn't abort, then it muddies up the working directory before+# we even get to the point of detecting renames, so we need some special+# handling. This was true even of normal renames, but there are additional+# codepaths that need special handling with directory renames. Add+# testcases for both renamed-by-directory-rename-detection and standard+# rename cases.+###########################################################################++# Testcase 11a, Avoid losing dirty contents with simple rename+# Commit O: z/{a,b_v1},+# Commit A: z/{a,c_v1}, and z/c_v1 has uncommitted mods+# Commit B: z/{a,b_v2}+# Expected: ERROR_MSG(Refusing to lose dirty file at z/c) ++# z/a, staged version of z/c has sha1sum matching B:z/b_v2,+# z/c~HEAD with contents of B:z/b_v2,+# z/c with uncommitted mods on top of A:z/c_v1++test_expect_success'11a-setup: Avoid losing dirty contents with simple rename''+test_create_repo11a&&+(+cd11a&&++mkdirz&&+echoa>z/a&&+test_seq110>z/b&&+gitaddz&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+gitmvz/bz/c&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+echo11>>z/b&&+gitaddz/b&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_failure'11a-check: Avoid losing dirty contents with simple rename''+(+cd11a&&++gitcheckoutA^0&&+echostuff>>z/c&&++test_must_failgitmerge-srecursiveB^0>out2>err&&+test_i18ngrep"Refusing to lose dirty file at z/c"out&&++test_seq110>expected&&+echostuff>>expected&&+test_cmpexpectedz/c&&++gitls-files-s>out&&+test_line_count=2out&&+gitls-files-u>out&&+test_line_count=1out&&+gitls-files-o>out&&+test_line_count=4out&&++gitrev-parse>actual\+:0:z/a:2:z/c&&+gitrev-parse>expect\+O:z/aB:z/b&&+test_cmpexpectactual&&++githash-objectz/c~HEAD>actual&&+gitrev-parseB:z/b>expect&&+test_cmpexpectactual+)+'++# Testcase 11b, Avoid losing dirty file involved in directory rename+# Commit O: z/a, x/{b,c_v1}+# Commit A: z/{a,c_v1}, x/b, and z/c_v1 has uncommitted mods+# Commit B: y/a, x/{b,c_v2}+# Expected: y/{a,c_v2}, x/b, z/c_v1 with uncommitted mods untracked,+# ERROR_MSG(Refusing to lose dirty file at z/c)+++test_expect_success'11b-setup: Avoid losing dirty file involved in directory rename''+test_create_repo11b&&+(+cd11b&&++mkdirzx&&+echoa>z/a&&+echob>x/b&&+test_seq110>x/c&&+gitaddzx&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+gitmvx/cz/c&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+gitmvzy&&+echo11>>x/c&&+gitaddx/c&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_failure'11b-check: Avoid losing dirty file involved in directory rename''+(+cd11b&&++gitcheckoutA^0&&+echostuff>>z/c&&++gitmerge-srecursiveB^0>out2>err&&+test_i18ngrep"Refusing to lose dirty file at z/c"out&&++grep-qstuffz/c&&+test_seq110>expected&&+echostuff>>expected&&+test_cmpexpectedz/c&&++gitls-files-s>out&&+test_line_count=3out&&+gitls-files-u>out&&+test_line_count=0out&&+gitls-files-m>out&&+test_line_count=0out&&+gitls-files-o>out&&+test_line_count=4out&&++gitrev-parse>actual\+:0:x/b:0:y/a:0:y/c&&+gitrev-parse>expect\+O:x/bO:z/aB:x/c&&+test_cmpexpectactual&&++githash-objecty/c>actual&&+gitrev-parseB:x/c>expect&&+test_cmpexpectactual+)+'++# Testcase 11c, Avoid losing not-up-to-date with rename + D/F conflict+# Commit O: y/a, x/{b,c_v1}+# Commit A: y/{a,c_v1}, x/b, and y/c_v1 has uncommitted mods+# Commit B: y/{a,c/d}, x/{b,c_v2}+# Expected: Abort_msg("following files would be overwritten by merge") ++# y/c left untouched (still has uncommitted mods)++test_expect_success'11c-setup: Avoid losing not-uptodate with rename + D/F conflict''+test_create_repo11c&&+(+cd11c&&++mkdiryx&&+echoa>y/a&&+echob>x/b&&+test_seq110>x/c&&+gitaddyx&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+gitmvx/cy/c&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+mkdiry/c&&+echod>y/c/d&&+echo11>>x/c&&+gitaddx/cy/c/d&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_success'11c-check: Avoid losing not-uptodate with rename + D/F conflict''+(+cd11c&&++gitcheckoutA^0&&+echostuff>>y/c&&++test_must_failgitmerge-srecursiveB^0>out2>err&&+test_i18ngrep"following files would be overwritten by merge"err&&++grep-qstuffy/c&&+test_seq110>expected&&+echostuff>>expected&&+test_cmpexpectedy/c&&++gitls-files-s>out&&+test_line_count=3out&&+gitls-files-u>out&&+test_line_count=0out&&+gitls-files-m>out&&+test_line_count=1out&&+gitls-files-o>out&&+test_line_count=3out+)+'++# Testcase 11d, Avoid losing not-up-to-date with rename + D/F conflict+# Commit O: z/a, x/{b,c_v1}+# Commit A: z/{a,c_v1}, x/b, and z/c_v1 has uncommitted mods+# Commit B: y/{a,c/d}, x/{b,c_v2}+# Expected: D/F: y/c_v2 vs y/c/d) ++# Warning_Msg("Refusing to lose dirty file at z/c) ++# y/{a,c~HEAD,c/d}, x/b, now-untracked z/c_v1 with uncommitted mods++test_expect_success'11d-setup: Avoid losing not-uptodate with rename + D/F conflict''+test_create_repo11d&&+(+cd11d&&++mkdirzx&&+echoa>z/a&&+echob>x/b&&+test_seq110>x/c&&+gitaddzx&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+gitmvx/cz/c&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+gitmvzy&&+mkdiry/c&&+echod>y/c/d&&+echo11>>x/c&&+gitaddx/cy/c/d&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_failure'11d-check: Avoid losing not-uptodate with rename + D/F conflict''+(+cd11d&&++gitcheckoutA^0&&+echostuff>>z/c&&++test_must_failgitmerge-srecursiveB^0>out2>err&&+test_i18ngrep"Refusing to lose dirty file at z/c"out&&++grep-qstuffz/c&&+test_seq110>expected&&+echostuff>>expected&&+test_cmpexpectedz/c++gitls-files-s>out&&+test_line_count=4out&&+gitls-files-u>out&&+test_line_count=1out&&+gitls-files-o>out&&+test_line_count=5out&&++gitrev-parse>actual\+:0:x/b:0:y/a:0:y/c/d:3:y/c&&+gitrev-parse>expect\+O:x/bO:z/aB:y/c/dB:x/c&&+test_cmpexpectactual&&++githash-objecty/c~HEAD>actual&&+gitrev-parseB:x/c>expect&&+test_cmpexpectactual+)+'++# Testcase 11e, Avoid deleting not-up-to-date with dir rename/rename(1to2)/add+# Commit O: z/{a,b}, x/{c_1,d}+# Commit A: y/{a,b,c_2}, x/d, w/c_1, and y/c_2 has uncommitted mods+# Commit B: z/{a,b,c_1}, x/d+# Expected: Failed Merge; y/{a,b} + x/d ++# CONFLICT(rename/rename) x/c_1 -> w/c_1 vs y/c_1 ++# ERROR_MSG(Refusing to lose dirty file at y/c)+# y/c~B^0 has O:x/c_1 contents+# y/c~HEAD has A:y/c_2 contents+# y/c has dirty file from before merge++test_expect_success'11e-setup: Avoid deleting not-uptodate with dir rename/rename(1to2)/add''+test_create_repo11e&&+(+cd11e&&++mkdirzx&&+echoa>z/a&&+echob>z/b&&+echoc>x/c&&+echod>x/d&&+gitaddzx&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+gitmvz/y/&&+echodifferent>y/c&&+mkdirw&&+gitmvx/cw/&&+gitaddy/c&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+gitmvx/cz/&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_failure'11e-check: Avoid deleting not-uptodate with dir rename/rename(1to2)/add''+(+cd11e&&++gitcheckoutA^0&&+echomods>>y/c&&++test_must_failgitmerge-srecursiveB^0>out2>err&&+test_i18ngrep"CONFLICT (rename/rename)"out&&+test_i18ngrep"Refusing to lose dirty file at y/c"out&&++gitls-files-s>out&&+test_line_count=7out&&+gitls-files-u>out&&+test_line_count=4out&&+gitls-files-o>out&&+test_line_count=4out&&++echodifferent>expected&&+echomods>>expected&&+test_cmpexpectedy/c&&++gitrev-parse>actual\+:0:y/a:0:y/b:0:x/d:1:x/c:2:w/c:2:y/c:3:y/c&&+gitrev-parse>expect\+O:z/aO:z/bO:x/dO:x/cO:x/cA:y/cO:x/c&&+test_cmpexpectactual&&++githash-object>actual\+y/c~B^0y/c~HEAD&&+gitrev-parse>expect\+O:x/cA:y/c&&+test_cmpexpectactual+)+'++# Testcase 11f, Avoid deleting not-up-to-date w/ dir rename/rename(2to1)+# Commit O: z/{a,b}, x/{c_1,d_2}+# Commit A: y/{a,b,wham_1}, x/d_2, except y/wham has uncommitted mods+# Commit B: z/{a,b,wham_2}, x/c_1+# Expected: Failed Merge; y/{a,b} + untracked y/{wham~B^0,wham~B^HEAD} ++# y/wham with dirty changes from before merge ++# CONFLICT(rename/rename) x/c vs x/d -> y/wham+# ERROR_MSG(Refusing to lose dirty file at y/wham)++test_expect_success'11f-setup: Avoid deleting not-uptodate with dir rename/rename(2to1)''+test_create_repo11f&&+(+cd11f&&++mkdirzx&&+echoa>z/a&&+echob>z/b&&+test_seq110>x/c&&+echod>x/d&&+gitaddzx&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+gitmvz/y/&&+gitmvx/cy/wham&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+gitmvx/dz/wham&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_failure'11f-check: Avoid deleting not-uptodate with dir rename/rename(2to1)''+(+cd11f&&++gitcheckoutA^0&&+echoimportant>>y/wham&&++test_must_failgitmerge-srecursiveB^0>out2>err&&+test_i18ngrep"CONFLICT (rename/rename)"out&&+test_i18ngrep"Refusing to lose dirty file at y/wham"out&&++gitls-files-s>out&&+test_line_count=4out&&+gitls-files-u>out&&+test_line_count=2out&&+gitls-files-o>out&&+test_line_count=4out&&++test_seq110>expected&&+echoimportant>>expected&&+test_cmpexpectedy/wham&&++test_must_failgitrev-parse:1:y/wham&&+githash-object>actual\+y/wham~B^0y/wham~HEAD&&+gitrev-parse>expect\+O:x/dO:x/c&&+test_cmpexpectactual&&++gitrev-parse>actual\+:0:y/a:0:y/b:2:y/wham:3:y/wham&&+gitrev-parse>expect\+O:z/aO:z/bO:x/cO:x/d&&+test_cmpexpectactual+)+'+ test_done
Add a long note about why we are not considering "partial directory
renames" for the current directory rename detection implementation.
Reviewed-by: Stefan Beller <redacted>
Signed-off-by: Elijah Newren <redacted>
---
t/t6043-merge-rename-directories.sh | 115 ++++++++++++++++++++++++++++++++++++
1 file changed, 115 insertions(+)
@@ -735,4 +735,119 @@ test_expect_success '3b-check: Avoid implicit rename if involved as source on cu# of a rename on either side of a merge.###########################################################################++###########################################################################+# SECTION 4: Partially renamed directory; still exists on both sides of merge+#+# What if we were to attempt to do directory rename detection when someone+# "mostly" moved a directory but still left some files around, or,+# equivalently, fully renamed a directory in one commmit and then recreated+# that directory in a later commit adding some new files and then tried to+# merge?+#+# It's hard to divine user intent in these cases, because you can make an+# argument that, depending on the intermediate history of the side being+# merged, that some users will want files in that directory to+# automatically be detected and renamed, while users with a different+# intermediate history wouldn't want that rename to happen.+#+# I think that it is best to simply not have directory rename detection+# apply to such cases. My reasoning for this is four-fold: (1) it's+# easiest for users in general to figure out what happened if we don't+# apply directory rename detection in any such case, (2) it's an easy rule+# to explain ["We don't do directory rename detection if the directory+# still exists on both sides of the merge"], (3) we can get some hairy+# edge/corner cases that would be really confusing and possibly not even+# representable in the index if we were to even try, and [related to 3] (4)+# attempting to resolve this issue of divining user intent by examining+# intermediate history goes against the spirit of three-way merges and is a+# path towards crazy corner cases that are far more complex than what we're+# already dealing with.+#+# Note that the wording of the rule ("We don't do directory rename+# detection if the directory still exists on both sides of the merge.")+# also excludes "renaming" of a directory into a subdirectory of itself+# (e.g. /some/dir/* -> /some/dir/subdir/*). It may be possible to carve+# out an exception for "renaming"-beneath-itself cases without opening+# weird edge/corner cases for other partial directory renames, but for now+# we are keeping the rule simple.+#+# This section contains a test for a partially-renamed-directory case.+###########################################################################++# Testcase 4a, Directory split, with original directory still present+# (Related to testcase 1f)+# Commit O: z/{b,c,d,e}+# Commit A: y/{b,c,d}, z/e+# Commit B: z/{b,c,d,e,f}+# Expected: y/{b,c,d}, z/{e,f}+# NOTE: Even though most files from z moved to y, we don't want f to follow.++test_expect_success'4a-setup: Directory split, with original directory still present''+test_create_repo4a&&+(+cd4a&&++mkdirz&&+echob>z/b&&+echoc>z/c&&+echod>z/d&&+echoe>z/e&&+gitaddz&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+mkdiry&&+gitmvz/by/&&+gitmvz/cy/&&+gitmvz/dy/&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+echof>z/f&&+gitaddz/f&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_success'4a-check: Directory split, with original directory still present''+(+cd4a&&++gitcheckoutA^0&&++gitmerge-srecursiveB^0&&++gitls-files-s>out&&+test_line_count=5out&&+gitls-files-u>out&&+test_line_count=0out&&+gitls-files-o>out&&+test_line_count=1out&&++gitrev-parse>actual\+HEAD:y/bHEAD:y/cHEAD:y/dHEAD:z/eHEAD:z/f&&+gitrev-parse>expect\+O:z/bO:z/cO:z/dO:z/eB:z/f&&+test_cmpexpectactual+)+'++###########################################################################+# Rules suggested by section 4:+#+# Directory-rename-detection should be turned off for any directories (as+# a source for renames) that exist on both sides of the merge. (The "as+# a source for renames" clarification is due to cases like 1c where+# the target directory exists on both sides and we do want the rename+# detection.) But, sadly, see testcase 8b.+###########################################################################+ test_done
@@ -0,0 +1,442 @@+#!/bin/sh++test_description="recursive merge with directory renames"+# includes checking of many corner cases, with a similar methodology to:+# t6042: corner cases with renames but not criss-cross merges+# t6036: corner cases with both renames and criss-cross merges+#+# The setup for all of them, pictorially, is:+#+# A+# o+# / \+# O o ?+# \ /+# o+# B+#+# To help make it easier to follow the flow of tests, they have been+# divided into sections and each test will start with a quick explanation+# of what commits O, A, and B contain.+#+# Notation:+# z/{b,c} means files z/b and z/c both exist+# x/d_1 means file x/d exists with content d1. (Purpose of the+# underscore notation is to differentiate different+# files that might be renamed into each other's paths.)++../test-lib.sh+++###########################################################################+# SECTION 1: Basic cases we should be able to handle+###########################################################################++# Testcase 1a, Basic directory rename.+# Commit O: z/{b,c}+# Commit A: y/{b,c}+# Commit B: z/{b,c,d,e/f}+# Expected: y/{b,c,d,e/f}++test_expect_success'1a-setup: Simple directory rename detection''+test_create_repo1a&&+(+cd1a&&++mkdirz&&+echob>z/b&&+echoc>z/c&&+gitaddz&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+gitmvzy&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+echod>z/d&&+mkdirz/e&&+echof>z/e/f&&+gitaddz/dz/e/f&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_failure'1a-check: Simple directory rename detection''+(+cd1a&&++gitcheckoutA^0&&++gitmerge-srecursiveB^0&&++gitls-files-s>out&&+test_line_count=4out&&++gitrev-parse>actual\+HEAD:y/bHEAD:y/cHEAD:y/dHEAD:y/e/f&&+gitrev-parse>expect\+O:z/bO:z/cB:z/dB:z/e/f&&+test_cmpexpectactual&&++githash-objecty/d>actual&&+gitrev-parseB:z/d>expect&&+test_cmpexpectactual&&++test_must_failgitrev-parseHEAD:z/d&&+test_must_failgitrev-parseHEAD:z/e/f&&+test_path_is_missingz/d&&+test_path_is_missingz/e/f+)+'++# Testcase 1b, Merge a directory with another+# Commit O: z/{b,c}, y/d+# Commit A: z/{b,c,e}, y/d+# Commit B: y/{b,c,d}+# Expected: y/{b,c,d,e}++test_expect_success'1b-setup: Merge a directory with another''+test_create_repo1b&&+(+cd1b&&++mkdirz&&+echob>z/b&&+echoc>z/c&&+mkdiry&&+echod>y/d&&+gitaddzy&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+echoe>z/e&&+gitaddz/e&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+gitmvz/by&&+gitmvz/cy&&+rmdirz&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_failure'1b-check: Merge a directory with another''+(+cd1b&&++gitcheckoutA^0&&++gitmerge-srecursiveB^0&&++gitls-files-s>out&&+test_line_count=4out&&++gitrev-parse>actual\+HEAD:y/bHEAD:y/cHEAD:y/dHEAD:y/e&&+gitrev-parse>expect\+O:z/bO:z/cO:y/dA:z/e&&+test_cmpexpectactual&&+test_must_failgitrev-parseHEAD:z/e+)+'++# Testcase 1c, Transitive renaming+# (Related to testcases 3a and 6d -- when should a transitive rename apply?)+# (Related to testcases 9c and 9d -- can transitivity repeat?)+# Commit O: z/{b,c}, x/d+# Commit A: y/{b,c}, x/d+# Commit B: z/{b,c,d}+# Expected: y/{b,c,d} (because x/d -> z/d -> y/d)++test_expect_success'1c-setup: Transitive renaming''+test_create_repo1c&&+(+cd1c&&++mkdirz&&+echob>z/b&&+echoc>z/c&&+mkdirx&&+echod>x/d&&+gitaddzx&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+gitmvzy&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+gitmvx/dz/d&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_failure'1c-check: Transitive renaming''+(+cd1c&&++gitcheckoutA^0&&++gitmerge-srecursiveB^0&&++gitls-files-s>out&&+test_line_count=3out&&++gitrev-parse>actual\+HEAD:y/bHEAD:y/cHEAD:y/d&&+gitrev-parse>expect\+O:z/bO:z/cO:x/d&&+test_cmpexpectactual&&+test_must_failgitrev-parseHEAD:x/d&&+test_must_failgitrev-parseHEAD:z/d&&+test_path_is_missingz/d+)+'++# Testcase 1d, Directory renames (merging two directories into one new one)+# cause a rename/rename(2to1) conflict+# (Related to testcases 1c and 7b)+# Commit O. z/{b,c}, y/{d,e}+# Commit A. x/{b,c}, y/{d,e,m,wham_1}+# Commit B. z/{b,c,n,wham_2}, x/{d,e}+# Expected: x/{b,c,d,e,m,n}, CONFLICT:(y/wham_1 & z/wham_2 -> x/wham)+# Note: y/m & z/n should definitely move into x. By the same token, both+# y/wham_1 & z/wham_2 should too...giving us a conflict.++test_expect_success'1d-setup: Directory renames cause a rename/rename(2to1) conflict''+test_create_repo1d&&+(+cd1d&&++mkdirz&&+echob>z/b&&+echoc>z/c&&+mkdiry&&+echod>y/d&&+echoe>y/e&&+gitaddzy&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+gitmvzx&&+echom>y/m&&+echowham1>y/wham&&+gitaddy&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+gitmvyx&&+echon>z/n&&+echowham2>z/wham&&+gitaddz&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_failure'1d-check: Directory renames cause a rename/rename(2to1) conflict''+(+cd1d&&++gitcheckoutA^0&&++test_must_failgitmerge-srecursiveB^0>out&&+test_i18ngrep"CONFLICT (rename/rename)"out&&++gitls-files-s>out&&+test_line_count=8out&&+gitls-files-u>out&&+test_line_count=2out&&+gitls-files-o>out&&+test_line_count=3out&&++gitrev-parse>actual\+:0:x/b:0:x/c:0:x/d:0:x/e:0:x/m:0:x/n&&+gitrev-parse>expect\+O:z/bO:z/cO:y/dO:y/eA:y/mB:z/n&&+test_cmpexpectactual&&++test_must_failgitrev-parse:0:x/wham&&+gitrev-parse>actual\+:2:x/wham:3:x/wham&&+gitrev-parse>expect\+A:y/whamB:z/wham&&+test_cmpexpectactual&&++test_path_is_missingx/wham&&+test_path_is_filex/wham~HEAD&&+test_path_is_filex/wham~B^0&&++githash-object>actual\+x/wham~HEADx/wham~B^0&&+gitrev-parse>expect\+A:y/whamB:z/wham&&+test_cmpexpectactual+)+'++# Testcase 1e, Renamed directory, with all filenames being renamed too+# Commit O: z/{oldb,oldc}+# Commit A: y/{newb,newc}+# Commit B: z/{oldb,oldc,d}+# Expected: y/{newb,newc,d}++test_expect_success'1e-setup: Renamed directory, with all files being renamed too''+test_create_repo1e&&+(+cd1e&&++mkdirz&&+echob>z/oldb&&+echoc>z/oldc&&+gitaddz&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+mkdiry&&+gitmvz/oldby/newb&&+gitmvz/oldcy/newc&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+echod>z/d&&+gitaddz/d&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_failure'1e-check: Renamed directory, with all files being renamed too''+(+cd1e&&++gitcheckoutA^0&&++gitmerge-srecursiveB^0&&++gitls-files-s>out&&+test_line_count=3out&&++gitrev-parse>actual\+HEAD:y/newbHEAD:y/newcHEAD:y/d&&+gitrev-parse>expect\+O:z/oldbO:z/oldcB:z/d&&+test_cmpexpectactual&&+test_must_failgitrev-parseHEAD:z/d+)+'++# Testcase 1f, Split a directory into two other directories+# (Related to testcases 3a, all of section 2, and all of section 4)+# Commit O: z/{b,c,d,e,f}+# Commit A: z/{b,c,d,e,f,g}+# Commit B: y/{b,c}, x/{d,e,f}+# Expected: y/{b,c}, x/{d,e,f,g}++test_expect_success'1f-setup: Split a directory into two other directories''+test_create_repo1f&&+(+cd1f&&++mkdirz&&+echob>z/b&&+echoc>z/c&&+echod>z/d&&+echoe>z/e&&+echof>z/f&&+gitaddz&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+echog>z/g&&+gitaddz/g&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+mkdiry&&+mkdirx&&+gitmvz/by/&&+gitmvz/cy/&&+gitmvz/dx/&&+gitmvz/ex/&&+gitmvz/fx/&&+rmdirz&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_failure'1f-check: Split a directory into two other directories''+(+cd1f&&++gitcheckoutA^0&&++gitmerge-srecursiveB^0&&++gitls-files-s>out&&+test_line_count=6out&&++gitrev-parse>actual\+HEAD:y/bHEAD:y/cHEAD:x/dHEAD:x/eHEAD:x/fHEAD:x/g&&+gitrev-parse>expect\+O:z/bO:z/cO:z/dO:z/eO:z/fA:z/g&&+test_cmpexpectactual&&+test_path_is_missingz/g&&+test_must_failgitrev-parseHEAD:z/g+)+'++###########################################################################+# Rules suggested by testcases in section 1:+#+# We should still detect the directory rename even if it wasn't just+# the directory renamed, but the files within it. (see 1b)+#+# If renames split a directory into two or more others, the directory+# with the most renames, "wins" (see 1c). However, see the testcases+# in section 2, plus testcases 3a and 4a.+###########################################################################++test_done
@@ -1180,4 +1180,340 @@ test_expect_failure '5d-check: Directory/file/file conflict due to directory ren# back to old handling. But, sadly, see testcases 8a and 8b.###########################################################################++###########################################################################+# SECTION 6: Same side of the merge was the one that did the rename+#+# It may sound obvious that you only want to apply implicit directory+# renames to directories if the _other_ side of history did the renaming.+# If you did make an implementation that didn't explicitly enforce this+# rule, the majority of cases that would fall under this section would+# also be solved by following the rules from the above sections. But+# there are still a few that stick out, so this section covers them just+# to make sure we also get them right.+###########################################################################++# Testcase 6a, Tricky rename/delete+# Commit O: z/{b,c,d}+# Commit A: z/b+# Commit B: y/{b,c}, z/d+# Expected: y/b, CONFLICT(rename/delete, z/c -> y/c vs. NULL)+# Note: We're just checking here that the rename of z/b and z/c to put+# them under y/ doesn't accidentally catch z/d and make it look like+# it is also involved in a rename/delete conflict.++test_expect_success'6a-setup: Tricky rename/delete''+test_create_repo6a&&+(+cd6a&&++mkdirz&&+echob>z/b&&+echoc>z/c&&+echod>z/d&&+gitaddz&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+gitrmz/c&&+gitrmz/d&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+mkdiry&&+gitmvz/by/&&+gitmvz/cy/&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_success'6a-check: Tricky rename/delete''+(+cd6a&&++gitcheckoutA^0&&++test_must_failgitmerge-srecursiveB^0>out&&+test_i18ngrep"CONFLICT (rename/delete).*z/c.*y/c"out&&++gitls-files-s>out&&+test_line_count=2out&&+gitls-files-u>out&&+test_line_count=1out&&+gitls-files-o>out&&+test_line_count=1out&&++gitrev-parse>actual\+:0:y/b:3:y/c&&+gitrev-parse>expect\+O:z/bO:z/c&&+test_cmpexpectactual+)+'++# Testcase 6b, Same rename done on both sides+# (Related to testcases 6c and 8e)+# Commit O: z/{b,c}+# Commit A: y/{b,c}+# Commit B: y/{b,c}, z/d+# Expected: y/{b,c}, z/d+# Note: If we did directory rename detection here, we'd move z/d into y/,+# but B did that rename and still decided to put the file into z/,+# so we probably shouldn't apply directory rename detection for it.++test_expect_success'6b-setup: Same rename done on both sides''+test_create_repo6b&&+(+cd6b&&++mkdirz&&+echob>z/b&&+echoc>z/c&&+gitaddz&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+gitmvzy&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+gitmvzy&&+mkdirz&&+echod>z/d&&+gitaddz/d&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_success'6b-check: Same rename done on both sides''+(+cd6b&&++gitcheckoutA^0&&++gitmerge-srecursiveB^0&&++gitls-files-s>out&&+test_line_count=3out&&+gitls-files-u>out&&+test_line_count=0out&&+gitls-files-o>out&&+test_line_count=1out&&++gitrev-parse>actual\+HEAD:y/bHEAD:y/cHEAD:z/d&&+gitrev-parse>expect\+O:z/bO:z/cB:z/d&&+test_cmpexpectactual+)+'++# Testcase 6c, Rename only done on same side+# (Related to testcases 6b and 8e)+# Commit O: z/{b,c}+# Commit A: z/{b,c} (no change)+# Commit B: y/{b,c}, z/d+# Expected: y/{b,c}, z/d+# NOTE: Seems obvious, but just checking that the implementation doesn't+# "accidentally detect a rename" and give us y/{b,c,d}.++test_expect_success'6c-setup: Rename only done on same side''+test_create_repo6c&&+(+cd6c&&++mkdirz&&+echob>z/b&&+echoc>z/c&&+gitaddz&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+test_tick&&+gitcommit--allow-empty-m"A"&&++gitcheckoutB&&+gitmvzy&&+mkdirz&&+echod>z/d&&+gitaddz/d&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_success'6c-check: Rename only done on same side''+(+cd6c&&++gitcheckoutA^0&&++gitmerge-srecursiveB^0&&++gitls-files-s>out&&+test_line_count=3out&&+gitls-files-u>out&&+test_line_count=0out&&+gitls-files-o>out&&+test_line_count=1out&&++gitrev-parse>actual\+HEAD:y/bHEAD:y/cHEAD:z/d&&+gitrev-parse>expect\+O:z/bO:z/cB:z/d&&+test_cmpexpectactual+)+'++# Testcase 6d, We don't always want transitive renaming+# (Related to testcase 1c)+# Commit O: z/{b,c}, x/d+# Commit A: z/{b,c}, x/d (no change)+# Commit B: y/{b,c}, z/d+# Expected: y/{b,c}, z/d+# NOTE: Again, this seems obvious but just checking that the implementation+# doesn't "accidentally detect a rename" and give us y/{b,c,d}.++test_expect_success'6d-setup: We do not always want transitive renaming''+test_create_repo6d&&+(+cd6d&&++mkdirz&&+echob>z/b&&+echoc>z/c&&+mkdirx&&+echod>x/d&&+gitaddzx&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+test_tick&&+gitcommit--allow-empty-m"A"&&++gitcheckoutB&&+gitmvzy&&+gitmvxz&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_success'6d-check: We do not always want transitive renaming''+(+cd6d&&++gitcheckoutA^0&&++gitmerge-srecursiveB^0&&++gitls-files-s>out&&+test_line_count=3out&&+gitls-files-u>out&&+test_line_count=0out&&+gitls-files-o>out&&+test_line_count=1out&&++gitrev-parse>actual\+HEAD:y/bHEAD:y/cHEAD:z/d&&+gitrev-parse>expect\+O:z/bO:z/cO:x/d&&+test_cmpexpectactual+)+'++# Testcase 6e, Add/add from one-side+# Commit O: z/{b,c}+# Commit A: z/{b,c} (no change)+# Commit B: y/{b,c,d_1}, z/d_2+# Expected: y/{b,c,d_1}, z/d_2+# NOTE: Again, this seems obvious but just checking that the implementation+# doesn't "accidentally detect a rename" and give us y/{b,c} ++# add/add conflict on y/d_1 vs y/d_2.++test_expect_success'6e-setup: Add/add from one side''+test_create_repo6e&&+(+cd6e&&++mkdirz&&+echob>z/b&&+echoc>z/c&&+gitaddz&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+test_tick&&+gitcommit--allow-empty-m"A"&&++gitcheckoutB&&+gitmvzy&&+echod1>y/d&&+mkdirz&&+echod2>z/d&&+gitaddy/dz/d&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_success'6e-check: Add/add from one side''+(+cd6e&&++gitcheckoutA^0&&++gitmerge-srecursiveB^0&&++gitls-files-s>out&&+test_line_count=4out&&+gitls-files-u>out&&+test_line_count=0out&&+gitls-files-o>out&&+test_line_count=1out&&++gitrev-parse>actual\+HEAD:y/bHEAD:y/cHEAD:y/dHEAD:z/d&&+gitrev-parse>expect\+O:z/bO:z/cB:y/dB:z/d&&+test_cmpexpectactual+)+'++###########################################################################+# Rules suggested by section 6:+#+# Only apply implicit directory renames to directories if the other+# side of history is the one doing the renaming.+###########################################################################+ test_done
When a file is present in HEAD before the merge and the other side of the
merge does not modify that file, we try to avoid re-writing the file and
making it stat-dirty. However, when a file is present in HEAD before the
merge and was in a directory that was renamed by the other side of the
merge, we have to move the file to a new location and re-write it.
Update the code that checks whether we can skip the update to also work in
the presence of directory renames.
Reviewed-by: Stefan Beller <redacted>
Signed-off-by: Elijah Newren <redacted>
---
merge-recursive.c | 4 +---
t/t6043-merge-rename-directories.sh | 2 +-
2 files changed, 2 insertions(+), 4 deletions(-)
@@ -2772,7 +2772,6 @@ static int merge_content(struct merge_options *o,if(mfi.clean&&!df_conflict_remains&&oid_eq(&mfi.oid,a_oid)&&mfi.mode==a_mode){-intpath_renamed_outside_HEAD;output(o,3,_("Skipped %s (merged same as existing)"),path);/**Thecontentmergeresultedinthesamefilecontentswe
@@ -2780,8 +2779,7 @@ static int merge_content(struct merge_options *o,*arerecordedatthecorrectpath(whichmaynotbetrue*ifthemergeinvolvesarename).*/-path_renamed_outside_HEAD=!path2||!strcmp(path,path2);-if(!path_renamed_outside_HEAD){+if(was_tracked(path)){add_cacheinfo(o,mfi.mode,&mfi.oid,path,0,(!o->call_depth),0);returnmfi.clean;
@@ -3884,7 +3884,7 @@ test_expect_success '12b-setup: Moving one directory hierarchy into another' ')'-test_expect_failure'12b-check: Moving one directory hierarchy into another''+test_expect_success'12b-check: Moving one directory hierarchy into another''(cd12b&&
get_renames() would look up stage data that already existed (populated
in get_unmerged(), taken from whatever unpack_trees() created), and if
it didn't exist, would call insert_stage_data() to create the necessary
entry for the given file. The insert_stage_data() fallback becomes
much more important for directory rename detection, because that creates
a mechanism to have a file in the resulting merge that didn't exist on
either side of history. However, insert_stage_data(), due to calling
get_tree_entry() loaded up trees as readily as files. We aren't
interested in comparing trees to files; the D/F conflict handling is
done elsewhere. This code is just concerned with what entries existed
for a given path on the different sides of the merge, so create a
get_tree_entry_if_blob() helper function and use it.
Reviewed-by: Stefan Beller <redacted>
Signed-off-by: Elijah Newren <redacted>
---
merge-recursive.c | 27 +++++++++++++++++++++------
1 file changed, 21 insertions(+), 6 deletions(-)
@@ -582,4 +582,157 @@ test_expect_success '2b-check: Directory split into two on one side, with equal# messages are handled correctly.###########################################################################++###########################################################################+# SECTION 3: Path in question is the source path for some rename already+#+# Combining cases from Section 1 and trying to handle them could lead to+# directory renaming detection being over-applied. So, this section+# provides some good testcases to check that the implementation doesn't go+# too far.+###########################################################################++# Testcase 3a, Avoid implicit rename if involved as source on other side+# (Related to testcases 1c and 1f)+# Commit O: z/{b,c,d}+# Commit A: z/{b,c,d} (no change)+# Commit B: y/{b,c}, x/d+# Expected: y/{b,c}, x/d+test_expect_success'3a-setup: Avoid implicit rename if involved as source on other side''+test_create_repo3a&&+(+cd3a&&++mkdirz&&+echob>z/b&&+echoc>z/c&&+echod>z/d&&+gitaddz&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+test_tick&&+gitcommit--allow-empty-m"A"&&++gitcheckoutB&&+mkdiry&&+mkdirx&&+gitmvz/by/&&+gitmvz/cy/&&+gitmvz/dx/&&+rmdirz&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_success'3a-check: Avoid implicit rename if involved as source on other side''+(+cd3a&&++gitcheckoutA^0&&++gitmerge-srecursiveB^0&&++gitls-files-s>out&&+test_line_count=3out&&++gitrev-parse>actual\+HEAD:y/bHEAD:y/cHEAD:x/d&&+gitrev-parse>expect\+O:z/bO:z/cO:z/d&&+test_cmpexpectactual+)+'++# Testcase 3b, Avoid implicit rename if involved as source on other side+# (Related to testcases 5c and 7c, also kind of 1e and 1f)+# Commit O: z/{b,c,d}+# Commit A: y/{b,c}, x/d+# Commit B: z/{b,c}, w/d+# Expected: y/{b,c}, CONFLICT:(z/d -> x/d vs. w/d)+# NOTE: We're particularly checking that since z/d is already involved as+# a source in a file rename on the same side of history, that we don't+# get it involved in directory rename detection. If it were, we might+# end up with CONFLICT:(z/d -> y/d vs. x/d vs. w/d), i.e. a+# rename/rename/rename(1to3) conflict, which is just weird.+test_expect_success'3b-setup: Avoid implicit rename if involved as source on current side''+test_create_repo3b&&+(+cd3b&&++mkdirz&&+echob>z/b&&+echoc>z/c&&+echod>z/d&&+gitaddz&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+mkdiry&&+mkdirx&&+gitmvz/by/&&+gitmvz/cy/&&+gitmvz/dx/&&+rmdirz&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+mkdirw&&+gitmvz/dw/&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_success'3b-check: Avoid implicit rename if involved as source on current side''+(+cd3b&&++gitcheckoutA^0&&++test_must_failgitmerge-srecursiveB^0>out&&+test_i18ngrepCONFLICT.*rename/rename.*z/d.*x/d.*w/dout&&+test_i18ngrep!CONFLICT.*rename/rename.*y/dout&&++gitls-files-s>out&&+test_line_count=5out&&+gitls-files-u>out&&+test_line_count=3out&&+gitls-files-o>out&&+test_line_count=1out&&++gitrev-parse>actual\+:0:y/b:0:y/c:1:z/d:2:x/d:3:w/d&&+gitrev-parse>expect\+O:z/bO:z/cO:z/dO:z/dO:z/d&&+test_cmpexpectactual&&++test_path_is_missingz/d&&+githash-object>actual\+x/dw/d&&+gitrev-parse>expect\+O:z/dO:z/d&&+test_cmpexpectactual+)+'++###########################################################################+# Rules suggested by section 3:+#+# Avoid directory-rename-detection for a path, if that path is the source+# of a rename on either side of a merge.+###########################################################################+ test_done
@@ -1516,4 +1516,400 @@ test_expect_success '6e-check: Add/add from one side' '# side of history is the one doing the renaming.###########################################################################++###########################################################################+# SECTION 7: More involved Edge/Corner cases+#+# The ruleset we have generated in the above sections seems to provide+# well-defined merges. But can we find edge/corner cases that either (a)+# are harder for users to understand, or (b) have a resolution that is+# non-intuitive or suboptimal?+#+# The testcases in this section dive into cases that I've tried to craft in+# a way to find some that might be surprising to users or difficult for+# them to understand (the next section will look at non-intuitive or+# suboptimal merge results). Some of the testcases are similar to ones+# from past sections, but have been simplified to try to highlight error+# messages using a "modified" path (due to the directory rename). Are+# users okay with these?+#+# In my opinion, testcases that are difficult to understand from this+# section is due to difficulty in the testcase rather than the directory+# renaming (similar to how t6042 and t6036 have difficult resolutions due+# to the problem setup itself being complex). And I don't think the+# error messages are a problem.+#+# On the other hand, the testcases in section 8 worry me slightly more...+###########################################################################++# Testcase 7a, rename-dir vs. rename-dir (NOT split evenly) PLUS add-other-file+# Commit O: z/{b,c}+# Commit A: y/{b,c}+# Commit B: w/b, x/c, z/d+# Expected: y/d, CONFLICT(rename/rename for both z/b and z/c)+# NOTE: There's a rename of z/ here, y/ has more renames, so z/d -> y/d.++test_expect_success'7a-setup: rename-dir vs. rename-dir (NOT split evenly) PLUS add-other-file''+test_create_repo7a&&+(+cd7a&&++mkdirz&&+echob>z/b&&+echoc>z/c&&+gitaddz&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+gitmvzy&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+mkdirw&&+mkdirx&&+gitmvz/bw/&&+gitmvz/cx/&&+echod>z/d&&+gitaddz/d&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_failure'7a-check: rename-dir vs. rename-dir (NOT split evenly) PLUS add-other-file''+(+cd7a&&++gitcheckoutA^0&&++test_must_failgitmerge-srecursiveB^0>out&&+test_i18ngrep"CONFLICT (rename/rename).*z/b.*y/b.*w/b"out&&+test_i18ngrep"CONFLICT (rename/rename).*z/c.*y/c.*x/c"out&&++gitls-files-s>out&&+test_line_count=7out&&+gitls-files-u>out&&+test_line_count=6out&&+gitls-files-o>out&&+test_line_count=1out&&++gitrev-parse>actual\+:1:z/b:2:y/b:3:w/b:1:z/c:2:y/c:3:x/c:0:y/d&&+gitrev-parse>expect\+O:z/bO:z/bO:z/bO:z/cO:z/cO:z/cB:z/d&&+test_cmpexpectactual&&++githash-object>actual\+y/bw/by/cx/c&&+gitrev-parse>expect\+O:z/bO:z/bO:z/cO:z/c&&+test_cmpexpectactual+)+'++# Testcase 7b, rename/rename(2to1), but only due to transitive rename+# (Related to testcase 1d)+# Commit O: z/{b,c}, x/d_1, w/d_2+# Commit A: y/{b,c,d_2}, x/d_1+# Commit B: z/{b,c,d_1}, w/d_2+# Expected: y/{b,c}, CONFLICT(rename/rename(2to1): x/d_1, w/d_2 -> y_d)++test_expect_success'7b-setup: rename/rename(2to1), but only due to transitive rename''+test_create_repo7b&&+(+cd7b&&++mkdirz&&+mkdirx&&+mkdirw&&+echob>z/b&&+echoc>z/c&&+echod1>x/d&&+echod2>w/d&&+gitaddzxw&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+gitmvzy&&+gitmvw/dy/&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+gitmvx/dz/&&+rmdirx&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_failure'7b-check: rename/rename(2to1), but only due to transitive rename''+(+cd7b&&++gitcheckoutA^0&&++test_must_failgitmerge-srecursiveB^0>out&&+test_i18ngrep"CONFLICT (rename/rename)"out&&++gitls-files-s>out&&+test_line_count=4out&&+gitls-files-u>out&&+test_line_count=2out&&+gitls-files-o>out&&+test_line_count=3out&&++gitrev-parse>actual\+:0:y/b:0:y/c:2:y/d:3:y/d&&+gitrev-parse>expect\+O:z/bO:z/cO:w/dO:x/d&&+test_cmpexpectactual&&++test_path_is_missingy/d&&+test_path_is_filey/d~HEAD&&+test_path_is_filey/d~B^0&&++githash-object>actual\+y/d~HEADy/d~B^0&&+gitrev-parse>expect\+O:w/dO:x/d&&+test_cmpexpectactual+)+'++# Testcase 7c, rename/rename(1to...2or3); transitive rename may add complexity+# (Related to testcases 3b and 5c)+# Commit O: z/{b,c}, x/d+# Commit A: y/{b,c}, w/d+# Commit B: z/{b,c,d}+# Expected: y/{b,c}, CONFLICT(x/d -> w/d vs. y/d)+# NOTE: z/ was renamed to y/ so we do want to report+# neither CONFLICT(x/d -> w/d vs. z/d)+# nor CONFLiCT x/d -> w/d vs. y/d vs. z/d)++test_expect_success'7c-setup: rename/rename(1to...2or3); transitive rename may add complexity''+test_create_repo7c&&+(+cd7c&&++mkdirz&&+echob>z/b&&+echoc>z/c&&+mkdirx&&+echod>x/d&&+gitaddzx&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+gitmvzy&&+gitmvxw&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+gitmvx/dz/&&+rmdirx&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_failure'7c-check: rename/rename(1to...2or3); transitive rename may add complexity''+(+cd7c&&++gitcheckoutA^0&&++test_must_failgitmerge-srecursiveB^0>out&&+test_i18ngrep"CONFLICT (rename/rename).*x/d.*w/d.*y/d"out&&++gitls-files-s>out&&+test_line_count=5out&&+gitls-files-u>out&&+test_line_count=3out&&+gitls-files-o>out&&+test_line_count=1out&&++gitrev-parse>actual\+:0:y/b:0:y/c:1:x/d:2:w/d:3:y/d&&+gitrev-parse>expect\+O:z/bO:z/cO:x/dO:x/dO:x/d&&+test_cmpexpectactual+)+'++# Testcase 7d, transitive rename involved in rename/delete; how is it reported?+# (Related somewhat to testcases 5b and 8d)+# Commit O: z/{b,c}, x/d+# Commit A: y/{b,c}+# Commit B: z/{b,c,d}+# Expected: y/{b,c}, CONFLICT(delete x/d vs rename to y/d)+# NOTE: z->y so NOT CONFLICT(delete x/d vs rename to z/d)++test_expect_success'7d-setup: transitive rename involved in rename/delete; how is it reported?''+test_create_repo7d&&+(+cd7d&&++mkdirz&&+echob>z/b&&+echoc>z/c&&+mkdirx&&+echod>x/d&&+gitaddzx&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+gitmvzy&&+gitrm-rfx&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+gitmvx/dz/&&+rmdirx&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_failure'7d-check: transitive rename involved in rename/delete; how is it reported?''+(+cd7d&&++gitcheckoutA^0&&++test_must_failgitmerge-srecursiveB^0>out&&+test_i18ngrep"CONFLICT (rename/delete).*x/d.*y/d"out&&++gitls-files-s>out&&+test_line_count=3out&&+gitls-files-u>out&&+test_line_count=1out&&+gitls-files-o>out&&+test_line_count=1out&&++gitrev-parse>actual\+:0:y/b:0:y/c:3:y/d&&+gitrev-parse>expect\+O:z/bO:z/cO:x/d&&+test_cmpexpectactual+)+'++# Testcase 7e, transitive rename in rename/delete AND dirs in the way+# (Very similar to 'both rename source and destination involved in D/F conflict' from t6022-merge-rename.sh)+# (Also related to testcases 9c and 9d)+# Commit O: z/{b,c}, x/d_1+# Commit A: y/{b,c,d/g}, x/d/f+# Commit B: z/{b,c,d_1}+# Expected: rename/delete(x/d_1->y/d_1 vs. None) + D/F conflict on y/d+# y/{b,c,d/g}, y/d_1~B^0, x/d/f++# NOTE: The main path of interest here is d_1 and where it ends up, but+# this is actually a case that has two potential directory renames+# involved and D/F conflict(s), so it makes sense to walk through+# each step.+#+# Commit A renames z/ -> y/. Thus everything that B adds to z/+# should be instead moved to y/. This gives us the D/F conflict on+# y/d because x/d_1 -> z/d_1 -> y/d_1 conflicts with y/d/g.+#+# Further, commit B renames x/ -> z/, thus everything A adds to x/+# should instead be moved to z/...BUT we removed z/ and renamed it+# to y/, so maybe everything should move not from x/ to z/, but+# from x/ to z/ to y/. Doing so might make sense from the logic so+# far, but note that commit A had both an x/ and a y/; it did the+# renaming of z/ to y/ and created x/d/f and it clearly made these+# things separate, so it doesn't make much sense to push these+# together. Doing so is what I'd call a doubly transitive rename;+# see testcases 9c and 9d for further discussion of this issue and+# how it's resolved.++test_expect_success'7e-setup: transitive rename in rename/delete AND dirs in the way''+test_create_repo7e&&+(+cd7e&&++mkdirz&&+echob>z/b&&+echoc>z/c&&+mkdirx&&+echod1>x/d&&+gitaddzx&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+gitmvzy&&+gitrmx/d&&+mkdir-px/d&&+mkdir-py/d&&+echof>x/d/f&&+echog>y/d/g&&+gitaddx/d/fy/d/g&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+gitmvx/dz/&&+rmdirx&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_failure'7e-check: transitive rename in rename/delete AND dirs in the way''+(+cd7e&&++gitcheckoutA^0&&++test_must_failgitmerge-srecursiveB^0>out&&+test_i18ngrep"CONFLICT (rename/delete).*x/d.*y/d"out&&++gitls-files-s>out&&+test_line_count=5out&&+gitls-files-u>out&&+test_line_count=1out&&+gitls-files-o>out&&+test_line_count=2out&&++gitrev-parse>actual\+:0:x/d/f:0:y/d/g:0:y/b:0:y/c:3:y/d&&+gitrev-parse>expect\+A:x/d/fA:y/d/gO:z/bO:z/cO:x/d&&+test_cmpexpectactual&&++githash-objecty/d~B^0>actual&&+gitrev-parseO:x/d>expect&&+test_cmpexpectactual+)+'+ test_done
The amount of logic in merge_trees() relative to renames was just a few
lines, but split it out into new handle_renames() and cleanup_renames()
functions to prepare for additional logic to be added to each. No code or
logic changes, just a new place to put stuff for when the rename detection
gains additional checks.
Note that process_renames() records pointers to various information (such
as diff_filepairs) into rename_conflict_info structs. Even though the
rename string_lists are not directly used once handle_renames() completes,
we should not immediately free the lists at the end of that function
because they store the information referenced in the rename_conflict_info,
which is used later in process_entry(). Thus the reason for a separate
cleanup_renames().
Reviewed-by: Stefan Beller <redacted>
Signed-off-by: Elijah Newren <redacted>
---
merge-recursive.c | 43 +++++++++++++++++++++++++++++++++----------
1 file changed, 33 insertions(+), 10 deletions(-)
This commit hooks together all the directory rename logic by making the
necessary changes to the rename struct, it's dst_entry, and the
diff_filepair under consideration.
Reviewed-by: Stefan Beller <redacted>
Signed-off-by: Elijah Newren <redacted>
---
merge-recursive.c | 187 +++++++++++++++++++++++++++++++++++-
t/t6043-merge-rename-directories.sh | 50 +++++-----
2 files changed, 211 insertions(+), 26 deletions(-)
@@ -136,7 +136,7 @@ test_expect_success '1b-setup: Merge a directory with another' ')'-test_expect_failure'1b-check: Merge a directory with another''+test_expect_success'1b-check: Merge a directory with another''(cd1b&&
@@ -263,7 +263,7 @@ test_expect_success '1d-setup: Directory renames cause a rename/rename(2to1) con)'-test_expect_failure'1d-check: Directory renames cause a rename/rename(2to1) conflict''+test_expect_success'1d-check: Directory renames cause a rename/rename(2to1) conflict''(cd1d&&
@@ -342,7 +342,7 @@ test_expect_success '1e-setup: Renamed directory, with all files being renamed t)'-test_expect_failure'1e-check: Renamed directory, with all files being renamed too''+test_expect_success'1e-check: Renamed directory, with all files being renamed too''(cd1e&&
@@ -408,7 +408,7 @@ test_expect_success '1f-setup: Split a directory into two other directories' ')'-test_expect_failure'1f-check: Split a directory into two other directories''+test_expect_success'1f-check: Split a directory into two other directories''(cd1f&&
@@ -907,7 +907,7 @@ test_expect_success '5a-setup: Merge directories, other side adds files to origi)'-test_expect_failure'5a-check: Merge directories, other side adds files to original and target''+test_expect_success'5a-check: Merge directories, other side adds files to original and target''(cd5a&&
@@ -981,7 +981,7 @@ test_expect_success '5b-setup: Rename/delete in order to get add/add/add conflic)'-test_expect_failure'5b-check: Rename/delete in order to get add/add/add conflict''+test_expect_success'5b-check: Rename/delete in order to get add/add/add conflict''(cd5b&&
@@ -1061,7 +1061,7 @@ test_expect_success '5c-setup: Transitive rename would cause rename/rename/renam)'-test_expect_failure'5c-check: Transitive rename would cause rename/rename/rename/add/add/add''+test_expect_success'5c-check: Transitive rename would cause rename/rename/rename/add/add/add''(cd5c&&
@@ -1145,7 +1145,7 @@ test_expect_success '5d-setup: Directory/file/file conflict due to directory ren)'-test_expect_failure'5d-check: Directory/file/file conflict due to directory rename''+test_expect_success'5d-check: Directory/file/file conflict due to directory rename''(cd5d&&
@@ -1583,7 +1583,7 @@ test_expect_success '7a-setup: rename-dir vs. rename-dir (NOT split evenly) PLUS)'-test_expect_failure'7a-check: rename-dir vs. rename-dir (NOT split evenly) PLUS add-other-file''+test_expect_success'7a-check: rename-dir vs. rename-dir (NOT split evenly) PLUS add-other-file''(cd7a&&
@@ -1655,7 +1655,7 @@ test_expect_success '7b-setup: rename/rename(2to1), but only due to transitive r)'-test_expect_failure'7b-check: rename/rename(2to1), but only due to transitive rename''+test_expect_success'7b-check: rename/rename(2to1), but only due to transitive rename''(cd7b&&
@@ -1795,7 +1795,7 @@ test_expect_success '7d-setup: transitive rename involved in rename/delete; how)'-test_expect_failure'7d-check: transitive rename involved in rename/delete; how is it reported?''+test_expect_success'7d-check: transitive rename involved in rename/delete; how is it reported?''(cd7d&&
@@ -1885,7 +1885,7 @@ test_expect_success '7e-setup: transitive rename in rename/delete AND dirs in th)'-test_expect_failure'7e-check: transitive rename in rename/delete AND dirs in the way''+test_expect_success'7e-check: transitive rename in rename/delete AND dirs in the way''(cd7e&&
@@ -1976,7 +1976,7 @@ test_expect_success '8a-setup: Dual-directory rename, one into the others way' ')'-test_expect_failure'8a-check: Dual-directory rename, one into the others way''+test_expect_success'8a-check: Dual-directory rename, one into the others way''(cd8a&&
@@ -2283,7 +2283,7 @@ test_expect_success '8e-setup: Both sides rename, one side adds to original dire)'-test_expect_failure'8e-check: Both sides rename, one side adds to original directory''+test_expect_success'8e-check: Both sides rename, one side adds to original directory''(cd8e&&
@@ -2370,7 +2370,7 @@ test_expect_success '9a-setup: Inner renamed directory within outer renamed dire)'-test_expect_failure'9a-check: Inner renamed directory within outer renamed directory''+test_expect_success'9a-check: Inner renamed directory within outer renamed directory''(cd9a&&
@@ -2440,7 +2440,7 @@ test_expect_success '9b-setup: Transitive rename with content merge' ')'-test_expect_failure'9b-check: Transitive rename with content merge''+test_expect_success'9b-check: Transitive rename with content merge''(cd9b&&
@@ -2778,7 +2778,7 @@ test_expect_success '9f-setup: Renamed directory that only contained immediate s)'-test_expect_failure'9f-check: Renamed directory that only contained immediate subdirs''+test_expect_success'9f-check: Renamed directory that only contained immediate subdirs''(cd9f&&
@@ -1150,6 +1150,26 @@ static int conflict_rename_dir(struct merge_options *o,{conststructdiff_filespec*dest=pair->two;+if(!o->call_depth&&would_lose_untracked(dest->path)){+char*alt_path=unique_path(o,dest->path,rename_branch);++output(o,1,_("Error: Refusing to lose untracked file at %s; "+"writing to %s instead."),+dest->path,alt_path);+/*+*Writethefileinworktreeatalt_path,butnotinthe+*index.Instead,writetodest->pathfortheindexbut+*onlyatthehigherappropriatestage.+*/+if(update_file(o,0,&dest->oid,dest->mode,alt_path))+return-1;+free(alt_path);+returnupdate_stages(o,dest->path,NULL,+rename_branch==o->branch1?dest:NULL,+rename_branch==o->branch1?NULL:dest);+}++/* Update dest->path both in index and in worktree */if(update_file(o,1,&dest->oid,dest->mode,dest->path))return-1;return0;
@@ -1168,7 +1188,8 @@ static int handle_change_delete(struct merge_options *o,constchar*update_path=path;intret=0;-if(dir_in_way(path,!o->call_depth,0)){+if(dir_in_way(path,!o->call_depth,0)||+(!o->call_depth&&would_lose_untracked(path))){update_path=alt_path=unique_path(o,path,change_branch);}
@@ -1294,6 +1315,12 @@ static int handle_file(struct merge_options *o,dst_name=unique_path(o,rename->path,cur_branch);output(o,1,_("%s is a directory in %s adding as %s instead"),rename->path,other_branch,dst_name);+}elseif(!o->call_depth&&+would_lose_untracked(rename->path)){+dst_name=unique_path(o,rename->path,cur_branch);+output(o,1,_("Refusing to lose untracked file at %s; "+"adding as %s instead"),+rename->path,dst_name);}}if((ret=update_file(o,0,&rename->oid,rename->mode,dst_name)))
@@ -1419,7 +1446,18 @@ static int conflict_rename_rename_2to1(struct merge_options *o,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);-remove_file(o,0,path,0);+if(would_lose_untracked(path))+/*+*Onlywaywegethereisifbothrenameswerefrom+*adirectoryrenameANDuserhadanuntrackedfile+*atthelocationwherebothfilesendupafterthe+*twodirectoryrenames.Seetestcase10doft6043.+*/+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,
@@ -2992,7 +2992,7 @@ test_expect_success '10b-setup: Overwrite untracked with dir rename + delete' ')'-test_expect_failure'10b-check: Overwrite untracked with dir rename + delete''+test_expect_success'10b-check: Overwrite untracked with dir rename + delete''(cd10b&&
@@ -3070,7 +3070,7 @@ test_expect_success '10c-setup: Overwrite untracked with dir rename/rename(1to2))'-test_expect_failure'10c-check: Overwrite untracked with dir rename/rename(1to2)''+test_expect_success'10c-check: Overwrite untracked with dir rename/rename(1to2)''(cd10c&&
@@ -3145,7 +3145,7 @@ test_expect_success '10d-setup: Delete untracked with dir rename/rename(2to1)' ')'-test_expect_failure'10d-check: Delete untracked with dir rename/rename(2to1)''+test_expect_success'10d-check: Delete untracked with dir rename/rename(2to1)''(cd10d&&
get_renames() has always zero'ed out diff_queued_diff.nr while only
manually free'ing diff_filepairs that did not correspond to renames.
Further, it allocated struct renames that were tucked away in the
return string_list. Make sure all of these are deallocated when we
are done with them.
Reviewed-by: Stefan Beller <redacted>
Signed-off-by: Elijah Newren <redacted>
---
merge-recursive.c | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)
directory renaming and merging can cause one or more files to be moved to
where an existing file is, or to cause several files to all be moved to
the same (otherwise vacant) location. Add checking and reporting for such
cases, falling back to no-directory-rename handling for such paths.
Reviewed-by: Stefan Beller <redacted>
Signed-off-by: Elijah Newren <redacted>
---
merge-recursive.c | 146 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
merge-recursive.h | 7 +++
2 files changed, 150 insertions(+), 3 deletions(-)
@@ -47,6 +47,13 @@ struct dir_rename_entry {structstring_listpossible_new_dirs;};+structcollision_entry{+structhashmap_entryent;/* must be the first member! */+char*target_file;+structstring_listsource_files;+unsignedreported_already:1;+};+/* merge_trees() but with recursive ancestor consolidation */intmerge_recursive(structmerge_options*o,structcommit*h1,
Add a testcase showing spurious rename/rename(1to2) conflicts occurring
due to directory rename detection.
Also add a pair of testcases dealing with moving directory hierarchies
around that were suggested by Stefan Beller as "food for thought" during
his review of an earlier patch series, but which actually uncovered a
bug. Round things out with a test that is a cross between the two
testcases that showed existing bugs in order to make sure we aren't
merely addressing problems in isolation but in general.
Reviewed-by: Stefan Beller <redacted>
Signed-off-by: Elijah Newren <redacted>
---
t/t6043-merge-rename-directories.sh | 296 ++++++++++++++++++++++++++++++++++++
1 file changed, 296 insertions(+)
@@ -159,6 +159,7 @@ test_expect_success '1b-check: Merge a directory with another' '# Testcase 1c, Transitive renaming# (Related to testcases 3a and 6d -- when should a transitive rename apply?)# (Related to testcases 9c and 9d -- can transitivity repeat?)+# (Related to testcase 12b -- joint-transitivity?)# Commit O: z/{b,c}, x/d# Commit A: y/{b,c}, x/d# Commit B: z/{b,c,d}
@@ -2871,6 +2872,68 @@ test_expect_failure '9g-check: Renamed directory that only contained immediate s)'+# Testcase 9h, Avoid implicit rename if involved as source on other side+# (Extremely closely related to testcase 3a)+# Commit O: z/{b,c,d_1}+# Commit A: z/{b,c,d_2}+# Commit B: y/{b,c}, x/d_1+# Expected: y/{b,c}, x/d_2+# NOTE: If we applied the z/ -> y/ rename to z/d, then we'd end up with+# a rename/rename(1to2) conflict (z/d -> y/d vs. x/d)+test_expect_success'9h-setup: Avoid dir rename on merely modified path''+test_create_repo9h&&+(+cd9h&&++mkdirz&&+echob>z/b&&+echoc>z/c&&+printf"1\n2\n3\n4\n5\n6\n7\n8\nd\n">z/d&&+gitaddz&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+test_tick&&+echomore>>z/d&&+gitaddz/d&&+gitcommit-m"A"&&++gitcheckoutB&&+mkdiry&&+mkdirx&&+gitmvz/by/&&+gitmvz/cy/&&+gitmvz/dx/&&+rmdirz&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_failure'9h-check: Avoid dir rename on merely modified path''+(+cd9h&&++gitcheckoutA^0&&++gitmerge-srecursiveB^0&&++gitls-files-s>out&&+test_line_count=3out&&++gitrev-parse>actual\+HEAD:y/bHEAD:y/cHEAD:x/d&&+gitrev-parse>expect\+O:z/bO:z/cA:z/d&&+test_cmpexpectactual+)+'+############################################################################ Rules suggested by section 9:#
@@ -3704,4 +3767,237 @@ test_expect_success '11f-check: Avoid deleting not-uptodate with dir rename/rena)'+###########################################################################+# SECTION 12: Everything else+#+# Tests suggested by others. Tests added after implementation completed+# and submitted. Grab bag.+###########################################################################++# Testcase 12a, Moving one directory hierarchy into another+# (Related to testcase 9a)+# Commit O: node1/{leaf1,leaf2}, node2/{leaf3,leaf4}+# Commit A: node1/{leaf1,leaf2,node2/{leaf3,leaf4}}+# Commit B: node1/{leaf1,leaf2,leaf5}, node2/{leaf3,leaf4,leaf6}+# Expected: node1/{leaf1,leaf2,leaf5,node2/{leaf3,leaf4,leaf6}}++test_expect_success'12a-setup: Moving one directory hierarchy into another''+test_create_repo12a&&+(+cd12a&&++mkdir-pnode1node2&&+echoleaf1>node1/leaf1&&+echoleaf2>node1/leaf2&&+echoleaf3>node2/leaf3&&+echoleaf4>node2/leaf4&&+gitaddnode1node2&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+gitmvnode2/node1/&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+echoleaf5>node1/leaf5&&+echoleaf6>node2/leaf6&&+gitaddnode1node2&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_success'12a-check: Moving one directory hierarchy into another''+(+cd12a&&++gitcheckoutA^0&&++gitmerge-srecursiveB^0&&++gitls-files-s>out&&+test_line_count=6out&&++gitrev-parse>actual\+HEAD:node1/leaf1HEAD:node1/leaf2HEAD:node1/leaf5\+HEAD:node1/node2/leaf3\+HEAD:node1/node2/leaf4\+HEAD:node1/node2/leaf6&&+gitrev-parse>expect\+O:node1/leaf1O:node1/leaf2B:node1/leaf5\+O:node2/leaf3\+O:node2/leaf4\+B:node2/leaf6&&+test_cmpexpectactual+)+'++# Testcase 12b, Moving two directory hierarchies into each other+# (Related to testcases 1c and 12c)+# Commit O: node1/{leaf1, leaf2}, node2/{leaf3, leaf4}+# Commit A: node1/{leaf1, leaf2, node2/{leaf3, leaf4}}+# Commit B: node2/{leaf3, leaf4, node1/{leaf1, leaf2}}+# Expected: node1/node2/node1/{leaf1, leaf2},+# node2/node1/node2/{leaf3, leaf4}+# NOTE: Without directory renames, we would expect+# node2/node1/{leaf1, leaf2},+# node1/node2/{leaf3, leaf4}+# with directory rename detection, we note that+# commit A renames node2/ -> node1/node2/+# commit B renames node1/ -> node2/node1/+# therefore, applying those directory renames to the initial result+# (making all four paths experience a transitive renaming), yields+# the expected result.+#+# You may ask, is it weird to have two directories rename each other?+# To which, I can do no more than shrug my shoulders and say that+# even simple rules give weird results when given weird inputs.++test_expect_success'12b-setup: Moving one directory hierarchy into another''+test_create_repo12b&&+(+cd12b&&++mkdir-pnode1node2&&+echoleaf1>node1/leaf1&&+echoleaf2>node1/leaf2&&+echoleaf3>node2/leaf3&&+echoleaf4>node2/leaf4&&+gitaddnode1node2&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+gitmvnode2/node1/&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+gitmvnode1/node2/&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_failure'12b-check: Moving one directory hierarchy into another''+(+cd12b&&++gitcheckoutA^0&&++gitmerge-srecursiveB^0&&++gitls-files-s>out&&+test_line_count=4out&&++gitrev-parse>actual\+HEAD:node1/node2/node1/leaf1\+HEAD:node1/node2/node1/leaf2\+HEAD:node2/node1/node2/leaf3\+HEAD:node2/node1/node2/leaf4&&+gitrev-parse>expect\+O:node1/leaf1\+O:node1/leaf2\+O:node2/leaf3\+O:node2/leaf4&&+test_cmpexpectactual+)+'++# Testcase 12c, Moving two directory hierarchies into each other w/ content merge+# (Related to testcase 12b)+# Commit O: node1/{ leaf1_1, leaf2_1}, node2/{leaf3_1, leaf4_1}+# Commit A: node1/{ leaf1_2, leaf2_2, node2/{leaf3_2, leaf4_2}}+# Commit B: node2/{node1/{leaf1_3, leaf2_3}, leaf3_3, leaf4_3}+# Expected: Content merge conflicts for each of:+# node1/node2/node1/{leaf1, leaf2},+# node2/node1/node2/{leaf3, leaf4}+# NOTE: This is *exactly* like 12c, except that every path is modified on+# each side of the merge.++test_expect_success'12c-setup: Moving one directory hierarchy into another w/ content merge''+test_create_repo12c&&+(+cd12c&&++mkdir-pnode1node2&&+printf"1\n2\n3\n4\n5\n6\n7\n8\nleaf1\n">node1/leaf1&&+printf"1\n2\n3\n4\n5\n6\n7\n8\nleaf2\n">node1/leaf2&&+printf"1\n2\n3\n4\n5\n6\n7\n8\nleaf3\n">node2/leaf3&&+printf"1\n2\n3\n4\n5\n6\n7\n8\nleaf4\n">node2/leaf4&&+gitaddnode1node2&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+gitmvnode2/node1/&&+foriin`gitls-files`;doechosideA>>$i;done&&+gitadd-u&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+gitmvnode1/node2/&&+foriin`gitls-files`;doechosideB>>$i;done&&+gitadd-u&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_failure'12c-check: Moving one directory hierarchy into another w/ content merge''+(+cd12c&&++gitcheckoutA^0&&++test_must_failgitmerge-srecursiveB^0&&++gitls-files-u>out&&+test_line_count=12out&&++gitrev-parse>actual\+:1:node1/node2/node1/leaf1\+:1:node1/node2/node1/leaf2\+:1:node2/node1/node2/leaf3\+:1:node2/node1/node2/leaf4\+:2:node1/node2/node1/leaf1\+:2:node1/node2/node1/leaf2\+:2:node2/node1/node2/leaf3\+:2:node2/node1/node2/leaf4\+:3:node1/node2/node1/leaf1\+:3:node1/node2/node1/leaf2\+:3:node2/node1/node2/leaf3\+:3:node2/node1/node2/leaf4&&+gitrev-parse>expect\+O:node1/leaf1\+O:node1/leaf2\+O:node2/leaf3\+O:node2/leaf4\+A:node1/leaf1\+A:node1/leaf2\+A:node1/node2/leaf3\+A:node1/node2/leaf4\+B:node2/node1/leaf1\+B:node2/node1/leaf2\+B:node2/leaf3\+B:node2/leaf4&&+test_cmpexpectactual+)+'+ test_done
@@ -1912,4 +1912,408 @@ test_expect_failure '7e-check: transitive rename in rename/delete AND dirs in th)'+###########################################################################+# SECTION 8: Suboptimal merges+#+# As alluded to in the last section, the ruleset we have built up for+# detecting directory renames unfortunately has some special cases where it+# results in slightly suboptimal or non-intuitive behavior. This section+# explores these cases.+#+# To be fair, we already had non-intuitive or suboptimal behavior for most+# of these cases in git before introducing implicit directory rename+# detection, but it'd be nice if there was a modified ruleset out there+# that handled these cases a bit better.+###########################################################################++# Testcase 8a, Dual-directory rename, one into the others' way+# Commit O. x/{a,b}, y/{c,d}+# Commit A. x/{a,b,e}, y/{c,d,f}+# Commit B. y/{a,b}, z/{c,d}+#+# Possible Resolutions:+# w/o dir-rename detection: y/{a,b,f}, z/{c,d}, x/e+# Currently expected: y/{a,b,e,f}, z/{c,d}+# Optimal: y/{a,b,e}, z/{c,d,f}+#+# Note: Both x and y got renamed and it'd be nice to detect both, and we do+# better with directory rename detection than git did without, but the+# simple rule from section 5 prevents me from handling this as optimally as+# we potentially could.++test_expect_success'8a-setup: Dual-directory rename, one into the others way''+test_create_repo8a&&+(+cd8a&&++mkdirx&&+mkdiry&&+echoa>x/a&&+echob>x/b&&+echoc>y/c&&+echod>y/d&&+gitaddxy&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+echoe>x/e&&+echof>y/f&&+gitaddx/ey/f&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+gitmvyz&&+gitmvxy&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_failure'8a-check: Dual-directory rename, one into the others way''+(+cd8a&&++gitcheckoutA^0&&++gitmerge-srecursiveB^0&&++gitls-files-s>out&&+test_line_count=6out&&+gitls-files-u>out&&+test_line_count=0out&&+gitls-files-o>out&&+test_line_count=1out&&++gitrev-parse>actual\+HEAD:y/aHEAD:y/bHEAD:y/eHEAD:y/fHEAD:z/cHEAD:z/d&&+gitrev-parse>expect\+O:x/aO:x/bA:x/eA:y/fO:y/cO:y/d&&+test_cmpexpectactual+)+'++# Testcase 8b, Dual-directory rename, one into the others' way, with conflicting filenames+# Commit O. x/{a_1,b_1}, y/{a_2,b_2}+# Commit A. x/{a_1,b_1,e_1}, y/{a_2,b_2,e_2}+# Commit B. y/{a_1,b_1}, z/{a_2,b_2}+#+# w/o dir-rename detection: y/{a_1,b_1,e_2}, z/{a_2,b_2}, x/e_1+# Currently expected: <same>+# Scary: y/{a_1,b_1}, z/{a_2,b_2}, CONFLICT(add/add, e_1 vs. e_2)+# Optimal: y/{a_1,b_1,e_1}, z/{a_2,b_2,e_2}+#+# Note: Very similar to 8a, except instead of 'e' and 'f' in directories x and+# y, both are named 'e'. Without directory rename detection, neither file+# moves directories. Implement directory rename detection suboptimally, and+# you get an add/add conflict, but both files were added in commit A, so this+# is an add/add conflict where one side of history added both files --+# something we can't represent in the index. Obviously, we'd prefer the last+# resolution, but our previous rules are too coarse to allow it. Using both+# the rules from section 4 and section 5 save us from the Scary resolution,+# making us fall back to pre-directory-rename-detection behavior for both+# e_1 and e_2.++test_expect_success'8b-setup: Dual-directory rename, one into the others way, with conflicting filenames''+test_create_repo8b&&+(+cd8b&&++mkdirx&&+mkdiry&&+echoa1>x/a&&+echob1>x/b&&+echoa2>y/a&&+echob2>y/b&&+gitaddxy&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+echoe1>x/e&&+echoe2>y/e&&+gitaddx/ey/e&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+gitmvyz&&+gitmvxy&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_success'8b-check: Dual-directory rename, one into the others way, with conflicting filenames''+(+cd8b&&++gitcheckoutA^0&&++gitmerge-srecursiveB^0&&++gitls-files-s>out&&+test_line_count=6out&&+gitls-files-u>out&&+test_line_count=0out&&+gitls-files-o>out&&+test_line_count=1out&&++gitrev-parse>actual\+HEAD:y/aHEAD:y/bHEAD:z/aHEAD:z/bHEAD:x/eHEAD:y/e&&+gitrev-parse>expect\+O:x/aO:x/bO:y/aO:y/bA:x/eA:y/e&&+test_cmpexpectactual+)+'++# Testcase 8c, rename+modify/delete+# (Related to testcases 5b and 8d)+# Commit O: z/{b,c,d}+# Commit A: y/{b,c}+# Commit B: z/{b,c,d_modified,e}+# Expected: y/{b,c,e}, CONFLICT(rename+modify/delete: x/d -> y/d or deleted)+#+# Note: This testcase doesn't present any concerns for me...until you+# compare it with testcases 5b and 8d. See notes in 8d for more+# details.++test_expect_success'8c-setup: rename+modify/delete''+test_create_repo8c&&+(+cd8c&&++mkdirz&&+echob>z/b&&+echoc>z/c&&+test_seq110>z/d&&+gitaddz&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+gitrmz/d&&+gitmvzy&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+echo11>z/d&&+test_chmod+xz/d&&+echoe>z/e&&+gitaddz/dz/e&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_failure'8c-check: rename+modify/delete''+(+cd8c&&++gitcheckoutA^0&&++test_must_failgitmerge-srecursiveB^0>out&&+test_i18ngrep"CONFLICT (rename/delete).* z/d.*y/d"out&&++gitls-files-s>out&&+test_line_count=4out&&+gitls-files-u>out&&+test_line_count=1out&&+gitls-files-o>out&&+test_line_count=1out&&++gitrev-parse>actual\+:0:y/b:0:y/c:0:y/e:3:y/d&&+gitrev-parse>expect\+O:z/bO:z/cB:z/eB:z/d&&+test_cmpexpectactual&&++test_must_failgitrev-parse:1:y/d&&+test_must_failgitrev-parse:2:y/d&&+gitls-files-sy/d|grep^100755&&+test_path_is_filey/d+)+'++# Testcase 8d, rename/delete...or not?+# (Related to testcase 5b; these may appear slightly inconsistent to users;+# Also related to testcases 7d and 7e)+# Commit O: z/{b,c,d}+# Commit A: y/{b,c}+# Commit B: z/{b,c,d,e}+# Expected: y/{b,c,e}+#+# Note: It would also be somewhat reasonable to resolve this as+# y/{b,c,e}, CONFLICT(rename/delete: x/d -> y/d or deleted)+# The logic being that the only difference between this testcase and 8c+# is that there is no modification to d. That suggests that instead of a+# rename/modify vs. delete conflict, we should just have a rename/delete+# conflict, otherwise we are being inconsistent.+#+# However...as far as consistency goes, we didn't report a conflict for+# path d_1 in testcase 5b due to a different file being in the way. So,+# we seem to be forced to have cases where users can change things+# slightly and get what they may perceive as inconsistent results. It+# would be nice to avoid that, but I'm not sure I see how.+#+# In this case, I'm leaning towards: commit A was the one that deleted z/d+# and it did the rename of z to y, so the two "conflicts" (rename vs.+# delete) are both coming from commit A, which is illogical. Conflicts+# during merging are supposed to be about opposite sides doing things+# differently.++test_expect_success'8d-setup: rename/delete...or not?''+test_create_repo8d&&+(+cd8d&&++mkdirz&&+echob>z/b&&+echoc>z/c&&+test_seq110>z/d&&+gitaddz&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+gitrmz/d&&+gitmvzy&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+echoe>z/e&&+gitaddz/e&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_failure'8d-check: rename/delete...or not?''+(+cd8d&&++gitcheckoutA^0&&++gitmerge-srecursiveB^0&&++gitls-files-s>out&&+test_line_count=3out&&++gitrev-parse>actual\+HEAD:y/bHEAD:y/cHEAD:y/e&&+gitrev-parse>expect\+O:z/bO:z/cB:z/e&&+test_cmpexpectactual+)+'++# Testcase 8e, Both sides rename, one side adds to original directory+# Commit O: z/{b,c}+# Commit A: y/{b,c}+# Commit B: w/{b,c}, z/d+#+# Possible Resolutions:+# w/o dir-rename detection: z/d, CONFLICT(z/b -> y/b vs. w/b),+# CONFLICT(z/c -> y/c vs. w/c)+# Currently expected: y/d, CONFLICT(z/b -> y/b vs. w/b),+# CONFLICT(z/c -> y/c vs. w/c)+# Optimal: ??+#+# Notes: In commit A, directory z got renamed to y. In commit B, directory z+# did NOT get renamed; the directory is still present; instead it is+# considered to have just renamed a subset of paths in directory z+# elsewhere. Therefore, the directory rename done in commit A to z/+# applies to z/d and maps it to y/d.+#+# It's possible that users would get confused about this, but what+# should we do instead? Silently leaving at z/d seems just as bad or+# maybe even worse. Perhaps we could print a big warning about z/d+# and how we're moving to y/d in this case, but when I started thinking+# about the ramifications of doing that, I didn't know how to rule out+# that opening other weird edge and corner cases so I just punted.++test_expect_success'8e-setup: Both sides rename, one side adds to original directory''+test_create_repo8e&&+(+cd8e&&++mkdirz&&+echob>z/b&&+echoc>z/c&&+gitaddz&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+gitmvzy&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+gitmvzw&&+mkdirz&&+echod>z/d&&+gitaddz/d&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_failure'8e-check: Both sides rename, one side adds to original directory''+(+cd8e&&++gitcheckoutA^0&&++test_must_failgitmerge-srecursiveB^0>out2>err&&+test_i18ngrepCONFLICT.*rename/rename.*z/c.*y/c.*w/cout&&+test_i18ngrepCONFLICT.*rename/rename.*z/b.*y/b.*w/bout&&++gitls-files-s>out&&+test_line_count=7out&&+gitls-files-u>out&&+test_line_count=6out&&+gitls-files-o>out&&+test_line_count=2out&&++gitrev-parse>actual\+:1:z/b:2:y/b:3:w/b:1:z/c:2:y/c:3:w/c:0:y/d&&+gitrev-parse>expect\+O:z/bO:z/bO:z/bO:z/cO:z/cO:z/cB:z/d&&+test_cmpexpectactual&&++githash-object>actual\+y/bw/by/cw/c&&+gitrev-parse>expect\+O:z/bO:z/bO:z/cO:z/c&&+test_cmpexpectactual&&++test_path_is_missingz/b&&+test_path_is_missingz/c+)+'+ test_done
If a file on one side of history was renamed, and merely modified on the
other side, then applying a directory rename to the modified side gives us
a rename/rename(1to2) conflict. We should only apply directory renames to
pairs representing either adds or renames.
Making this change means that a directory rename testcase that was
previously reported as a rename/delete conflict will now be reported as a
modify/delete conflict.
Reviewed-by: Stefan Beller <redacted>
Signed-off-by: Elijah Newren <redacted>
---
merge-recursive.c | 4 +--
t/t6043-merge-rename-directories.sh | 55 +++++++++++++++++--------------------
2 files changed, 27 insertions(+), 32 deletions(-)
@@ -2078,18 +2078,23 @@ test_expect_success '8b-check: Dual-directory rename, one into the others way, w)'-# Testcase 8c, rename+modify/delete-# (Related to testcases 5b and 8d)+# Testcase 8c, modify/delete or rename+modify/delete?+# (Related to testcases 5b, 8d, and 9h)# Commit O: z/{b,c,d}# Commit A: y/{b,c}# Commit B: z/{b,c,d_modified,e}-# Expected: y/{b,c,e}, CONFLICT(rename+modify/delete: x/d -> y/d or deleted)+# Expected: y/{b,c,e}, CONFLICT(modify/delete: on z/d)#-# Note: This testcase doesn't present any concerns for me...until you-# compare it with testcases 5b and 8d. See notes in 8d for more-# details.--test_expect_success'8c-setup: rename+modify/delete''+# Note: It could easily be argued that the correct resolution here is+# y/{b,c,e}, CONFLICT(rename/delete: z/d -> y/d vs deleted)+# and that the modifed version of d should be present in y/ after+# the merge, just marked as conflicted. Indeed, I previously did+# argue that. But applying directory renames to the side of+# history where a file is merely modified results in spurious+# rename/rename(1to2) conflicts -- see testcase 9h. See also+# notes in 8d.++test_expect_success'8c-setup: modify/delete or rename+modify/delete?''test_create_repo8c&&(cd8c&&
@@ -2161,16 +2166,6 @@ test_expect_success '8c-check: rename+modify/delete' '## Note: It would also be somewhat reasonable to resolve this as# y/{b,c,e}, CONFLICT(rename/delete: x/d -> y/d or deleted)-# The logic being that the only difference between this testcase and 8c-# is that there is no modification to d. That suggests that instead of a-# rename/modify vs. delete conflict, we should just have a rename/delete-# conflict, otherwise we are being inconsistent.-#-# However...as far as consistency goes, we didn't report a conflict for-# path d_1 in testcase 5b due to a different file being in the way. So,-# we seem to be forced to have cases where users can change things-# slightly and get what they may perceive as inconsistent results. It-# would be nice to avoid that, but I'm not sure I see how.## In this case, I'm leaning towards: commit A was the one that deleted z/d# and it did the rename of z to y, so the two "conflicts" (rename vs.
@@ -2915,7 +2910,7 @@ test_expect_success '9h-setup: Avoid dir rename on merely modified path' ')'-test_expect_failure'9h-check: Avoid dir rename on merely modified path''+test_expect_success'9h-check: Avoid dir rename on merely modified path''(cd9h&&
@@ -3959,7 +3954,7 @@ test_expect_success '12c-setup: Moving one directory hierarchy into another w/ c)'-test_expect_failure'12c-check: Moving one directory hierarchy into another w/ content merge''+test_expect_success'12c-check: Moving one directory hierarchy into another w/ content merge''(cd12c&&
This fixes an issue that existed before my directory rename detection
patches that affects both normal renames and renames implied by
directory rename detection. Additional codepaths that only affect
overwriting of dirty files that are involved in directory rename
detection will be added in a subsequent commit.
Reviewed-by: Stefan Beller <redacted>
Signed-off-by: Elijah Newren <redacted>
---
merge-recursive.c | 85 ++++++++++++++++++++++++++++---------
merge-recursive.h | 2 +
t/t3501-revert-cherry-pick.sh | 2 +-
t/t6043-merge-rename-directories.sh | 2 +-
t/t7607-merge-overwrite.sh | 2 +-
unpack-trees.c | 4 +-
unpack-trees.h | 4 ++
7 files changed, 77 insertions(+), 24 deletions(-)
@@ -795,6 +800,20 @@ static int would_lose_untracked(const char *path)return!was_tracked(path)&&file_exists(path);}+staticintwas_dirty(structmerge_options*o,constchar*path)+{+structcache_entry*ce;+intdirty=1;++if(o->call_depth||!was_tracked(path))+return!dirty;++ce=cache_file_exists(path,strlen(path),ignore_case);+dirty=(ce->ce_stat_data.sd_mtime.sec>0&&+verify_uptodate(ce,&o->unpack_opts)!=0);+returndirty;+}+staticintmake_room_for_path(structmerge_options*o,constchar*path){intstatus,i;
@@ -2686,6 +2705,7 @@ static int handle_modify_delete(struct merge_options *o,staticintmerge_content(structmerge_options*o,constchar*path,+intfile_in_way,structobject_id*o_oid,into_mode,structobject_id*a_oid,inta_mode,structobject_id*b_oid,intb_mode,
@@ -2760,7 +2780,7 @@ static int merge_content(struct merge_options *o,return-1;}-if(df_conflict_remains){+if(df_conflict_remains||file_in_way){char*new_path;if(o->call_depth){remove_file_from_cache(path);
@@ -2794,6 +2814,30 @@ static int merge_content(struct merge_options *o,returnmfi.clean;}+staticintconflict_rename_normal(structmerge_options*o,+constchar*path,+structobject_id*o_oid,unsignedinto_mode,+structobject_id*a_oid,unsignedinta_mode,+structobject_id*b_oid,unsignedintb_mode,+structrename_conflict_info*ci)+{+intclean_merge;+intfile_in_the_way=0;++if(was_dirty(o,path)){+file_in_the_way=1;+output(o,1,_("Refusing to lose dirty file at %s"),path);+}++/* Merge the content and write it out */+clean_merge=merge_content(o,path,file_in_the_way,+o_oid,o_mode,a_oid,a_mode,b_oid,b_mode,+ci);+if(clean_merge>0&&file_in_the_way)+clean_merge=0;+returnclean_merge;+}+/* Per entry merge function */staticintprocess_entry(structmerge_options*o,constchar*path,structstage_data*entry)
@@ -2813,9 +2857,12 @@ static int process_entry(struct merge_options *o,switch(conflict_info->rename_type){caseRENAME_NORMAL:caseRENAME_ONE_FILE_TO_ONE:-clean_merge=merge_content(o,path,-o_oid,o_mode,a_oid,a_mode,b_oid,b_mode,-conflict_info);+clean_merge=conflict_rename_normal(o,+path,+o_oid,o_mode,+a_oid,a_mode,+b_oid,b_mode,+conflict_info);break;caseRENAME_DIR:clean_merge=1;
@@ -2911,7 +2958,7 @@ static int process_entry(struct merge_options *o,}elseif(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,+clean_merge=merge_content(o,path,0/* file_in_way */,o_oid,o_mode,a_oid,a_mode,b_oid,b_mode,NULL);}elseif(!o_oid&&!a_oid&&!b_oid){
@@ -2952,7 +2999,7 @@ int merge_trees(struct merge_options *o,return1;}-code=git_merge_trees(o->call_depth,common,head,merge);+code=git_merge_trees(o,common,head,merge);if(code!=0){if(show(o,4)||o->call_depth)
@@ -141,7 +141,7 @@ test_expect_success 'cherry-pick "-" works with arguments' 'test_cmpexpectactual'-test_expect_failure'cherry-pick works with dirty renamed file''+test_expect_success'cherry-pick works with dirty renamed file''test_committo-rename&&gitcheckout-bunrelated&&test_commitunrelated&&
@@ -92,7 +92,7 @@ test_expect_success 'will not overwrite removed file with staged changes' 'test_cmpimportantc1.c'-test_expect_failure'will not overwrite unstaged changes in renamed file''+test_expect_success'will not overwrite unstaged changes in renamed file''gitreset--hardc1&&gitmvc1.cother.c&&gitcommit-mrename&&
@@ -2879,4 +2879,371 @@ test_expect_failure '9g-check: Renamed directory that only contained immediate s# side of history for any implicit directory renames.###########################################################################+###########################################################################+# SECTION 10: Handling untracked files+#+# unpack_trees(), upon which the recursive merge algorithm is based, aborts+# the operation if untracked or dirty files would be deleted or overwritten+# by the merge. Unfortunately, unpack_trees() does not understand renames,+# and if it doesn't abort, then it muddies up the working directory before+# we even get to the point of detecting renames, so we need some special+# handling, at least in the case of directory renames.+###########################################################################++# Testcase 10a, Overwrite untracked: normal rename/delete+# Commit O: z/{b,c_1}+# Commit A: z/b + untracked z/c + untracked z/d+# Commit B: z/{b,d_1}+# Expected: Aborted Merge ++# ERROR_MSG(untracked working tree files would be overwritten by merge)++test_expect_success'10a-setup: Overwrite untracked with normal rename/delete''+test_create_repo10a&&+(+cd10a&&++mkdirz&&+echob>z/b&&+echoc>z/c&&+gitaddz&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+gitrmz/c&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+gitmvz/cz/d&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_success'10a-check: Overwrite untracked with normal rename/delete''+(+cd10a&&++gitcheckoutA^0&&+echovery>z/c&&+echoimportant>z/d&&++test_must_failgitmerge-srecursiveB^0>out2>err&&+test_i18ngrep"The following untracked working tree files would be overwritten by merge"err&&++gitls-files-s>out&&+test_line_count=1out&&+gitls-files-o>out&&+test_line_count=4out&&++echovery>expect&&+test_cmpexpectz/c&&++echoimportant>expect&&+test_cmpexpectz/d&&++gitrev-parseHEAD:z/b>actual&&+gitrev-parseO:z/b>expect&&+test_cmpexpectactual+)+'++# Testcase 10b, Overwrite untracked: dir rename + delete+# Commit O: z/{b,c_1}+# Commit A: y/b + untracked y/{c,d,e}+# Commit B: z/{b,d_1,e}+# Expected: Failed Merge; y/b + untracked y/c + untracked y/d on disk ++# z/c_1 -> z/d_1 rename recorded at stage 3 for y/d ++# ERROR_MSG(refusing to lose untracked file at 'y/d')++test_expect_success'10b-setup: Overwrite untracked with dir rename + delete''+test_create_repo10b&&+(+cd10b&&++mkdirz&&+echob>z/b&&+echoc>z/c&&+gitaddz&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+gitrmz/c&&+gitmvz/y/&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+gitmvz/cz/d&&+echoe>z/e&&+gitaddz/e&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_failure'10b-check: Overwrite untracked with dir rename + delete''+(+cd10b&&++gitcheckoutA^0&&+echovery>y/c&&+echoimportant>y/d&&+echocontents>y/e&&++test_must_failgitmerge-srecursiveB^0>out2>err&&+test_i18ngrep"CONFLICT (rename/delete).*Version B\^0 of y/d left in tree at y/d~B\^0"out&&+test_i18ngrep"Error: Refusing to lose untracked file at y/e; writing to y/e~B\^0 instead"out&&++gitls-files-s>out&&+test_line_count=3out&&+gitls-files-u>out&&+test_line_count=2out&&+gitls-files-o>out&&+test_line_count=5out&&++gitrev-parse>actual\+:0:y/b:3:y/d:3:y/e&&+gitrev-parse>expect\+O:z/bO:z/cB:z/e&&+test_cmpexpectactual&&++echovery>expect&&+test_cmpexpecty/c&&++echoimportant>expect&&+test_cmpexpecty/d&&++echocontents>expect&&+test_cmpexpecty/e+)+'++# Testcase 10c, Overwrite untracked: dir rename/rename(1to2)+# Commit O: z/{a,b}, x/{c,d}+# Commit A: y/{a,b}, w/c, x/d + different untracked y/c+# Commit B: z/{a,b,c}, x/d+# Expected: Failed Merge; y/{a,b} + x/d + untracked y/c ++# CONFLICT(rename/rename) x/c -> w/c vs y/c ++# y/c~B^0 ++# ERROR_MSG(Refusing to lose untracked file at y/c)++test_expect_success'10c-setup: Overwrite untracked with dir rename/rename(1to2)''+test_create_repo10c&&+(+cd10c&&++mkdirzx&&+echoa>z/a&&+echob>z/b&&+echoc>x/c&&+echod>x/d&&+gitaddzx&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+mkdirw&&+gitmvx/cw/c&&+gitmvz/y/&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+gitmvx/cz/&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_failure'10c-check: Overwrite untracked with dir rename/rename(1to2)''+(+cd10c&&++gitcheckoutA^0&&+echoimportant>y/c&&++test_must_failgitmerge-srecursiveB^0>out2>err&&+test_i18ngrep"CONFLICT (rename/rename)"out&&+test_i18ngrep"Refusing to lose untracked file at y/c; adding as y/c~B\^0 instead"out&&++gitls-files-s>out&&+test_line_count=6out&&+gitls-files-u>out&&+test_line_count=3out&&+gitls-files-o>out&&+test_line_count=3out&&++gitrev-parse>actual\+:0:y/a:0:y/b:0:x/d:1:x/c:2:w/c:3:y/c&&+gitrev-parse>expect\+O:z/aO:z/bO:x/dO:x/cO:x/cO:x/c&&+test_cmpexpectactual&&++githash-objecty/c~B^0>actual&&+gitrev-parseO:x/c>expect&&+test_cmpexpectactual&&++echoimportant>expect&&+test_cmpexpecty/c+)+'++# Testcase 10d, Delete untracked w/ dir rename/rename(2to1)+# Commit O: z/{a,b,c_1}, x/{d,e,f_2}+# Commit A: y/{a,b}, x/{d,e,f_2,wham_1} + untracked y/wham+# Commit B: z/{a,b,c_1,wham_2}, y/{d,e}+# Expected: Failed Merge; y/{a,b,d,e} + untracked y/{wham,wham~B^0,wham~HEAD}++# CONFLICT(rename/rename) z/c_1 vs x/f_2 -> y/wham+# ERROR_MSG(Refusing to lose untracked file at y/wham)++test_expect_success'10d-setup: Delete untracked with dir rename/rename(2to1)''+test_create_repo10d&&+(+cd10d&&++mkdirzx&&+echoa>z/a&&+echob>z/b&&+echoc>z/c&&+echod>x/d&&+echoe>x/e&&+echof>x/f&&+gitaddzx&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+gitmvz/cx/wham&&+gitmvz/y/&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+gitmvx/fz/wham&&+gitmvx/y/&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_failure'10d-check: Delete untracked with dir rename/rename(2to1)''+(+cd10d&&++gitcheckoutA^0&&+echoimportant>y/wham&&++test_must_failgitmerge-srecursiveB^0>out2>err&&+test_i18ngrep"CONFLICT (rename/rename)"out&&+test_i18ngrep"Refusing to lose untracked file at y/wham"out&&++gitls-files-s>out&&+test_line_count=6out&&+gitls-files-u>out&&+test_line_count=2out&&+gitls-files-o>out&&+test_line_count=4out&&++gitrev-parse>actual\+:0:y/a:0:y/b:0:y/d:0:y/e:2:y/wham:3:y/wham&&+gitrev-parse>expect\+O:z/aO:z/bO:x/dO:x/eO:z/cO:x/f&&+test_cmpexpectactual&&++test_must_failgitrev-parse:1:y/wham&&++echoimportant>expect&&+test_cmpexpecty/wham&&++githash-object>actual\+y/wham~B^0y/wham~HEAD&&+gitrev-parse>expect\+O:x/fO:z/c&&+test_cmpexpectactual+)+'++# Testcase 10e, Does git complain about untracked file that's not in the way?+# Commit O: z/{a,b}+# Commit A: y/{a,b} + untracked z/c+# Commit B: z/{a,b,c}+# Expected: y/{a,b,c} + untracked z/c++test_expect_success'10e-setup: Does git complain about untracked file that is not really in the way?''+test_create_repo10e&&+(+cd10e&&++mkdirz&&+echoa>z/a&&+echob>z/b&&+gitaddz&&+test_tick&&+gitcommit-m"O"&&++gitbranchO&&+gitbranchA&&+gitbranchB&&++gitcheckoutA&&+gitmvz/y/&&+test_tick&&+gitcommit-m"A"&&++gitcheckoutB&&+echoc>z/c&&+gitaddz/c&&+test_tick&&+gitcommit-m"B"+)+'++test_expect_failure'10e-check: Does git complain about untracked file that is not really in the way?''+(+cd10e&&++gitcheckoutA^0&&+mkdirz&&+echorandom>z/c&&++gitmerge-srecursiveB^0>out2>err&&+test_i18ngrep!"following untracked working tree files would be overwritten by merge"err&&++gitls-files-s>out&&+test_line_count=3out&&+gitls-files-u>out&&+test_line_count=0out&&+gitls-files-o>out&&+test_line_count=3out&&++gitrev-parse>actual\+:0:y/a:0:y/b:0:y/c&&+gitrev-parse>expect\+O:z/aO:z/bB:z/c&&+test_cmpexpectactual&&++echorandom>expect&&+test_cmpexpectz/c+)+'+ test_done
Before trying to apply directory renames to paths within the given
directories, we want to make sure that there aren't conflicts at the
file level either. If there aren't any, then get the new name from
any directory renames.
Reviewed-by: Stefan Beller <redacted>
Signed-off-by: Elijah Newren <redacted>
---
merge-recursive.c | 174 ++++++++++++++++++++++++++++++++++--
strbuf.c | 16 ++++
strbuf.h | 16 ++++
t/t6043-merge-rename-directories.sh | 2 +-
4 files changed, 199 insertions(+), 9 deletions(-)
@@ -1519,6 +1519,91 @@ static void remove_hashmap_entries(struct hashmap *dir_renames,string_list_clear(items_to_remove,0);}+/*+*Seeifthereisadirectoryrenameforpath,andifthereareanyfile+*levelconflictsfortherenamedlocation.Ifthereisarenameand+*therearenoconflicts,returnthenewname.Otherwise,returnNULL.+*/+staticchar*handle_path_level_conflicts(structmerge_options*o,+constchar*path,+structdir_rename_entry*entry,+structhashmap*collisions,+structtree*tree)+{+char*new_path=NULL;+structcollision_entry*collision_ent;+intclean=1;+structstrbufcollision_paths=STRBUF_INIT;++/*+*entryhasthemappingofolddirectorynametonewdirectoryname+*thatwewanttoapplytopath.+*/+new_path=apply_dir_rename(entry,path);++if(!new_path){+/* This should only happen when entry->non_unique_new_dir set */+if(!entry->non_unique_new_dir)+BUG("entry->non_unqiue_dir not set and !new_path");+output(o,1,_("CONFLICT (directory rename split): "+"Unclear where to place %s because directory "+"%s was renamed to multiple other directories, "+"with no destination getting a majority of the "+"files."),+path,entry->dir);+clean=0;+returnNULL;+}++/*+*Thecallerneedstohaveensuredthatithaspre-populated+*collisionswithallpathsthatmaptonew_path.Doaquickcheck+*toensurethat'sthecase.+*/+collision_ent=collision_find_entry(collisions,new_path);+if(collision_ent==NULL)+BUG("collision_ent is NULL");++/*+*Checkforone-sidedadd/add/.../addconflicts,i.e.+*whereimplicitrenamesfromtheothersidedoing+*directoryrename(s)canaffectthissideofhistory+*toputmultiplepathsintothesamelocation.Warn+*andbailondirectoryrenamesforsuchpaths.+*/+if(collision_ent->reported_already){+clean=0;+}elseif(tree_has_path(tree,new_path)){+collision_ent->reported_already=1;+strbuf_add_separated_string_list(&collision_paths,", ",+&collision_ent->source_files);+output(o,1,_("CONFLICT (implicit dir rename): Existing "+"file/dir at %s in the way of implicit "+"directory rename(s) putting the following "+"path(s) there: %s."),+new_path,collision_paths.buf);+clean=0;+}elseif(collision_ent->source_files.nr>1){+collision_ent->reported_already=1;+strbuf_add_separated_string_list(&collision_paths,", ",+&collision_ent->source_files);+output(o,1,_("CONFLICT (implicit dir rename): Cannot map "+"more than one path to %s; implicit directory "+"renames tried to put these paths there: %s"),+new_path,collision_paths.buf);+clean=0;+}++/* Free memory we no longer need */+strbuf_release(&collision_paths);+if(!clean&&new_path){+free(new_path);+returnNULL;+}++returnnew_path;+}+/**Thereareacouplethingswewanttodoatthedirectorylevel:*1.Checkforbothsidesrenamingtothesamething,inordertoavoid
@@ -489,7 +489,7 @@ test_expect_success '2a-setup: Directory split into two on one side, with equal)'-test_expect_failure'2a-check: Directory split into two on one side, with equal numbers of paths''+test_expect_success'2a-check: Directory split into two on one side, with equal numbers of paths''(cd2a&&