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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jan 20, 2024, 14:13 UTC
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:

Show 6 quoted lines

> 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".

Show 6 quoted lines

> [...]
> +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.

Show 5 quoted lines

>   			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