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

[PATCH v2 0/1] Let merge-file write out conflict markers with correct EOLs

From
Johannes Schindelin <johannes.schindelin@gmx.de>
Date
Jan 24, 2016, 10:48 UTC
Message-ID
<cover.1453632296.git.johannes.schindelin@gmx.de>
In-Reply-To
<cover.1453482052.git.johannes.schindelin@gmx.de>

The original patch was sent by Beat Bolli in http://thread.gmane.org/gmane.comp.version-control.git/281600

My suggestion to extend it to respect gitattributes led to changes that broke the original patch. And they were misguided to begin with (see below).

Since there have been a couple of "What's cooking" mails containing unheard calls for updates on this patch, I took it on myself to fix things.

Junio's comment that we might look at blobs containing CR/LF line endings made me rethink the entire approach, and I am now convinced that we need to abandon the entire idea to make the conflict markers depend on config settings or attributes: after all, I introduced `git merge-file` as a replacement for GNU merge that can be used *outside* of any repository, by design.

The crucial idea hit me yesterday when I took a step back: all we need to do is to ensure that the end-of-line style is matched when *all* input files are LF-only, or when they are all CR/LF. In all other cases, we have mixed line endings anyway.

And to do that, it is sufficient to look at *one single line ending* in the context. Technically, it does not even have to be the context, but the first line endings of the first file would be enough, however it is so much more pleasant if the conflict marker's eol matches the one of the preceding line.

To prevent my future self from repeating mistakes, I added a little bit of background to the commit message.

Since this is a complete rewrite, the interdiff is pretty useless and only included for the record.

Johannes Schindelin (1):
  merge-file: let conflict markers match end-of-line style of the
    context
 t/t6023-merge-file.sh | 12 +++++++++++
 xdiff/xmerge.c        | 57 +++++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 65 insertions(+), 4 deletions(-)
