Re: [PATCH v2 2/3] merge-ort: shuffle the computation and cleanup of potential collisions

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Jun 30 2022, Elijah Newren via GitGitGadget wrote:

> From: Elijah Newren <newren@gmail.com>
>
> Run compute_collisions() for renames on both sides of history before
> any calls to collect_renames(), and do not free the computed collisions
> until after both calls to collect_renames().  This is just a code
> reorganization at this point that doesn't make sense on its own, but
> will permit us to use the computed collision info from both sides
> within each call to collect_renames() in a subsequent commit.

I think this would be easier to follow if split in two, since...

> +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);
> +}

...a large part of it...
>  
> -	/* 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);

..is moving existing code into a utility function...

>  	return clean;
>  }
>  
> @@ -3100,6 +3105,7 @@ static int detect_and_process_renames(struct merge_options *opt,
>  {
>  	struct diff_queue_struct combined = { 0 };
>  	struct rename_info *renames = &opt->priv->renames;
> +	struct strmap collisions[3];
>  	int need_dir_renames, s, i, clean = 1;
>  	unsigned detection_run = 0;
>  
> @@ -3149,12 +3155,22 @@ static int detect_and_process_renames(struct merge_options *opt,
>  	ALLOC_GROW(combined.queue,
>  		   renames->pairs[1].nr + renames->pairs[2].nr,
>  		   combined.alloc);
> +	for (int i = MERGE_SIDE1; i <= MERGE_SIDE2; i++) {
> +		int other_side = 3 - i;
> +		compute_collisions(&collisions[i],
> +				   &renames->dir_renames[other_side],
> +				   &renames->pairs[i]);
> +	}
>  	clean &= collect_renames(opt, &combined, MERGE_SIDE1,
> +				 collisions,
>  				 &renames->dir_renames[2],
>  				 &renames->dir_renames[1]);
>  	clean &= collect_renames(opt, &combined, MERGE_SIDE2,
> +				 collisions,
>  				 &renames->dir_renames[1],
>  				 &renames->dir_renames[2]);
> +	for (int i = MERGE_SIDE1; i <= MERGE_SIDE2; i++)
> +		free_collisions(&collisions[i]);
>  	STABLE_QSORT(combined.queue, combined.nr, compare_pairs);
>  	trace2_region_leave("merge", "directory renames", opt->repo);

...which when looking at it makes the functional change harder to follow
than it otherwise would be.