Re: [PATCH v2 08/10] diffcore-rename: add a new idx_possible_rename function

[Derrick Stolee <stolee@gmail.com>]

On 2/23/2021 6:44 PM, Elijah Newren via GitGitGadget wrote:> +static char *get_dirname(const char *filename)
> +{
> +	char *slash = strrchr(filename, '/');
> +	return slash ? xstrndup(filename, slash-filename) : xstrdup("");

My brain interpreted "slash-filename" as a single token on first
read, which confused me briefly. Inserting spaces would help
readers like me.

> +	 *   (4) Check if applying that directory rename to the original file
> +	 *       would result in a destination filename that is in the
> +	 *       potential rename set.  If so, return the index of the
> +	 *       destination file (the index within rename_dst).

> +	 * This function, idx_possible_rename(), is only responsible for (4).

This helps isolate the important step to care about for the implementation,
while the rest of the context is important, too.

> +	char *old_dir, *new_dir, *new_path;
> +	int idx;
> +
> +	if (!info->setup)
> +		return -1;
> +
> +	old_dir = get_dirname(filename);
> +	new_dir = strmap_get(&info->dir_rename_guess, old_dir);
> +	free(old_dir);
> +	if (!new_dir)
> +		return -1;
> +
> +	new_path = xstrfmt("%s/%s", new_dir, get_basename(filename));

This is running in a loop, so `xstrfmt()` might be overkill compared
to something like

	strbuf_addstr(&new_path, new_dir);
	strbuf_addch(&new_path, '/');
	strbuf_addstr(&new_path, get_basename(filename));

but maybe the difference is too small to notice. (notice the type
change to "struct strbuf new_path = STRBUF_INIT;")

> +
> +	idx = strintmap_get(&info->idx_map, new_path);
> +	free(new_path);
> +	return idx;
> +}

Does what it says it does.

Thanks,
-Stolee