From: Phillip Wood <phillip.wood123@gmail.com>
Date: Sat, 20 Jan 2024 14:13:50 GMT
Subject: Re: [PATCH v3] merge-ll: expose revision names to custom drivers
Message-ID: <82624802-aa7f-4856-b819-9a2990b25a69@gmail.com>
In-Reply-To: <pull.1648.v3.git.git.1705615794307.gitgitgadget@gmail.com>

Hi Antonin

On 18/01/2024 22:09, Antonin Delpeuch via GitGitGadget wrote:
> From: Antonin Delpeuch <antonin@delpeuch.eu>
> 
> Custom merge drivers need access to the names of the revisions they
> are working on, so that the merge conflict markers they introduce
> can refer to those revisions. The placeholders '%S', '%X' and '%Y'
> are introduced to this end.

Thanks for working on this, I think it is a useful improvement. I guess 
'%X' and '%Y' are no worse than the existing '%A' and '%B' but I do 
wonder if we want to take the opportunity to switch to more descriptive 
names for the various parameters passed to the custom merge strategy. We 
do do this by supporting %(label:ours) modeled after the format 
specifiers used by other commands such as "git log" and "git for-each-ref".

> [...]
> +will be stored via placeholder `%P`. Additionally, the names of the
> +common ancestor revision (`%S`), of the current revision (`%X`) and
> +of the other branch (`%Y`) can also be supplied. Those are short > +revision names, optionally joined with the paths of the file in each
> +revision. Those paths are only present if they differ and are separated
> +from the revision by a colon.

It might be simpler to just call these the "conflict marker labels" 
without tying ourselves to a particular format. Something like

     The conflict labels to be used for the common ancestor, local head
     and other head can be passed by using '%(label:base)',
     '%(label:ours)' and '%(label:theirs) respectively.


> @@ -222,6 +222,12 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,

Not part of this patch but I noticed that we're passing the filenames 
for '%A' etc. unquoted which is a bit scary.

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

I think you can avoid the SIGSEV problem you mentioned in your other 
email by changing this to

	sq_quote_buf(&cmd, orig_name ? orig_name, "");

That would make sure the labels we pass match the ones used by the 
internal merge.

Best Wishes

Phillip