This series adds some testcases based on the tensorflow repository issue reported by Glen Choo at [1], demonstrating bugs in both the ort and recursive strategies. It also provides a fix for the ort strategy. Changes since v2: * Added a couple preparatory cleanup patches * Added a comment about why sub1/newfile is important to the new testcases * A couple other minor code cleanups Changes since v1: * Fixed some wording issues in comments, and added a bit more details to one of the commit messages [1] https://lore.kernel.org/git/kl6lee006mle.fsf@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/ Elijah Newren (5): t6423: add tests of dual directory rename plus add/add conflict merge-ort: small cleanups of check_for_directory_rename merge-ort: make a separate function for freeing struct collisions merge-ort: shuffle the computation and cleanup of potential collisions merge-ort: fix issue with dual rename and add/add conflict merge-ort.c | 74 +++++++++++++------- t/t6423-merge-rename-directories.sh | 105 ++++++++++++++++++++++++++++ 2 files changed, 153 insertions(+), 26 deletions(-) base-commit: e54793a95afeea1e10de1e5ad7eab914e7416250 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1268%2Fnewren%2Ffix-dual-rename-into-each-other-plus-conflicting-adds-v3 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1268/newren/fix-dual-rename-into-each-other-plus-conflicting-adds-v3 Pull-Request: https://github.com/gitgitgadget/git/pull/1268 Range-diff vs v2: 1: bf4c03d01d5 ! 1: a16a1c4d947 t6423: add tests of dual directory rename plus add/add conflict @@ t/t6423-merge-rename-directories.sh: test_expect_success '12k: Directory rename +# +# Expected: sub3/{file, newfile, sub2/other} +# CONFLICT (add/add): sub1/sub2/new_add_add_file ++# ++# Note that sub1/newfile is not extraneous. Directory renames are only ++# detected if they are needed, and they are only needed if the old directory ++# had a new file added on the opposite side of history. So sub1/newfile ++# is needed for there to be a sub1/ -> sub3/ rename. + +test_setup_12l () { + test_create_repo 12l_$1 && + ( + cd 12l_$1 && + -+ mkdir -p sub1 sub2 ++ mkdir sub1 sub2 + echo file >sub1/file && + echo other >sub2/other && + git add sub1 sub2 && @@ t/t6423-merge-rename-directories.sh: test_expect_success '12k: Directory rename + + test_must_fail git -c merge.directoryRenames=true merge -s recursive B^0 && + -+ git ls-files -s >out && -+ test_line_count = 5 out && ++ test_stdout_line_count = 5 git ls-files -s && + + git rev-parse >actual \ + :0:sub3/file :0:sub3/newfile :0:sub3/sub2/other \ @@ t/t6423-merge-rename-directories.sh: test_expect_success '12k: Directory rename + test_cmp expect actual && + + git ls-files -o >actual && -+ test_write_lines actual expect out >expect && ++ test_write_lines actual expect >expect && + test_cmp expect actual + ) +' @@ t/t6423-merge-rename-directories.sh: test_expect_success '12k: Directory rename + + test_must_fail git -c merge.directoryRenames=true merge -s recursive A^0 && + -+ git ls-files -s >out && -+ test_line_count = 5 out && ++ test_stdout_line_count = 5 git ls-files -s && + + git rev-parse >actual \ + :0:sub3/file :0:sub3/newfile :0:sub3/sub2/other \ @@ t/t6423-merge-rename-directories.sh: test_expect_success '12k: Directory rename + test_cmp expect actual && + + git ls-files -o >actual && -+ test_write_lines actual expect out >expect && ++ test_write_lines actual expect >expect && + test_cmp expect actual + ) +' -: ----------- > 2: 297fef60b19 merge-ort: small cleanups of check_for_directory_rename -: ----------- > 3: f5f87acbbd2 merge-ort: make a separate function for freeing struct collisions 2: cfa38f01481 ! 4: d3eac3d0bf6 merge-ort: shuffle the computation and cleanup of potential collisions @@ Commit message Signed-off-by: Elijah Newren <newren@xxxxxxxxx> ## merge-ort.c ## -@@ merge-ort.c: static void compute_collisions(struct strmap *collisions, - } - } - -+static void free_collisions(struct strmap *collisions) -+{ -+ struct hashmap_iter iter; -+ struct strmap_entry *entry; -+ -+ /* Free each value in the collisions map */ -+ strmap_for_each_entry(collisions, &iter, entry) { -+ struct collision_info *info = entry->value; -+ string_list_clear(&info->source_files, 0); -+ } -+ /* -+ * In compute_collisions(), we set collisions.strdup_strings to 0 -+ * so that we wouldn't have to make another copy of the new_path -+ * allocated by apply_dir_rename(). But now that we've used them -+ * and have no other references to these strings, it is time to -+ * deallocate them. -+ */ -+ free_strmap_strings(collisions); -+ strmap_clear(collisions, 1); -+} -+ - static char *check_for_directory_rename(struct merge_options *opt, - const char *path, - unsigned side_index, @@ merge-ort.c: static char *check_for_directory_rename(struct merge_options *opt, } @@ merge-ort.c: static int detect_regular_renames(struct merge_options *opt, int i, clean = 1; - struct strmap collisions; struct diff_queue_struct *side_pairs; -- struct hashmap_iter iter; -- struct strmap_entry *entry; struct rename_info *renames = &opt->priv->renames; side_pairs = &renames->pairs[side_index]; @@ merge-ort.c: static int collect_renames(struct merge_options *opt, result->queue[result->nr++] = p; } -- /* Free each value in the collisions map */ -- strmap_for_each_entry(&collisions, &iter, entry) { -- struct collision_info *info = entry->value; -- string_list_clear(&info->source_files, 0); -- } -- /* -- * In compute_collisions(), we set collisions.strdup_strings to 0 -- * so that we wouldn't have to make another copy of the new_path -- * allocated by apply_dir_rename(). But now that we've used them -- * and have no other references to these strings, it is time to -- * deallocate them. -- */ -- free_strmap_strings(&collisions); -- strmap_clear(&collisions, 1); +- free_collisions(&collisions); return clean; } 3: da3ae38e390 ! 5: 121761e26e2 merge-ort: fix issue with dual rename and add/add conflict @@ Commit message ## merge-ort.c ## @@ merge-ort.c: static char *check_for_directory_rename(struct merge_options *opt, struct strmap_entry *rename_info; - struct strmap_entry *otherinfo = NULL; + struct strmap_entry *otherinfo; const char *new_dir; + int other_side = 3 - side_index; +- /* Cases where we don't have a directory rename for this path */ + /* + * Cases where we don't have or don't want a directory rename for -+ * this path, so we return NULL. ++ * this path. + */ if (strmap_empty(dir_renames)) - return new_path; + return NULL; + if (strmap_get(&collisions[other_side], path)) -+ return new_path; ++ return NULL; rename_info = check_dir_renamed(path, dir_renames); if (!rename_info) - return new_path; + return NULL; ## t/t6423-merge-rename-directories.sh ## @@ t/t6423-merge-rename-directories.sh: test_setup_12l () { -- gitgitgadget
