[PATCH v3 0/5] Fix dual rename into each other plus conflicting adds

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@chooglen-macbookpro.roam.corp.google.com/

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@gmail.com>
      
       ## 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 () {