Interdiff vs v1:
 diff --git a/builtin/merge-file.c b/builtin/merge-file.c
 index 9ce830a..5544705 100644
 --- a/builtin/merge-file.c
 +++ b/builtin/merge-file.c
 @@ -81,7 +81,6 @@ 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/convert.c b/convert.c
 index b458734..814e814 100644
 --- a/convert.c
 +++ b/convert.c
 @@ -758,35 +758,6 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)
  	}
  }
  
 -enum eol eol_for_path(const char *path, const char *src, size_t len)
 -{
 -	struct conv_attrs ca;
 -	struct text_stat stats;
 -
 -	if (!path) {
 -		memset(&ca, 0, sizeof(ca));
 -		ca.crlf_action = CRLF_AUTO;
 -		ca.eol_attr = EOL_UNSET;
 -	} else {
 -		convert_attrs(&ca, path);
 -		if (ca.eol_attr == EOL_UNSET)
 -			ca.eol_attr = output_eol(ca.crlf_action);
 -		if (ca.eol_attr != EOL_UNSET)
 -			return ca.eol_attr;
 -	}
 -	if (!len || (ca.crlf_action != CRLF_AUTO &&
 -				ca.crlf_action != CRLF_GUESS))
 -		return core_eol;
 -	ca.crlf_action = input_crlf_action(ca.crlf_action, ca.eol_attr);
 -	gather_stats(src, len, &stats);
 -	if (ca.crlf_action == CRLF_GUESS && stats.cr > stats.crlf)
 -		return core_eol;
 -	else if (stats.crlf)
 -		return EOL_CRLF;
 -	else
 -		return EOL_LF;
 -}
 -
  int would_convert_to_git_filter_fd(const char *path)
  {
  	struct conv_attrs ca;
 diff --git a/convert.h b/convert.h
 index 1892867..d9d853c 100644
 --- a/convert.h
 +++ b/convert.h
 @@ -33,8 +33,6 @@ enum eol {
  
  extern enum eol core_eol;
  
 -extern enum eol eol_for_path(const char *path, const char *src, size_t len);
 -
  /* returns 1 if *dst was used */
  extern int convert_to_git(const char *path, const char *src, size_t len,
  			  struct strbuf *dst, enum safe_crlf checksafe);
 diff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh
 index a131749..f0cb9ce 100755
 --- a/t/t6023-merge-file.sh
 +++ b/t/t6023-merge-file.sh
 @@ -346,18 +346,16 @@ 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_expect_success 'conflict markers match existing line endings' '
 +	append_cr <nolf-orig.txt >crlf-orig.txt &&
 +	append_cr <nolf-diff1.txt >crlf-diff1.txt &&
 +	append_cr <nolf-diff2.txt >crlf-diff2.txt &&
  	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 &&
 +		crlf-diff1.txt crlf-orig.txt crlf-diff2.txt >crlf.txt &&
 +	test $(tr "\015" Q <crlf.txt | grep "\\.txtQ$" | wc -l) = 3 &&
  	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
 +		nolf-diff1.txt nolf-orig.txt nolf-diff2.txt >nolf.txt &&
 +	test $(tr "\015" Q <nolf.txt | grep "\\.txtQ$" | wc -l) = 0
  '
  
  test_done
 diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
 index a5bea4a..c033991 100644
 --- a/xdiff/xdiff.h
 +++ b/xdiff/xdiff.h
 @@ -122,7 +122,6 @@ 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 4ff0db4..c852acc 100644
 --- a/xdiff/xmerge.c
 +++ b/xdiff/xmerge.c
 @@ -143,15 +143,56 @@ static int xdl_orig_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest)
  	return xdl_recs_copy_0(1, xe, i, count, add_nl, dest);
  }
  
 +/*
 + * Returns 1 if the i'th line ends in CR/LF (if it is the last line and
 + * has no eol, the preceding line, if any), 0 if it ends in LF-only, and
 + * -1 if the line ending cannot be determined.
 + */
 +static int is_eol_crlf(xdfile_t *file, int i)
 +{
 +	long size;
 +
 +	if (i < file->nrec - 1)
 +		/* All lines before the last *must* end in LF */
 +		return (size = file->recs[i]->size) > 1 &&
 +			file->recs[i]->ptr[size - 2] == '\r';
 +	if (!file->nrec)
 +		/* Cannot determine eol style from empty file */
 +		return -1;
 +	if ((size = file->recs[i]->size) &&
 +			file->recs[i]->ptr[size - 1] == '\n')
 +		/* Last line; ends in LF; Is it CR/LF? */
 +		return size > 1 &&
 +			file->recs[i]->ptr[size - 2] == '\r';
 +	if (!i)
 +		/* The only line has no eol */
 +		return -1;
 +	/* Determine eol from second-to-last line */
 +	return (size = file->recs[i - 1]->size) > 1 &&
 +		file->recs[i - 1]->ptr[size - 2] == '\r';
 +}
 +
  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 crlf,
 +			      int size, int i, int style,
  			      xdmerge_t *m, char *dest, int marker_size)
  {
  	int marker1_size = (name1 ? strlen(name1) + 1 : 0);
  	int marker2_size = (name2 ? strlen(name2) + 1 : 0);
  	int marker3_size = (name3 ? strlen(name3) + 1 : 0);
 +	int needs_cr;
 +
 +	/* Match post-images' preceding, or first, lines' end-of-line style */
 +	needs_cr = is_eol_crlf(&xe1->xdf2, m->i1 ? m->i1 - 1 : 0);
 +	if (needs_cr)
 +		needs_cr = is_eol_crlf(&xe2->xdf2, m->i2 ? m->i2 - 1 : 0);
 +	/* Look at pre-image's first line, unless we already settled on LF */
 +	if (needs_cr)
 +		needs_cr = is_eol_crlf(&xe1->xdf1, 0);
 +	/* If still undecided, use LF-only */
 +	if (needs_cr < 0)
 +		needs_cr = 0;
  
  	if (marker_size <= 0)
  		marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
 @@ -161,7 +202,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
  			      dest ? dest + size : NULL);
  
  	if (!dest) {
 -		size += marker_size + 1 + crlf + marker1_size;
 +		size += marker_size + 1 + needs_cr + marker1_size;
  	} else {
  		memset(dest + size, '<', marker_size);
  		size += marker_size;
 @@ -170,7 +211,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
  			memcpy(dest + size + 1, name1, marker1_size - 1);
  			size += marker1_size;
  		}
 -		if (crlf)
 +		if (needs_cr)
  			dest[size++] = '\r';
  		dest[size++] = '\n';
  	}
 @@ -182,7 +223,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 + crlf + marker3_size;
 +			size += marker_size + 1 + needs_cr + marker3_size;
  		} else {
  			memset(dest + size, '|', marker_size);
  			size += marker_size;
 @@ -191,7 +232,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
  				memcpy(dest + size + 1, name3, marker3_size - 1);
  				size += marker3_size;
  			}
 -			if (crlf)
 +			if (needs_cr)
  				dest[size++] = '\r';
  			dest[size++] = '\n';
  		}
 @@ -200,11 +241,11 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
  	}
  
  	if (!dest) {
 -		size += marker_size + 1 + crlf;
 +		size += marker_size + 1 + needs_cr;
  	} else {
  		memset(dest + size, '=', marker_size);
  		size += marker_size;
 -		if (crlf)
 +		if (needs_cr)
  			dest[size++] = '\r';
  		dest[size++] = '\n';
  	}
 @@ -213,7 +254,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 + crlf + marker2_size;
 +		size += marker_size + 1 + needs_cr + marker2_size;
  	} else {
  		memset(dest + size, '>', marker_size);
  		size += marker_size;
 @@ -222,7 +263,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
  			memcpy(dest + size + 1, name2, marker2_size - 1);
  			size += marker2_size;
  		}
 -		if (crlf)
 +		if (needs_cr)
  			dest[size++] = '\r';
  		dest[size++] = '\n';
  	}
 @@ -234,7 +275,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 crlf, int marker_size)
 +				 int marker_size)
  {
  	int size, i;
  
 @@ -245,8 +286,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, crlf, m,
 -						  dest, marker_size);
 +						  size, i, style, m, dest,
 +						  marker_size);
  		else if (m->mode & 3) {
  			/* Before conflicting part */
  			size += xdl_recs_copy(xe1, i, m->i1 - i, 0,
 @@ -427,7 +468,6 @@ 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) {
  		/*
 @@ -563,7 +603,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,
 -						 crlf, marker_size);
 +						 marker_size);
  		result->ptr = xdl_malloc(size);
  		if (!result->ptr) {
  			xdl_cleanup_merge(changes);
 @@ -572,7 +612,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, crlf, marker_size);
 +				      result->ptr, style, marker_size);
  	}
  	return xdl_cleanup_merge(changes);
  }
-- 
2.7.0.windows.1.7.g55a05c8
Previous: Beat BolliNext: Johannes Schindelin
Message 22 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.