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

From: Derrick Stolee <stolee@gmail.com>
Date: Feb 24, 2021, 17:35 UTC
Message-ID: <aaf7c657-ef16-7612-166f-01560918d82d@gmail.com>
In-Reply-To: <cbd055ab33998962cb7712906cdad5dff3390660.1614123848.git.gitgitgadget@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.

Show 13 quoted lines

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

Show 5 quoted lines

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

Does what it says it does.

Thanks, -Stolee