Re: [PATCH v2 05/17] merge-ort: add outline of get_provisional_directory_renames()

[Taylor Blau <me@ttaylorr.com>]

On Thu, Jan 07, 2021 at 09:35:53PM +0000, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
>
> This function is based on merge-recursive.c's get_directory_renames(),
> except that the first half has been split out into a not-yet-implemented
> compute_rename_counts().  The primary difference here is our lack of the
> non_unique_new_dir boolean in our strmap.  The lack of that field will
> at first cause us to fail testcase 2b of t6423; however, future
> optimizations will obviate the need for that ugly field so we have just
> left it out.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  merge-ort.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 59 insertions(+), 1 deletion(-)
>
> diff --git a/merge-ort.c b/merge-ort.c
> index 378ac495d09..73d3ff97f52 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -721,11 +721,69 @@ static int handle_content_merge(struct merge_options *opt,
>
>  /*** Function Grouping: functions related to directory rename detection ***/
>
> +static void compute_rename_counts(struct diff_queue_struct *pairs,
> +				  struct strmap *dir_rename_count,
> +				  struct strset *dirs_removed)
> +{
> +	die("Not yet implemented!");
> +}
> +
>  static void get_provisional_directory_renames(struct merge_options *opt,
>  					      unsigned side,
>  					      int *clean)
>  {
> -	die("Not yet implemented!");
> +	struct hashmap_iter iter;
> +	struct strmap_entry *entry;
> +	struct rename_info *renames = &opt->priv->renames;
> +
> +	compute_rename_counts(&renames->pairs[side],
> +			      &renames->dir_rename_count[side],
> +			      &renames->dirs_removed[side]);
> +	/*
> +	 * Collapse
> +	 *    dir_rename_count: old_directory -> {new_directory -> count}
> +	 * down to
> +	 *    dir_renames: old_directory -> best_new_directory
> +	 * where best_new_directory is the one with the unique highest count.
> +	 */
> +	strmap_for_each_entry(&renames->dir_rename_count[side], &iter, entry) {
> +		const char *source_dir = entry->key;
> +		struct strintmap *counts = entry->value;
> +		struct hashmap_iter count_iter;
> +		struct strmap_entry *count_entry;
> +		int max = 0;
> +		int bad_max = 0;
> +		const char *best = NULL;
> +
> +		strintmap_for_each_entry(counts, &count_iter, count_entry) {
> +			const char *target_dir = count_entry->key;
> +			intptr_t count = (intptr_t)count_entry->value;

Just to make sure I understand what's going on here: you're storing the
whole int inside the pointer (and not pointing at it)?

> +			if (count == max)
> +				bad_max = max;
> +			else if (count > max) {
> +				max = count;
> +				best = target_dir;
> +			}
> +		}
> +
> +		if (max == 0)
> +			continue;

This is new from merge_recursive.c:get_directory_renames(). What is it
doing here?

> +		if (bad_max == max) {
> +			path_msg(opt, source_dir, 0,
> +			       _("CONFLICT (directory rename split): "
> +				 "Unclear where to rename %s to; it was "
> +				 "renamed to multiple other directories, with "
> +				 "no destination getting a majority of the "
> +				 "files."),
> +			       source_dir);
> +			*clean &= 0;

Also not a big deal, but why not simply '*clean = 0'?

> +		} else {
> +			strmap_put(&renames->dir_renames[side],
> +				   source_dir, (void*)best);

Ah, answering my onw question: this is indeed shoving the int into your
void*. That said, shouldn't this be '(void*)(intptr_t)best'?

Thanks,
Taylor