Re: [PATCH 03/10] merge-ort: implement unique_path() helper

From: Derrick Stolee <stolee@gmail.com>
To: Elijah Newren via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org
Cc: Elijah Newren <newren@gmail.com>
Subject: Re: [PATCH 03/10] merge-ort: implement unique_path() helper
Date: Wed, 30 Dec 2020 09:16:28 -0500	[thread overview]
Message-ID: <3a07f700-18fc-9f4e-3117-41e534d737b0@gmail.com> (raw)
In-Reply-To: <6ac555b3c0fe605fbbe6e304482c2e3aef321865.1608270687.git.gitgitgadget@gmail.com>

On 12/18/2020 12:51 AM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> Implement unique_path(), based on the one from merge-recursive.c.  It is
> simplified, however, due to: (1) using strmaps, and (2) the fact that
> merge-ort lets the checkout codepath handle possible collisions with the
> working tree means that other code locations don't have to.
> 
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  merge-ort.c | 25 ++++++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/merge-ort.c b/merge-ort.c
> index d300a02810e..1adc27a11bc 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -343,11 +343,34 @@ static void path_msg(struct merge_options *opt,
>  	strbuf_addch(sb, '\n');
>  }
>  
> +/* add a string to a strbuf, but converting "/" to "_" */
> +static void add_flattened_path(struct strbuf *out, const char *s)
> +{
> +	size_t i = out->len;
> +	strbuf_addstr(out, s);
> +	for (; i < out->len; i++)
> +		if (out->buf[i] == '/')
> +			out->buf[i] = '_';
> +}
> +

Thank you for pointing out that you based your code on merge-recursive.c.
I see that this implementation is identical to the one there. I question
whether this causes collisions in a problematic way, when "a/b/c" and
"a_b_c" both exist in a tree.

To avoid such a problem, we'd likely need to also expand "_" to "__" or
similar. This might never actually affect any users because of the
strange filename matches _and_ we need to be in a directory/file conflict.

And maybe it's not a hole at all? If it is, we can consider patching or
at least documenting the problem.

Thanks,
-Stolee