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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 20, 2024, 22:49 UTC
Message-ID: <xmqqcytvei3c.fsf@gitster.g>
In-Reply-To: <82624802-aa7f-4856-b819-9a2990b25a69@gmail.com>

Phillip Wood <phillip.wood123@gmail.com> writes:

Show 7 quoted lines

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

Perhaps. Unlike the --format option these commands take, the placeholders are never typed from the command line (they always are taken from the configuration file), so mnemonic value longer version gives over the current single-letter ones is not as valuable, while making the total line length longer. So I dunno.

Show 13 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.

Yeah, that sounds like a good improvement, even if we did not use the longhand placeholders and replaced %(label:{base,ours,theirs}) with %S, %X, and %Y.

Show 18 quoted lines

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

Makes sense. That would be much better than using hardcoded string "ours", "theirs", etc.