git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Junio C Hamano <gitster@pobox.com>
Date
Jan 22, 2016, 18:15 UTC
Message-ID
<xmqqmvrxv5tn.fsf@gitster.mtv.corp.google.com>
In-Reply-To
<c0c775ea7a9ba3244748b784241de685cefc73b1.1453482052.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 9 quoted lines
> 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.

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?

If such a set-up is supported, using "<<<\r\n" etc. instead of the usual "<<<\n" would be the correct thing to do.

In a setting where people use whatever they like on the filesystem but their blobs are standardized to use LF, you are right. Contents that come from the filesystem and from the object store should be made to the internal format, by using convert_to_git() as necessary, and adding the markers with "<<<\n" without CR, and writing out the result as CRLF (or whatever their local convention is) is absolutely the right thing to do.

It's just I am not sure what our stance is about storing CRLF blobs. IIRC, in the meeting we met in person the last time I thought I heard some from the Windows camp talk against use of any of the core.crlf and other settings, saying that it is much better using the platform native convention throughout, and I got an impression that it is a recommended practice at least in some circles.

Would the approach taken by this patch still work for them, too?
Show 184 quoted lines
> 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;
>  	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
> +'
> +
> +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
> +'
> +
>  test_done
> diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
> index c033991..a5bea4a 100644
> --- a/xdiff/xdiff.h
> +++ b/xdiff/xdiff.h
> @@ -122,6 +122,7 @@ typedef struct s_xmparam {
>  	int level;
>  	int favor;
>  	int style;
> +	int crlf;
>  	const char *ancestor;	/* label for orig */
>  	const char *file1;	/* label for mf1 */
>  	const char *file2;	/* label for mf2 */
> diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c
> index 625198e..4ff0db4 100644
> --- a/xdiff/xmerge.c
> +++ b/xdiff/xmerge.c
> @@ -146,7 +146,7 @@ static int xdl_orig_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest)
>  static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
>  			      xdfenv_t *xe2, const char *name2,
>  			      const char *name3,
> -			      int size, int i, int style,
> +			      int size, int i, int style, int crlf,
>  			      xdmerge_t *m, char *dest, int marker_size)
>  {
>  	int marker1_size = (name1 ? strlen(name1) + 1 : 0);
> @@ -161,7 +161,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
>  			      dest ? dest + size : NULL);
>  
>  	if (!dest) {
> -		size += marker_size + 1 + marker1_size;
> +		size += marker_size + 1 + crlf + marker1_size;
>  	} else {
>  		memset(dest + size, '<', marker_size);
>  		size += marker_size;
> @@ -170,6 +170,8 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
>  			memcpy(dest + size + 1, name1, marker1_size - 1);
>  			size += marker1_size;
>  		}
> +		if (crlf)
> +			dest[size++] = '\r';
>  		dest[size++] = '\n';
>  	}
>  
> @@ -180,7 +182,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
>  	if (style == XDL_MERGE_DIFF3) {
>  		/* Shared preimage */
>  		if (!dest) {
> -			size += marker_size + 1 + marker3_size;
> +			size += marker_size + 1 + crlf + marker3_size;
>  		} else {
>  			memset(dest + size, '|', marker_size);
>  			size += marker_size;
> @@ -189,6 +191,8 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
>  				memcpy(dest + size + 1, name3, marker3_size - 1);
>  				size += marker3_size;
>  			}
> +			if (crlf)
> +				dest[size++] = '\r';
>  			dest[size++] = '\n';
>  		}
>  		size += xdl_orig_copy(xe1, m->i0, m->chg0, 1,
> @@ -196,10 +200,12 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
>  	}
>  
>  	if (!dest) {
> -		size += marker_size + 1;
> +		size += marker_size + 1 + crlf;
>  	} else {
>  		memset(dest + size, '=', marker_size);
>  		size += marker_size;
> +		if (crlf)
> +			dest[size++] = '\r';
>  		dest[size++] = '\n';
>  	}
>  
> @@ -207,7 +213,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
>  	size += xdl_recs_copy(xe2, m->i2, m->chg2, 1,
>  			      dest ? dest + size : NULL);
>  	if (!dest) {
> -		size += marker_size + 1 + marker2_size;
> +		size += marker_size + 1 + crlf + marker2_size;
>  	} else {
>  		memset(dest + size, '>', marker_size);
>  		size += marker_size;
> @@ -216,6 +222,8 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
>  			memcpy(dest + size + 1, name2, marker2_size - 1);
>  			size += marker2_size;
>  		}
> +		if (crlf)
> +			dest[size++] = '\r';
>  		dest[size++] = '\n';
>  	}
>  	return size;
> @@ -226,7 +234,7 @@ static int xdl_fill_merge_buffer(xdfenv_t *xe1, const char *name1,
>  				 const char *ancestor_name,
>  				 int favor,
>  				 xdmerge_t *m, char *dest, int style,
> -				 int marker_size)
> +				 int crlf, int marker_size)
>  {
>  	int size, i;
>  
> @@ -237,8 +245,8 @@ static int xdl_fill_merge_buffer(xdfenv_t *xe1, const char *name1,
>  		if (m->mode == 0)
>  			size = fill_conflict_hunk(xe1, name1, xe2, name2,
>  						  ancestor_name,
> -						  size, i, style, m, dest,
> -						  marker_size);
> +						  size, i, style, crlf, m,
> +						  dest, marker_size);
>  		else if (m->mode & 3) {
>  			/* Before conflicting part */
>  			size += xdl_recs_copy(xe1, i, m->i1 - i, 0,
> @@ -419,6 +427,7 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1,
>  	int level = xmp->level;
>  	int style = xmp->style;
>  	int favor = xmp->favor;
> +	int crlf = xmp->crlf;
>  
>  	if (style == XDL_MERGE_DIFF3) {
>  		/*
> @@ -554,7 +563,7 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1,
>  		int size = xdl_fill_merge_buffer(xe1, name1, xe2, name2,
>  						 ancestor_name,
>  						 favor, changes, NULL, style,
> -						 marker_size);
> +						 crlf, marker_size);
>  		result->ptr = xdl_malloc(size);
>  		if (!result->ptr) {
>  			xdl_cleanup_merge(changes);
> @@ -563,7 +572,7 @@ static int xdl_do_merge(xdfenv_t *xe1, xdchange_t *xscr1,
>  		result->size = size;
>  		xdl_fill_merge_buffer(xe1, name1, xe2, name2,
>  				      ancestor_name, favor, changes,
> -				      result->ptr, style, marker_size);
> +				      result->ptr, style, crlf, marker_size);
>  	}
>  	return xdl_cleanup_merge(changes);
>  }
Previous: Johannes SchindelinNext: Johannes Schindelin
Message 10 of 55 in “Let merge-file write out conflict markers with correct EOLs”
  1. 0/2 Let merge-file write out conflict markers with correct EOLsJohannes Schindelin, Jan 22, 2016
  2. 1/2 convert: add a helper to determine the correct EOL for a given pathJohannes Schindelin, Jan 22, 2016
  3. Junio C HamanoJan 22, 2016
  4. Johannes SchindelinJan 22, 2016
  5. Torsten BögershausenJan 23, 2016
  6. Johannes SchindelinJan 24, 2016
  7. Torsten BögershausenJan 23, 2016
  8. Johannes SchindelinJan 24, 2016
  9. 2/2 merge-file: consider core.crlf when writing merge markersJohannes Schindelin, Jan 22, 2016
  10. Junio C HamanoJan 22, 2016
  11. Johannes SchindelinJan 22, 2016
  12. Junio C HamanoJan 22, 2016
  13. Johannes SchindelinJan 24, 2016
  14. Eric SunshineJan 22, 2016
  15. Eric SunshineJan 22, 2016
  16. Johannes SchindelinJan 24, 2016
  17. Eric SunshineJan 24, 2016
  18. Johannes SchindelinJan 25, 2016
  19. Torsten BögershausenJan 23, 2016
  20. Johannes SchindelinJan 24, 2016
  21. Beat BolliJan 22, 2016
  22. 0/1 Let merge-file write out conflict markers with correct EOLsJohannes Schindelin, Jan 24, 2016
  23. 1/1 merge-file: let conflict markers match end-of-line style of the contextJohannes Schindelin, Jan 24, 2016
  24. Torsten BögershausenJan 24, 2016
  25. Torsten BögershausenJan 24, 2016
  26. Johannes SchindelinJan 25, 2016
  27. Ramsay JonesJan 25, 2016
  28. Johannes SchindelinJan 26, 2016
  29. Ramsay JonesJan 26, 2016
  30. Johannes SchindelinJan 26, 2016
  31. Junio C HamanoJan 24, 2016
  32. Johannes SchindelinJan 25, 2016
  33. 0/2 Let merge-file write out conflict markers with correct EOLsJohannes Schindelin, Jan 25, 2016
  34. 1/2 merge-file: let conflict markers match end-of-line style of the contextJohannes Schindelin, Jan 25, 2016
  35. Torsten BögershausenJan 25, 2016
  36. Johannes SchindelinJan 25, 2016
  37. Junio C HamanoJan 25, 2016
  38. Johannes SchindelinJan 26, 2016
  39. Junio C HamanoJan 26, 2016
  40. 2/2 merge-file: ensure that conflict sections match eol styleJohannes Schindelin, Jan 25, 2016
  41. Johannes SchindelinJan 25, 2016
  42. 0/2 Let merge-file write out conflict markers with correct EOLsJohannes Schindelin, Jan 26, 2016
  43. 1/2 merge-file: let conflict markers match end-of-line style of the contextJohannes Schindelin, Jan 26, 2016
  44. Junio C HamanoJan 26, 2016
  45. Junio C HamanoJan 26, 2016
  46. Johannes SchindelinJan 27, 2016
  47. Junio C HamanoJan 27, 2016
  48. Johannes SchindelinJan 27, 2016
  49. Junio C HamanoJan 27, 2016
  50. Johannes SchindelinJan 28, 2016
  51. 2/2 merge-file: ensure that conflict sections match eol styleJohannes Schindelin, Jan 26, 2016
  52. 0/2 Let merge-file write out conflict markers with correct EOLsJohannes Schindelin, Jan 27, 2016
  53. 1/2 merge-file: let conflict markers match end-of-line style of the contextJohannes Schindelin, Jan 27, 2016
  54. 2/2 merge-file: ensure that conflict sections match eol styleJohannes Schindelin, Jan 27, 2016
  55. Junio C HamanoJan 27, 2016

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.