Re: [PATCH v3] merge-ll: expose revision names to custom drivers

From: Antonin Delpeuch <antonin@delpeuch.eu>
Date: Jan 19, 2024, 20:02 UTC
Message-ID: <386c0318-0138-47c9-9e7f-d1004277226c@delpeuch.eu>
In-Reply-To: <pull.1648.v3.git.git.1705615794307.gitgitgadget@gmail.com>

Hi Junio,

After more testing (combining custom merge drivers with rerere) I realized that my patch can lead to a segmentation error. Many apologies for not having caught that earlier!

On 18/01/2024 23:09, Antonin Delpeuch via GitGitGadget wrote:

Show 10 quoted lines

> @@ -222,6 +222,12 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
>   			strbuf_addf(&cmd, "%d", marker_size);
>   		else if (skip_prefix(format, "P", &format))
>   			sq_quote_buf(&cmd, path);
> +		else if (skip_prefix(format, "S", &format))
> +			sq_quote_buf(&cmd, orig_name);
> +		else if (skip_prefix(format, "X", &format))
> +			sq_quote_buf(&cmd, name1);
> +		else if (skip_prefix(format, "Y", &format))
> +			sq_quote_buf(&cmd, name2);

The "orig_name", "name1" and "name2" pointers can be NULL at this stage. This can happen when the merge is invoked from rerere, to resolve a conflict using a previous resolution.

I wonder what the appropriate fallback would be in such a case. I am tempted to use the temporary filenames of the files to merge instead, so that the merge driver can rely on those names being non-empty and being the best string to use to identify the files. Passing an empty string seems dangerous to me, as it is likely to change the index of arguments passed to the merge driver. Passing fixed strings such as "base", "ours" and "theirs" could perhaps work too.

Let me know if you have any preference about this.

Best,

Antonin