Re: [PATCH 2/2] merge-file: consider core.crlf when writing merge markers

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 22, 2016, 19:08 UTC
Message-ID: <xmqqa8nxv3do.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <alpine.DEB.2.20.1601221937440.2964@virtualbox>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 30 quoted lines

>> > It is the duty of ll_merge()'s *caller* (in case of Git's `merge`
>> > command, the merge_content() function) to convert the merge result
>> > into the correct working file contents, and ll_merge() should not muck
>> > with line endings at all.
>> 
>> Hmph, aren't there people who use CRLF throughout their set-up (that is,
>> it is normal for both of their blobs and working tree files to use CRLF
>> line endings)?  Or is such a setting illegal and unsupported?
>
> Good point.
>
> While I would love to simply not support such a case, it would be turning
> a blind eye to reality.
>
> So I guess I need another patch like this (plus a test case):
>
> -- snipsnap --
> t a/ll-merge.c b/ll-merge.c
> index 0338630..4a565c8 100644
> --- a/ll-merge.c
> +++ b/ll-merge.c
> @@ -111,6 +111,7 @@ static int ll_xdl_merge(const struct ll_merge_driver
> *drv_unused,
>  		xmp.style = git_xmerge_style;
>  	if (marker_size > 0)
>  		xmp.marker_size = marker_size;
> +	xmp.crlf = eol_for_path(NULL, src1->ptr, src1->size) == EOL_CRLF;
>  	xmp.ancestor = orig_name;
>  	xmp.file1 = name1;
>  	xmp.file2 = name2;

What I do not understand is that you already had xmp.crlf even the log message claimed that the caller is supposed to feed LF blobs and the ll_merge() shouldn't have to worry about this.

If the user sets the repository up to use CRLF in working tree and LF in blob (e.g. crlf = input), shouldn't cmd_merge_file() be doing the convert_to_git() to the buffer stored in mmfs[] before calling down to xdl_merge() so that ll_merge() layer will see LF not CRLF?

And if the interface is truly done in such a way that was outlined in the proposed log message, I do not think xmp.crlf is a good name for the new field. What the updated code needs is a boolean option, xmp.end_marker_with_crlf, that is set only when the internal blob encoding is CRLF and affects only the ending of the marker strings.

It looks to me that the code is doing something different from what the proposed log message claimed, which is puzzling.