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

["Torsten Bögershausen" <tboegi@web.de>]

On 2016-01-22 18.01, Johannes Schindelin wrote:
> From: Beat Bolli <dev+git@drbeat.li>
> 
> When merging files in repos with core.eol = crlf, git merge-file inserts
> just a LF at the end of the merge markers. Files with mixed line endings
> cause trouble in Windows editors and e.g. contrib/git-jump, where an
> unmerged file in a run of "git jump merge" is reported as simply "binary
> file matches".
> 
> Fixing this improves merge-file's behavior under Windows.
> 
> The original version of this patch also modified ll_merge(), but that
> was incorrect: low-level merge operates on blobs, not on working files.
> Therefore, the data passed to the low-level merge, as well as its
> result, is expected to have LF-only line endings.
> 
> 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.
> 
> Signed-off-by: Beat Bolli <dev+git@drbeat.li>
> Signed-off-by: Jeff King <peff@peff.net>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/merge-file.c  |  1 +
>  t/t6023-merge-file.sh | 14 ++++++++++++++
>  xdiff/xdiff.h         |  1 +
>  xdiff/xmerge.c        | 29 +++++++++++++++++++----------
>  4 files changed, 35 insertions(+), 10 deletions(-)
> 
> diff --git a/builtin/merge-file.c b/builtin/merge-file.c
> index 5544705..9ce830a 100644
> --- a/builtin/merge-file.c
> +++ b/builtin/merge-file.c
> @@ -81,6 +81,7 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
>  					argv[i]);
>  	}
>  
> +	xmp.crlf = eol_for_path(names[0], NULL, 0) == EOL_CRLF;
2 comments:
What happens when the original file already has CRLF ?
And why don't we feed any content into the function ?
When the source is already in CRLF, do we need to add any CR ?

Or is the content feed into the merge machine always normalized ?
And in this case you can skip this comment completely.
>  	xmp.ancestor = names[1];
>  	xmp.file1 = names[0];
>  	xmp.file2 = names[2];
> diff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh
> index 190ee90..a131749 100755
> --- a/t/t6023-merge-file.sh
> +++ b/t/t6023-merge-file.sh
> @@ -346,4 +346,18 @@ test_expect_success 'conflict at EOF without LF resolved by --union' \
>  	 printf "line1\nline2\nline3x\nline3y" >expect.txt &&
>  	 test_cmp expect.txt output.txt'
>  
> +test_expect_success 'conflict markers contain CRLF when core.eol=crlf' '
> +	test_must_fail git -c core.eol=crlf merge-file -p \
> +		nolf-diff1.txt nolf-orig.txt nolf-diff2.txt >output.txt &&
> +	test $(sed -n "/\.txt\r$/p" output.txt | wc -l) = 3
$(wc -l)  == 3 tends to be non-portable - either use "test .. -eq" or
use test_line_count()
> +'

> +
> +test_expect_success 'conflict markers heed gitattributes over core.eol=crlf' '
> +	git config core.eol crlf &&
> +	echo "*.txt eol=lf" >>.gitattributes &&
> +	test_must_fail git -c core.eol=crlf merge-file -p \
> +		nolf-diff1.txt nolf-orig.txt nolf-diff2.txt >output.txt &&
> +	test $(sed -n "/\.txt\r$/p" output.txt | wc -l) = 0
Same here
And If I remember that "\r" in sed is non-portable, Ramsay suggested a nice fix:
gmane/283262
"t6023 broken under Mac OS"