Re: [PATCH v2 07/17] merge-ort: implement compute_rename_counts()

From: Taylor Blau <me@ttaylorr.com>
To: Elijah Newren via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	Elijah Newren <newren@gmail.com>
Subject: Re: [PATCH v2 07/17] merge-ort: implement compute_rename_counts()
Date: Mon, 18 Jan 2021 15:36:30 -0500	[thread overview]
Message-ID: <YAXxTsnQ0gqQZv75@nand.local> (raw)
In-Reply-To: <1e48cde01b9e2fe24eeda063e0298db8421b13a7.1610055365.git.gitgitgadget@gmail.com>

On Thu, Jan 07, 2021 at 09:35:55PM +0000, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
>
> This function is based on the first half of get_directory_renames() from
> merge-recursive.c
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  merge-ort.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 51 insertions(+), 2 deletions(-)
>
> diff --git a/merge-ort.c b/merge-ort.c
> index 7e0cc597055..a8fcc026031 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -721,7 +721,6 @@ static int handle_content_merge(struct merge_options *opt,
>
>  /*** Function Grouping: functions related to directory rename detection ***/
>
> -MAYBE_UNUSED
>  static void get_renamed_dir_portion(const char *old_path, const char *new_path,
>  				    char **old_dir, char **new_dir)
>  {
> @@ -825,11 +824,61 @@ static void get_renamed_dir_portion(const char *old_path, const char *new_path,
>  	*new_dir = xstrndup(new_path, end_of_new - new_path);
>  }
>
> +static void increment_count(struct strmap *dir_rename_count,
> +			    char *old_dir,
> +			    char *new_dir)
> +{
> +	struct strintmap *counts;
> +	struct strmap_entry *e;
> +
> +	/* Get the {new_dirs -> counts} mapping using old_dir */
> +	e = strmap_get_entry(dir_rename_count, old_dir);
> +	if (e) {
> +		counts = e->value;
> +	} else {
> +		counts = xmalloc(sizeof(*counts));
> +		strintmap_init_with_options(counts, 0, NULL, 1);
> +		strmap_put(dir_rename_count, old_dir, counts);
> +	}
> +
> +	/* Increment the count for new_dir */
> +	strintmap_incr(counts, new_dir, 1);
> +}
> +
>  static void compute_rename_counts(struct diff_queue_struct *pairs,
>  				  struct strmap *dir_rename_count,
>  				  struct strset *dirs_removed)
>  {
> -	die("Not yet implemented!");
> +	int i;
> +
> +	for (i = 0; i < pairs->nr; ++i) {
> +		char *old_dir, *new_dir;
> +		struct diff_filepair *pair = pairs->queue[i];
> +
> +		if (pair->status != 'R')
> +			continue;

This had a useful comment in merge-recursive.c that I think would help
future readers here. Would you consider brining over the comment that
starts with "File not part of directory rename ..." here?

> +		/* Get the old and new directory names */
> +		get_renamed_dir_portion(pair->one->path, pair->two->path,
> +					&old_dir,        &new_dir);

This spacing is a little odd to me, but it does come directly from
merge-recursive.c. I don't feel strongly about it.

> +		if (!old_dir)
> +			/* Directory didn't change at all; ignore this one. */
> +			continue;
> +
> +		/*
> +		 * Make dir_rename_count contain a map of a map:
> +		 *   old_directory -> {new_directory -> count}
> +		 * In other words, for every pair look at the directories for
> +		 * the old filename and the new filename and count how many
> +		 * times that pairing occurs.
> +		 */
> +		if (strset_contains(dirs_removed, old_dir))
> +			increment_count(dir_rename_count, old_dir, new_dir);

Much clearer. If you're rerolling anyway, it may be worth it to say
something about this movement in the patch message: "along the way,
factor out the routine to update the bookkeeping to track the number of
items renamed into new directories".

> +		/* Free resources we don't need anymore */
> +		free(old_dir);
> +		free(new_dir);
> +	}

And obviously this is new, but it makes sense here. Thanks.

Thanks,
Taylor