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

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

[Johannes Schindelin]

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.

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.

The most crucial fix is that ll-merge.c is now *untouched*,
as it works on blob contents (i.e. LF-only line endings),
*not* on file contents from the working directory.


Beat Bolli (1):
  merge-file: consider core.crlf when writing merge markers

Johannes Schindelin (1):
  convert: add a helper to determine the correct EOL for a given path

 builtin/merge-file.c  |  1 +
 convert.c             | 29 +++++++++++++++++++++++++++++
 convert.h             |  2 ++
 t/t6023-merge-file.sh | 14 ++++++++++++++
 xdiff/xdiff.h         |  1 +
 xdiff/xmerge.c        | 29 +++++++++++++++++++----------
 6 files changed, 66 insertions(+), 10 deletions(-)

-- 
2.7.0.windows.1.7.g55a05c8

[PATCH 1/2] convert: add a helper to determine the correct EOL for a given path

[Johannes Schindelin]

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 convert.c | 29 +++++++++++++++++++++++++++++
 convert.h |  2 ++
 2 files changed, 31 insertions(+)

diff --git a/convert.c b/convert.c
index 814e814..b458734 100644
--- a/convert.c
+++ b/convert.c
@@ -758,6 +758,35 @@ 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 d9d853c..1892867 100644
--- a/convert.h
+++ b/convert.h
@@ -33,6 +33,8 @@ 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);
-- 
2.7.0.windows.1.7.g55a05c8

Re: [PATCH 1/2] convert: add a helper to determine the correct EOL for a given path

[Junio C Hamano]

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

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---

This change somehow ringed a bell and reminded me of your recent
ls-files stuff.  Are there things that these topics can use from
each other?

These topics are similar in that they add many lines of code as
"helpers" that inspect data and guess what the existing code would
do to the data, without refactoring much existing code whose
behaviour these helpers are guessing/defining to use them, which is
somewhat disturbing, as the behaviour the actual code exhibits and
the guesses the helpers make can easily drift apart.



>  convert.c | 29 +++++++++++++++++++++++++++++
>  convert.h |  2 ++
>  2 files changed, 31 insertions(+)
>
> diff --git a/convert.c b/convert.c
> index 814e814..b458734 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -758,6 +758,35 @@ 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 d9d853c..1892867 100644
> --- a/convert.h
> +++ b/convert.h
> @@ -33,6 +33,8 @@ 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);

Re: [PATCH 1/2] convert: add a helper to determine the correct EOL for a given path

[Johannes Schindelin]

Hi Junio,

On Fri, 22 Jan 2016, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> 
> This change somehow ringed a bell and reminded me of your recent
> ls-files stuff.  Are there things that these topics can use from
> each other?

Quite possibly. I'll have a look tomorrow.

Ciao,
Dscho

Re: [PATCH 1/2] convert: add a helper to determine the correct EOL for a given path

[Torsten Bögershausen]

On 2016-01-22 20.04, Johannes Schindelin wrote:
> Hi Junio,
> 
> On Fri, 22 Jan 2016, Junio C Hamano wrote:
> 
>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>>
>>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>>> ---
>>
>> This change somehow ringed a bell and reminded me of your recent
>> ls-files stuff.  Are there things that these topics can use from
>> each other?
> 
> Quite possibly. I'll have a look tomorrow.

Something like this should do:
(Fully untested)

enum eol eol_for_path(const char *path, const char *src, size_t len)
{
  struct conv_attrs ca;
  if (!path)
     path = "*"; /* catch * text=xxx */
  convert_attrs(&ca, path);
  ca.crlf_action = input_crlf_action(ca.crlf_action, ca.eol_attr);

  return output_eol(crlf_action);
}

Re: [PATCH 1/2] convert: add a helper to determine the correct EOL for a given path

[Johannes Schindelin]

Hi Junio,

On Fri, 22 Jan 2016, Johannes Schindelin wrote:

> On Fri, 22 Jan 2016, Junio C Hamano wrote:
> 
> > Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> > 
> > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > > ---
> > 
> > This change somehow ringed a bell and reminded me of your recent
> > ls-files stuff.  Are there things that these topics can use from
> > each other?
> 
> Quite possibly. I'll have a look tomorrow.

I did, but could not quite finish reworking the patch until today. Will
send out the result in a moment.

Ciao,
Dscho

Re: [PATCH 1/2] convert: add a helper to determine the correct EOL for a given path

[Torsten Bögershausen]

On 2016-01-22 18.01, Johannes Schindelin wrote:
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  convert.c | 29 +++++++++++++++++++++++++++++
>  convert.h |  2 ++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/convert.c b/convert.c
> index 814e814..b458734 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -758,6 +758,35 @@ 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) {
I don't think this is ideal.
When path is NULL, can we set it to "*" to catch .gitattributes like
"* text=auto"
?
> +		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;
This doesn't seem to be correct.
When "File -text" is set, no CRLF needs to be added.
> +	}
> +	if (!len || (ca.crlf_action != CRLF_AUTO &&
> +				ca.crlf_action != CRLF_GUESS))
Currently core.autocrlf = true overrules "core.eol".

See the line in t0027:
#              core.eol  core.autocrlf
checkout_files    lf      true   ""       CRLF  CRLF  CRLF_mix_LF  LF_mix_CR
LF_nul

Letting core.autocrlf overrule core.eol is not documented very well,
I have it on my todo-list. But I think it makes sense to keep the behavour.

I'm currently working on simplification of convert.c, so a funtion like this
is really a step forward.
Does it makes sense to add a comment, that some corner cases may
need additional tweaking ?

Re: [PATCH 1/2] convert: add a helper to determine the correct EOL for a given path

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 416 bytes --]

Hi Torsten,

On Sat, 23 Jan 2016, Torsten Bögershausen wrote:

> When path is NULL, can we set it to "*" to catch .gitattributes like
> "* text=auto"
> ?

The point of that part of the patch was to make things *independent* of
the gitattributes because we operate not on working files, but on blob
contents.

In any case, this point will be moot with the upcoming iteration of the
patch.

Ciao,
Dscho

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

[Johannes Schindelin]

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;
 	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);
 }
-- 
2.7.0.windows.1.7.g55a05c8

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

[Junio C Hamano]

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

> 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?

> 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);
>  }

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

[Johannes Schindelin]

Hi Junio,

On Fri, 22 Jan 2016, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > 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?

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;

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

[Junio C Hamano]

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

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

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

[Johannes Schindelin]

Hi Junio,

On Fri, 22 Jan 2016, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> > 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.

Well, I just heeded your point about blobs not necessarily having LF-only
end-of-lines ;-)

In any case, the upcoming iteration works quite a bit differently, and I
want to add, robustly.

Ciao,
Dscho

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

[Eric Sunshine]

On Fri, Jan 22, 2016 at 06:01:25PM +0100, Johannes Schindelin wrote:
> 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".
> [...]
> 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>
> ---
> diff --git 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' \
> +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

The "\r" isn't recognized by 'sed' on Mac OS X or BSD. Perhaps use
instead:

    test $(cat output.txt | tr "\015" Q | sed -n "/\.txtQ$/p" | wc -l) = 3

which works on Linux, Mac OS X, and BSD (in my tests).

Or, implement a cr_to_q() to complement the existing q_to_cr() in
t/test-lib-functions.sh.

> +'
> +
> +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

Ditto.

> +'

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

[Eric Sunshine]

On Fri, Jan 22, 2016 at 2:50 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Fri, Jan 22, 2016 at 06:01:25PM +0100, Johannes Schindelin wrote:
>> +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
>
> The "\r" isn't recognized by 'sed' on Mac OS X or BSD. Perhaps use
> instead:
>
>     test $(cat output.txt | tr "\015" Q | sed -n "/\.txtQ$/p" | wc -l) = 3

Or the 'sed' could even become at 'grep' at this point.

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

[Johannes Schindelin]

Hi Eric,

On Fri, 22 Jan 2016, Eric Sunshine wrote:

> On Fri, Jan 22, 2016 at 2:50 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> > On Fri, Jan 22, 2016 at 06:01:25PM +0100, Johannes Schindelin wrote:
> >> +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
> >
> > The "\r" isn't recognized by 'sed' on Mac OS X or BSD. Perhaps use
> > instead:
> >
> >     test $(cat output.txt | tr "\015" Q | sed -n "/\.txtQ$/p" | wc -l) = 3
> 
> Or the 'sed' could even become at 'grep' at this point.

Good point, thanks!

Ciao,
Dscho

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

[Eric Sunshine]

On Sun, Jan 24, 2016 at 5:37 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Fri, 22 Jan 2016, Eric Sunshine wrote:
>> On Fri, Jan 22, 2016 at 2:50 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> > On Fri, Jan 22, 2016 at 06:01:25PM +0100, Johannes Schindelin wrote:
>> >> +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
>> >
>> > The "\r" isn't recognized by 'sed' on Mac OS X or BSD. Perhaps use
>> > instead:
>> >
>> >     test $(cat output.txt | tr "\015" Q | sed -n "/\.txtQ$/p" | wc -l) = 3
>>
>> Or the 'sed' could even become at 'grep' at this point.
>
> Good point, thanks!

And, without the unnecessary 'cat', of course (don't know what I was thinking):

    test $(tr "\015" Q <output.txt | grep "\.txtQ$" | wc -l) = 3

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

[Johannes Schindelin]

Hi Eric,

On Sun, 24 Jan 2016, Eric Sunshine wrote:

> On Sun, Jan 24, 2016 at 5:37 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > On Fri, 22 Jan 2016, Eric Sunshine wrote:
> >> On Fri, Jan 22, 2016 at 2:50 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> >> > On Fri, Jan 22, 2016 at 06:01:25PM +0100, Johannes Schindelin wrote:
> >> >> +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
> >> >
> >> > The "\r" isn't recognized by 'sed' on Mac OS X or BSD. Perhaps use
> >> > instead:
> >> >
> >> >     test $(cat output.txt | tr "\015" Q | sed -n "/\.txtQ$/p" | wc -l) = 3
> >>
> >> Or the 'sed' could even become at 'grep' at this point.
> >
> > Good point, thanks!
> 
> And, without the unnecessary 'cat', of course (don't know what I was thinking):
> 
>     test $(tr "\015" Q <output.txt | grep "\.txtQ$" | wc -l) = 3

Heh, I automatically removed the "cat" in my mind already, and as you can
see from my v2, I used precisely the cat-less form you suggested.

Ciao,
Dscho

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

[Torsten Bögershausen]

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"

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 744 bytes --]

Hi Torsten,

On Sat, 23 Jan 2016, Torsten Bögershausen wrote:

> 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".
> > 
> > [...]
>
> What happens when the original file already has CRLF ?
> And why don't we feed any content into the function ?

Why, indeed. I completely reworked the entire approach and will send out
the result in a moment.

Ciao,
Dscho

Re: [PATCH 0/2] Let merge-file write out conflict markers with correct EOLs

[Beat Bolli]

On 22.01.16 18:01, Johannes Schindelin wrote:
> 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.
> 
> 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.

Thanks a lot! I was really stuck on the heuristics of eol_for_path(), so
I'm very glad you took over.

Regards,
Beat

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

[Johannes Schindelin]

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

[PATCH v2 1/1] merge-file: let conflict markers match end-of-line style of the context

[Johannes Schindelin]

When merging files with CR/LF line endings, the conflict markers should
match those, lest the output file has mixed line endings.

This is particularly of interest on Windows, where some editors get
*really* confused by mixed line endings.

The original version of this patch by Beat Bolli respected core.eol, and
a subsequent improvement by this developer also respected gitattributes.
This approach was suboptimal, though: `git merge-file` was invented as a
drop-in replacement for GNU merge and as such has no problem operating
outside of any repository at all!

Another problem with the original approach was pointed out by Junio
Hamano: legacy repositories might have their text files committed using
CR/LF line endings (and core.eol and the gitattributes would give us a
false impression there). Therefore, the much superior approach is to
simply match the context's line endings, if any.

We actually do not have to look at the *entire* context at all: if the
files are all LF-only, or if they all have CR/LF line endings, it is
sufficient to look at just a *single* line to match that style. And if
the line endings are mixed anyway, it is *still* okay to imitate just a
single line's eol: we will just add to the pile of mixed line endings,
and there is nothing we can do about that.

So what we do is: we look at the line preceding the conflict, falling
back to the line preceding that in case it was the last line and had no
line ending, falling back to the first line, first in the first
post-image, then the second post-image, and finally the pre-image.
If we find consistent CR/LF (or undecided) end-of-line style, we match
that, otherwise we use LF-only line endings for the conflict markers.

Note that while it is true that there have to be at least two lines we
can look at (otherwise there would be no conflict), the same is not true
for line *endings*: the three files in question could all consist of a
single line without any line ending, each. In this case we fall back to
using LF-only.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t6023-merge-file.sh | 12 +++++++++++
 xdiff/xmerge.c        | 57 +++++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 65 insertions(+), 4 deletions(-)

diff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh
index 190ee90..f0cb9ce 100755
--- a/t/t6023-merge-file.sh
+++ b/t/t6023-merge-file.sh
@@ -346,4 +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 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 \
+		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 >nolf.txt &&
+	test $(tr "\015" Q <nolf.txt | grep "\\.txtQ$" | wc -l) = 0
+'
+
 test_done
diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c
index 625198e..c852acc 100644
--- a/xdiff/xmerge.c
+++ b/xdiff/xmerge.c
@@ -143,6 +143,35 @@ 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,
@@ -152,6 +181,18 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 	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 + marker1_size;
+		size += marker_size + 1 + needs_cr + marker1_size;
 	} else {
 		memset(dest + size, '<', marker_size);
 		size += marker_size;
@@ -170,6 +211,8 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 			memcpy(dest + size + 1, name1, marker1_size - 1);
 			size += marker1_size;
 		}
+		if (needs_cr)
+			dest[size++] = '\r';
 		dest[size++] = '\n';
 	}
 
@@ -180,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 + marker3_size;
+			size += marker_size + 1 + needs_cr + marker3_size;
 		} else {
 			memset(dest + size, '|', marker_size);
 			size += marker_size;
@@ -189,6 +232,8 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 				memcpy(dest + size + 1, name3, marker3_size - 1);
 				size += marker3_size;
 			}
+			if (needs_cr)
+				dest[size++] = '\r';
 			dest[size++] = '\n';
 		}
 		size += xdl_orig_copy(xe1, m->i0, m->chg0, 1,
@@ -196,10 +241,12 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 	}
 
 	if (!dest) {
-		size += marker_size + 1;
+		size += marker_size + 1 + needs_cr;
 	} else {
 		memset(dest + size, '=', marker_size);
 		size += marker_size;
+		if (needs_cr)
+			dest[size++] = '\r';
 		dest[size++] = '\n';
 	}
 
@@ -207,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 + marker2_size;
+		size += marker_size + 1 + needs_cr + marker2_size;
 	} else {
 		memset(dest + size, '>', marker_size);
 		size += marker_size;
@@ -216,6 +263,8 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 			memcpy(dest + size + 1, name2, marker2_size - 1);
 			size += marker2_size;
 		}
+		if (needs_cr)
+			dest[size++] = '\r';
 		dest[size++] = '\n';
 	}
 	return size;
-- 
2.7.0.windows.1.7.g55a05c8

Re: [PATCH v2 1/1] merge-file: let conflict markers match end-of-line style of the context

[Torsten Bögershausen]

On 24.01.16 11:48, Johannes Schindelin wrote:
(I had the same reasoning about the CRLF in the working tree:
We don't need to look at core.autocrlf/attributes, so Ack from me)

> +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 \
> +		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 >nolf.txt &&
> +	test $(tr "\015" Q <nolf.txt | grep "\\.txtQ$" | wc -l) = 0
> +'
> +

Minor remark:

Ramsay suggested a test that doesn't use grep or wc and looks like this:

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 &&                                                                                                                                 
  tr "\015" Q <output.txt | sed -n "/^[<=>|].*Q$/p" >out.txt &&                                                                                                                                
  cat >expect.txt <<-\EOF &&                                                                                                                                                                   
  <<<<<<< nolf-diff1.txtQ                                                                                                                                                                      
  ||||||| nolf-orig.txtQ                                                                                                                                                                       
  =======Q                                                                                                                                                                                     
  >>>>>>> nolf-diff2.txtQ                                                                                                                                                                      
  EOF                                                                                                                                                                                          
  test_cmp expect.txt out.txt                                                                                                                                                                  
'

Re: [PATCH v2 1/1] merge-file: let conflict markers match end-of-line style of the context

[Torsten Bögershausen]

On 24.01.16 17:27, Torsten Bögershausen wrote:
> On 24.01.16 11:48, Johannes Schindelin wrote:
> (I had the same reasoning about the CRLF in the working tree:
> We don't need to look at core.autocrlf/attributes, so Ack from me)
> 
>> +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 \
>> +		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 >nolf.txt &&
>> +	test $(tr "\015" Q <nolf.txt | grep "\\.txtQ$" | wc -l) = 0
>> +'
>> +
> 
> Minor remark:
> 
> Ramsay suggested a test that doesn't use grep or wc and looks like this:
(That looked really garbled in my mail pgm. The suggestion is here:) 
<http://article.gmane.org/gmane.comp.version-control.git/283261>

Re: [PATCH v2 1/1] merge-file: let conflict markers match end-of-line style of the context

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1843 bytes --]

Hi Torsten,

On Sun, 24 Jan 2016, Torsten Bögershausen wrote:

> On 24.01.16 11:48, Johannes Schindelin wrote:
> (I had the same reasoning about the CRLF in the working tree:
> We don't need to look at core.autocrlf/attributes, so Ack from me)
> 
> > +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 \
> > +		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 >nolf.txt &&
> > +	test $(tr "\015" Q <nolf.txt | grep "\\.txtQ$" | wc -l) = 0
> > +'
> > +
> 
> Minor remark:
> 
> Ramsay suggested a test that doesn't use grep or wc and looks like this:
> 
> 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 &&
>   tr "\015" Q <output.txt | sed -n "/^[<=>|].*Q$/p" >out.txt &&
>   cat >expect.txt <<-\EOF &&
>   <<<<<<< nolf-diff1.txtQ
>   ||||||| nolf-orig.txtQ
>   =======Q
>   >>>>>>> nolf-diff2.txtQ
>   EOF
>   test_cmp expect.txt out.txt
> '

Probably he wrapped it at less than 192 columns per row, though ;-)

Seriously again, this longer version might test more, but it definitely
also tests more than what I actually want to test: I am simply interested
to verify that the conflict markers end in CR/LF when appropriate. Read: I
am uncertain that I want to spend the additional lines on testing more
than actually necessary.

Ciao,
Dscho

Re: [PATCH v2 1/1] merge-file: let conflict markers match end-of-line style of the context

[Ramsay Jones]

On 25/01/16 06:53, Johannes Schindelin wrote:
> Hi Torsten,
> 
> On Sun, 24 Jan 2016, Torsten Bögershausen wrote:
> 
>> On 24.01.16 11:48, Johannes Schindelin wrote:
>> (I had the same reasoning about the CRLF in the working tree:
>> We don't need to look at core.autocrlf/attributes, so Ack from me)
>>
>>> +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 \
>>> +		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 >nolf.txt &&
>>> +	test $(tr "\015" Q <nolf.txt | grep "\\.txtQ$" | wc -l) = 0
>>> +'
>>> +
>>
>> Minor remark:
>>
>> Ramsay suggested a test that doesn't use grep or wc and looks like this:
>>
>> 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 &&
>>   tr "\015" Q <output.txt | sed -n "/^[<=>|].*Q$/p" >out.txt &&
>>   cat >expect.txt <<-\EOF &&
>>   <<<<<<< nolf-diff1.txtQ
>>   ||||||| nolf-orig.txtQ
>>   =======Q
>>   >>>>>>> nolf-diff2.txtQ
>>   EOF
>>   test_cmp expect.txt out.txt
>> '
> 
> Probably he wrapped it at less than 192 columns per row, though ;-)
> 
;-)
> Seriously again, this longer version might test more, but it definitely
> also tests more than what I actually want to test: I am simply interested
> to verify that the conflict markers end in CR/LF when appropriate.

But you are only testing 3/4 conflict markers end in CR/LF. :-D

>                                                                     Read: I
> am uncertain that I want to spend the additional lines on testing more
> than actually necessary.

If the here doc is too verbose for you, how about something like this
(totally untested):

    test $(tr "\015" Q <crlf.txt | grep "^[<=>|].*Q$" | wc -l) -eq 4

instead?

HTH

ATB,
Ramsay Jones

Re: [PATCH v2 1/1] merge-file: let conflict markers match end-of-line style of the context

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2811 bytes --]

Hi Ramsay,

On Mon, 25 Jan 2016, Ramsay Jones wrote:

> On 25/01/16 06:53, Johannes Schindelin wrote:
> > 
> > On Sun, 24 Jan 2016, Torsten Bögershausen wrote:
> > 
> >> On 24.01.16 11:48, Johannes Schindelin wrote:
> >> (I had the same reasoning about the CRLF in the working tree:
> >> We don't need to look at core.autocrlf/attributes, so Ack from me)
> >>
> >>> +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 \
> >>> +		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 >nolf.txt &&
> >>> +	test $(tr "\015" Q <nolf.txt | grep "\\.txtQ$" | wc -l) = 0
> >>> +'
> >>> +
> >>
> >> Minor remark:
> >>
> >> Ramsay suggested a test that doesn't use grep or wc and looks like this:
> >>
> >> 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 &&
> >>   tr "\015" Q <output.txt | sed -n "/^[<=>|].*Q$/p" >out.txt &&
> >>   cat >expect.txt <<-\EOF &&
> >>   <<<<<<< nolf-diff1.txtQ
> >>   ||||||| nolf-orig.txtQ
> >>   =======Q
> >>   >>>>>>> nolf-diff2.txtQ
> >>   EOF
> >>   test_cmp expect.txt out.txt
> >> '
> > 
> > Probably he wrapped it at less than 192 columns per row, though ;-)
> > 
> ;-)
> > Seriously again, this longer version might test more, but it definitely
> > also tests more than what I actually want to test: I am simply interested
> > to verify that the conflict markers end in CR/LF when appropriate.
> 
> But you are only testing 3/4 conflict markers end in CR/LF. :-D

The fact that ||| markers are present is the fault of previous test cases.
I tried to make a point of *not* relying on such a side effect (so as to
debug failures quicker by commenting out all previous test cases).

So the fact that I am testing only 3 of the 4 conflict markers is very
much by design.

> > Read: I am uncertain that I want to spend the additional lines on
> > testing more than actually necessary.
> 
> If the here doc is too verbose for you, how about something like this
> (totally untested):
> 
>     test $(tr "\015" Q <crlf.txt | grep "^[<=>|].*Q$" | wc -l) -eq 4
> 
> instead?

Hmm. I do not see the benefit over grepping for `txtQ$` it's essentially
the same.

> HTH

How the hell?

> ATB,
> Ramsay Jones

Authorization to Buy?

Ciao,
Dscho

Re: [PATCH v2 1/1] merge-file: let conflict markers match end-of-line style of the context

[Ramsay Jones]

On 26/01/16 08:54, Johannes Schindelin wrote:
> Hi Ramsay,
[snip]

>>
>> But you are only testing 3/4 conflict markers end in CR/LF. :-D
> 
> The fact that ||| markers are present is the fault of previous test cases.
> I tried to make a point of *not* relying on such a side effect (so as to
> debug failures quicker by commenting out all previous test cases).
> 
> So the fact that I am testing only 3 of the 4 conflict markers is very
> much by design.

I'm sorry, but I don't understand what you are trying to say here. :(

> 
>>> Read: I am uncertain that I want to spend the additional lines on
>>> testing more than actually necessary.
>>
>> If the here doc is too verbose for you, how about something like this
>> (totally untested):
>>
>>     test $(tr "\015" Q <crlf.txt | grep "^[<=>|].*Q$" | wc -l) -eq 4
>>
>> instead?
> 
> Hmm. I do not see the benefit over grepping for `txtQ$` it's essentially
> the same.

Well, as I said 'totally untested', but it _should_ be different. ;-)

The output from the 'tr "\015" Q <crlf.txt' subcommand should look
something like:

    1Q
    2Q
    <<<<<<< crlf-diff1.txtQ
    4Q
    ||||||| crlf-orig.txtQ
    3Q
    =======Q
    5Q
    >>>>>>> crlf-diff2.txtQ

so that your 'grep "\\.txtQ$"' should select these lines:

    <<<<<<< crlf-diff1.txtQ
    ||||||| crlf-orig.txtQ
    >>>>>>> crlf-diff2.txtQ

whereas my 'grep "^[<=>|].*Q$"' should select these lines:

    <<<<<<< crlf-diff1.txtQ
    ||||||| crlf-orig.txtQ
    =======Q
    >>>>>>> crlf-diff2.txtQ

(ie. select all 4 conflict markers).

> 
>> HTH
> 
> How the hell?

:-D Hope That Helps.

> 
>> ATB,
>> Ramsay Jones
> 
> Authorization to Buy?

All The Best.

ATB,
Ramsay Jones

Re: [PATCH v2 1/1] merge-file: let conflict markers match end-of-line style of the context

[Johannes Schindelin]

Hi Ramsay,

On Tue, 26 Jan 2016, Ramsay Jones wrote:

> On 26/01/16 08:54, Johannes Schindelin wrote:
> > Hi Ramsay,
> [snip]
> 
> >>
> >> But you are only testing 3/4 conflict markers end in CR/LF. :-D
> > 
> > The fact that ||| markers are present is the fault of previous test cases.
> > I tried to make a point of *not* relying on such a side effect (so as to
> > debug failures quicker by commenting out all previous test cases).
> > 
> > So the fact that I am testing only 3 of the 4 conflict markers is very
> > much by design.
> 
> I'm sorry, but I don't understand what you are trying to say here. :(

Typically, there are only 3 conflict markers:

<<<
diff 1
===
diff 2
>>>

However, you can tell merge-file to show also the original hunk, in this
case it was done by calling `git config merge.conflictstyle diff3` in an
earlier test, and this implementation detail seeps through to the current
test case.

Guess what, if you really only quickly want to run this last test (and
comment-out the previous ones), the conflict style is *not* diff3, and you
end up with the three conflict markers again.

> >>> Read: I am uncertain that I want to spend the additional lines on
> >>> testing more than actually necessary.
> >>
> >> If the here doc is too verbose for you, how about something like this
> >> (totally untested):
> >>
> >>     test $(tr "\015" Q <crlf.txt | grep "^[<=>|].*Q$" | wc -l) -eq 4
> >>
> >> instead?
> > 
> > Hmm. I do not see the benefit over grepping for `txtQ$` it's essentially
> > the same.
> 
> Well, as I said 'totally untested', but it _should_ be different. ;-)
> 
> The output from the 'tr "\015" Q <crlf.txt' subcommand should look
> something like:
> 
>     1Q
>     2Q
>     <<<<<<< crlf-diff1.txtQ
>     4Q
>     ||||||| crlf-orig.txtQ
>     3Q
>     =======Q
>     5Q
>     >>>>>>> crlf-diff2.txtQ
> 
> so that your 'grep "\\.txtQ$"' should select these lines:
> 
>     <<<<<<< crlf-diff1.txtQ
>     ||||||| crlf-orig.txtQ
>     >>>>>>> crlf-diff2.txtQ
> 
> whereas my 'grep "^[<=>|].*Q$"' should select these lines:
> 
>     <<<<<<< crlf-diff1.txtQ
>     ||||||| crlf-orig.txtQ
>     =======Q
>     >>>>>>> crlf-diff2.txtQ
> 
> (ie. select all 4 conflict markers).

True, I overlooked that the middle txtQ comes from the ||| line that I
actually did *not* want to test for (see above).

Will fix.

Ciao,
Dscho

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

[Junio C Hamano]

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

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

I like that approach.  My understanding of the xdiff/xmerge code is
very rusty and I have some time to re-learn it before I can judge
that the change implements that approach correctly, though X-<.

Thanks.

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

[Johannes Schindelin]

Hi Junio,

On Sun, 24 Jan 2016, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > 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.
> 
> I like that approach.  My understanding of the xdiff/xmerge code is
> very rusty and I have some time to re-learn it before I can judge
> that the change implements that approach correctly, though X-<.

I hear ya. The xmerge code is my fault, too, and it did not help that I
imitated the lack of documentation of the surrounding code.

But it all came back to me when I looked at the code. Hopefully these
hints are helpful:

- xe1 and xe2 refer to "xdfenv_t" types that essentially hold pointers to
  one pre-image and one post-image, each

- The pre-images of xe1 and xe2 are identical, of course, because we are
  looking at a three-way merge

- The xdfenv_t type refers to the pre-image as xdf1 and to the post-image
  as xdf2, both of the type "xdfile_t"

- The xdfile_t structure contains the pointers to a parsed file. The lines
  are called "recs" (record) here

- Each record has a pointed to the start of the line and a size that
  includes the newline byte, if any

- The record parsing hard-codes 0x0a as newline (NEWLINEBYTES), which is
  used in the xdl_hash_record() function that is called from the
  xdl_prepare_ctx() function that parses each input file, and which in
  turn is called from the xdl_prepare_env() function that prepares a
  pair of files for diff'ing by the xdl_do_diff() function that in turn
  is called twice from xdl_merge() to diff orig->a and orig->b

- Imitating libdiff's existing code, "i" is used to refer to a specific
  line; it is the line number decremented by one, i.e. 0 <= i < nrec
  (where "nrec" is the field of the xdfile_t structure indicating the
  line count)

You know, cobbling these notes together had an unexpected side effect: I
realized that another patch is needed to properly support CR/LF files:
when the conflict hunk is at the end of one file and said file has no
trailing line ending, we add \n always, but we should add \r before that
when appropriate.

I'm on it.

Ciao,
Dscho

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

[Johannes Schindelin]

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 first commit message.

Triggered by a comment by Junio, I realized that a second patch is
needed: we need to make sure that the conflicting sections are also
augmented by the appropriate line endings if they lack any.

Relative to v2, the first patch changed only in the test code, to
accomodate for the newly-introduced second patch.


Johannes Schindelin (2):
  merge-file: let conflict markers match end-of-line style of the
    context
  merge-file: ensure that conflict sections match eol style

 t/t6023-merge-file.sh | 13 +++++++
 xdiff/xmerge.c        | 98 +++++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 93 insertions(+), 18 deletions(-)

Interdiff vs v2:

 diff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh
 index f0cb9ce..1390548 100755
 --- a/t/t6023-merge-file.sh
 +++ b/t/t6023-merge-file.sh
 @@ -346,13 +346,14 @@ 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 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_expect_success 'conflict sections match existing line endings' '
 +	printf "1\\r\\n2\\r\\n3" >crlf-orig.txt &&
 +	printf "1\\r\\n2\\r\\n4" >crlf-diff1.txt &&
 +	printf "1\\r\\n2\\r\\n5" >crlf-diff2.txt &&
  	test_must_fail git -c core.eol=crlf merge-file -p \
  		crlf-diff1.txt crlf-orig.txt crlf-diff2.txt >crlf.txt &&
  	test $(tr "\015" Q <crlf.txt | grep "\\.txtQ$" | wc -l) = 3 &&
 +	test $(tr "\015" Q <crlf.txt | grep "[345]Q$" | wc -l) = 3 &&
  	test_must_fail git -c core.eol=crlf merge-file -p \
  		nolf-diff1.txt nolf-orig.txt nolf-diff2.txt >nolf.txt &&
  	test $(tr "\015" Q <nolf.txt | grep "\\.txtQ$" | wc -l) = 0
 diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c
 index c852acc..d98f430 100644
 --- a/xdiff/xmerge.c
 +++ b/xdiff/xmerge.c
 @@ -109,7 +109,7 @@ static int xdl_merge_cmp_lines(xdfenv_t *xe1, int i1, xdfenv_t *xe2, int i2,
  	return 0;
  }
  
 -static int xdl_recs_copy_0(int use_orig, xdfenv_t *xe, int i, int count, int add_nl, char *dest)
 +static int xdl_recs_copy_0(int use_orig, xdfenv_t *xe, int i, int count, int needs_cr, int add_nl, char *dest)
  {
  	xrecord_t **recs;
  	int size = 0;
 @@ -125,6 +125,12 @@ static int xdl_recs_copy_0(int use_orig, xdfenv_t *xe, int i, int count, int add
  	if (add_nl) {
  		i = recs[count - 1]->size;
  		if (i == 0 || recs[count - 1]->ptr[i - 1] != '\n') {
 +			if (needs_cr) {
 +				if (dest)
 +					dest[size] = '\r';
 +				size++;
 +			}
 +
  			if (dest)
  				dest[size] = '\n';
  			size++;
 @@ -133,14 +139,14 @@ static int xdl_recs_copy_0(int use_orig, xdfenv_t *xe, int i, int count, int add
  	return size;
  }
  
 -static int xdl_recs_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest)
 +static int xdl_recs_copy(xdfenv_t *xe, int i, int count, int needs_cr, int add_nl, char *dest)
  {
 -	return xdl_recs_copy_0(0, xe, i, count, add_nl, dest);
 +	return xdl_recs_copy_0(0, xe, i, count, needs_cr, add_nl, dest);
  }
  
 -static int xdl_orig_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest)
 +static int xdl_orig_copy(xdfenv_t *xe, int i, int count, int needs_cr, int add_nl, char *dest)
  {
 -	return xdl_recs_copy_0(1, xe, i, count, add_nl, dest);
 +	return xdl_recs_copy_0(1, xe, i, count, needs_cr, add_nl, dest);
  }
  
  /*
 @@ -172,15 +178,8 @@ static int is_eol_crlf(xdfile_t *file, int i)
  		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,
 -			      xdmerge_t *m, char *dest, int marker_size)
 +static int is_cr_needed(xdfenv_t *xe1, xdfenv_t *xe2, xdmerge_t *m)
  {
 -	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 */
 @@ -191,14 +190,25 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
  	if (needs_cr)
  		needs_cr = is_eol_crlf(&xe1->xdf1, 0);
  	/* If still undecided, use LF-only */
 -	if (needs_cr < 0)
 -		needs_cr = 0;
 +	return needs_cr < 0 ? 0 : needs_cr;
 +}
 +
 +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,
 +			      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 = is_cr_needed(xe1, xe2, m);
  
  	if (marker_size <= 0)
  		marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
  
  	/* Before conflicting part */
 -	size += xdl_recs_copy(xe1, i, m->i1 - i, 0,
 +	size += xdl_recs_copy(xe1, i, m->i1 - i, 0, 0,
  			      dest ? dest + size : NULL);
  
  	if (!dest) {
 @@ -217,7 +227,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
  	}
  
  	/* Postimage from side #1 */
 -	size += xdl_recs_copy(xe1, m->i1, m->chg1, 1,
 +	size += xdl_recs_copy(xe1, m->i1, m->chg1, needs_cr, 1,
  			      dest ? dest + size : NULL);
  
  	if (style == XDL_MERGE_DIFF3) {
 @@ -236,7 +246,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
  				dest[size++] = '\r';
  			dest[size++] = '\n';
  		}
 -		size += xdl_orig_copy(xe1, m->i0, m->chg0, 1,
 +		size += xdl_orig_copy(xe1, m->i0, m->chg0, needs_cr, 1,
  				      dest ? dest + size : NULL);
  	}
  
 @@ -251,7 +261,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
  	}
  
  	/* Postimage from side #2 */
 -	size += xdl_recs_copy(xe2, m->i2, m->chg2, 1,
 +	size += xdl_recs_copy(xe2, m->i2, m->chg2, needs_cr, 1,
  			      dest ? dest + size : NULL);
  	if (!dest) {
  		size += marker_size + 1 + needs_cr + marker2_size;
 @@ -290,21 +300,24 @@ static int xdl_fill_merge_buffer(xdfenv_t *xe1, const char *name1,
  						  marker_size);
  		else if (m->mode & 3) {
  			/* Before conflicting part */
 -			size += xdl_recs_copy(xe1, i, m->i1 - i, 0,
 +			size += xdl_recs_copy(xe1, i, m->i1 - i, 0, 0,
  					      dest ? dest + size : NULL);
  			/* Postimage from side #1 */
 -			if (m->mode & 1)
 -				size += xdl_recs_copy(xe1, m->i1, m->chg1, (m->mode & 2),
 +			if (m->mode & 1) {
 +				int needs_cr = is_cr_needed(xe1, xe2, m);
 +
 +				size += xdl_recs_copy(xe1, m->i1, m->chg1, needs_cr, (m->mode & 2),
  						      dest ? dest + size : NULL);
 +			}
  			/* Postimage from side #2 */
  			if (m->mode & 2)
 -				size += xdl_recs_copy(xe2, m->i2, m->chg2, 0,
 +				size += xdl_recs_copy(xe2, m->i2, m->chg2, 0, 0,
  						      dest ? dest + size : NULL);
  		} else
  			continue;
  		i = m->i1 + m->chg1;
  	}
 -	size += xdl_recs_copy(xe1, i, xe1->xdf2.nrec - i, 0,
 +	size += xdl_recs_copy(xe1, i, xe1->xdf2.nrec - i, 0, 0,
  			      dest ? dest + size : NULL);
  	return size;
  }

-- 
2.7.0.windows.1.7.g55a05c8

[PATCH v3 1/2] merge-file: let conflict markers match end-of-line style of the context

[Johannes Schindelin]

When merging files with CR/LF line endings, the conflict markers should
match those, lest the output file has mixed line endings.

This is particularly of interest on Windows, where some editors get
*really* confused by mixed line endings.

The original version of this patch by Beat Bolli respected core.eol, and
a subsequent improvement by this developer also respected gitattributes.
This approach was suboptimal, though: `git merge-file` was invented as a
drop-in replacement for GNU merge and as such has no problem operating
outside of any repository at all!

Another problem with the original approach was pointed out by Junio
Hamano: legacy repositories might have their text files committed using
CR/LF line endings (and core.eol and the gitattributes would give us a
false impression there). Therefore, the much superior approach is to
simply match the context's line endings, if any.

We actually do not have to look at the *entire* context at all: if the
files are all LF-only, or if they all have CR/LF line endings, it is
sufficient to look at just a *single* line to match that style. And if
the line endings are mixed anyway, it is *still* okay to imitate just a
single line's eol: we will just add to the pile of mixed line endings,
and there is nothing we can do about that.

So what we do is: we look at the line preceding the conflict, falling
back to the line preceding that in case it was the last line and had no
line ending, falling back to the first line, first in the first
post-image, then the second post-image, and finally the pre-image.
If we find consistent CR/LF (or undecided) end-of-line style, we match
that, otherwise we use LF-only line endings for the conflict markers.

Note that while it is true that there have to be at least two lines we
can look at (otherwise there would be no conflict), the same is not true
for line *endings*: the three files in question could all consist of a
single line without any line ending, each. In this case we fall back to
using LF-only.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t6023-merge-file.sh | 12 +++++++++++
 xdiff/xmerge.c        | 57 +++++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 65 insertions(+), 4 deletions(-)

diff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh
index 190ee90..bb20cbc 100755
--- a/t/t6023-merge-file.sh
+++ b/t/t6023-merge-file.sh
@@ -346,4 +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 match existing line endings' '
+	printf "1\\r\\n2\\r\\n3" >crlf-orig.txt &&
+	printf "1\\r\\n2\\r\\n4" >crlf-diff1.txt &&
+	printf "1\\r\\n2\\r\\n5" >crlf-diff2.txt &&
+	test_must_fail git -c core.eol=crlf merge-file -p \
+		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 >nolf.txt &&
+	test $(tr "\015" Q <nolf.txt | grep "\\.txtQ$" | wc -l) = 0
+'
+
 test_done
diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c
index 625198e..c852acc 100644
--- a/xdiff/xmerge.c
+++ b/xdiff/xmerge.c
@@ -143,6 +143,35 @@ 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,
@@ -152,6 +181,18 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 	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 + marker1_size;
+		size += marker_size + 1 + needs_cr + marker1_size;
 	} else {
 		memset(dest + size, '<', marker_size);
 		size += marker_size;
@@ -170,6 +211,8 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 			memcpy(dest + size + 1, name1, marker1_size - 1);
 			size += marker1_size;
 		}
+		if (needs_cr)
+			dest[size++] = '\r';
 		dest[size++] = '\n';
 	}
 
@@ -180,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 + marker3_size;
+			size += marker_size + 1 + needs_cr + marker3_size;
 		} else {
 			memset(dest + size, '|', marker_size);
 			size += marker_size;
@@ -189,6 +232,8 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 				memcpy(dest + size + 1, name3, marker3_size - 1);
 				size += marker3_size;
 			}
+			if (needs_cr)
+				dest[size++] = '\r';
 			dest[size++] = '\n';
 		}
 		size += xdl_orig_copy(xe1, m->i0, m->chg0, 1,
@@ -196,10 +241,12 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 	}
 
 	if (!dest) {
-		size += marker_size + 1;
+		size += marker_size + 1 + needs_cr;
 	} else {
 		memset(dest + size, '=', marker_size);
 		size += marker_size;
+		if (needs_cr)
+			dest[size++] = '\r';
 		dest[size++] = '\n';
 	}
 
@@ -207,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 + marker2_size;
+		size += marker_size + 1 + needs_cr + marker2_size;
 	} else {
 		memset(dest + size, '>', marker_size);
 		size += marker_size;
@@ -216,6 +263,8 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 			memcpy(dest + size + 1, name2, marker2_size - 1);
 			size += marker2_size;
 		}
+		if (needs_cr)
+			dest[size++] = '\r';
 		dest[size++] = '\n';
 	}
 	return size;
-- 
2.7.0.windows.1.7.g55a05c8

Re: [PATCH v3 1/2] merge-file: let conflict markers match end-of-line style of the context

[Torsten Bögershausen]

On 25.01.16 09:07, Johannes Schindelin wrote:
[]
> diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c
> index 625198e..c852acc 100644
> --- a/xdiff/xmerge.c
> +++ b/xdiff/xmerge.c
> @@ -143,6 +143,35 @@ 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)
Minot nit: Could that be 
is_eol_crlf(xdfile_t *file, int lineno)
(or may be)
is_eol_crlf(const xdfile_t *file, int lineno)
(or may be)
has_eol_crlf(const xdfile_t *file, int lineno)

> +{
> +	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';
> +}
> +

Re: [PATCH v3 1/2] merge-file: let conflict markers match end-of-line style of the context

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1149 bytes --]

Hi Torsten,

On Mon, 25 Jan 2016, Torsten Bögershausen wrote:

> On 25.01.16 09:07, Johannes Schindelin wrote:
> []
> > diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c
> > index 625198e..c852acc 100644
> > --- a/xdiff/xmerge.c
> > +++ b/xdiff/xmerge.c
> > @@ -143,6 +143,35 @@ 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)
> Minot nit: Could that be 
> is_eol_crlf(xdfile_t *file, int lineno)
> (or may be)
> is_eol_crlf(const xdfile_t *file, int lineno)
> (or may be)
> has_eol_crlf(const xdfile_t *file, int lineno)

The surrounding code consistently uses i, and not lineno. The reason (I
guess) is that i starts at 0 whereas lineno would have to start at 1, and
we can easily avoid adding and subtracting 1 all the time.

Ciao,
Dscho

Re: [PATCH v3 1/2] merge-file: let conflict markers match end-of-line style of the context

[Junio C Hamano]

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

> We actually do not have to look at the *entire* context at all: if the
> files are all LF-only, or if they all have CR/LF line endings, it is
> sufficient to look at just a *single* line to match that style. And if
> the line endings are mixed anyway, it is *still* okay to imitate just a
> single line's eol: we will just add to the pile of mixed line endings,
> and there is nothing we can do about that.

Isn't there one thing we can do still?  If we use CRLF for the
marker lines when the content is already mixed, I'd think it would
help Notepad (not necessary for Notepad2 or Wordpad IIUC) by making
sure that they can see where the marker lines end correctly.

I do not care too deeply about this; just throwing it out as a
possibility to help Windowsy folks a bit more.

> Note that while it is true that there have to be at least two lines we
> can look at (otherwise there would be no conflict), the same is not true
> for line *endings*: the three files in question could all consist of a
> single line without any line ending, each. In this case we fall back to
> using LF-only.

Yeah, this is tricky, and from the same "helping Notepad that
concatenates lines with LF-only" perspective I should perhaps be
suggesting to use CRLF in such a case, too, but I would say we
should not do so.  Three variants of a LF-only file may have
conflict at the incomplete last line, and if we only look at their
"no EOL"-ness and decide to add CRLF to the result, that would be
irritatingly wrong.

Re: [PATCH v3 1/2] merge-file: let conflict markers match end-of-line style of the context

[Johannes Schindelin]

Hi Junio,

On Mon, 25 Jan 2016, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > We actually do not have to look at the *entire* context at all: if the
> > files are all LF-only, or if they all have CR/LF line endings, it is
> > sufficient to look at just a *single* line to match that style. And if
> > the line endings are mixed anyway, it is *still* okay to imitate just a
> > single line's eol: we will just add to the pile of mixed line endings,
> > and there is nothing we can do about that.
> 
> Isn't there one thing we can do still?  If we use CRLF for the
> marker lines when the content is already mixed, I'd think it would
> help Notepad (not necessary for Notepad2 or Wordpad IIUC) by making
> sure that they can see where the marker lines end correctly.

Not sure. You might end up with a very long line (containing plenty of LF
"characters") and the conflict marker *at the end* of said line, with a
CR/LF after it. I would not call that particularly helpful.

Seeing as we really cannot do anything in this case, I thought it would be
a good idea to avoid trying (and failing) to be smart here.

> > Note that while it is true that there have to be at least two lines we
> > can look at (otherwise there would be no conflict), the same is not true
> > for line *endings*: the three files in question could all consist of a
> > single line without any line ending, each. In this case we fall back to
> > using LF-only.
> 
> Yeah, this is tricky, and from the same "helping Notepad that
> concatenates lines with LF-only" perspective I should perhaps be
> suggesting to use CRLF in such a case, too, but I would say we
> should not do so.  Three variants of a LF-only file may have
> conflict at the incomplete last line, and if we only look at their
> "no EOL"-ness and decide to add CRLF to the result, that would be
> irritatingly wrong.

Oh, but there is the fall-back to the first line. So if we have three
variants of an LF-only file, the logic will figure out that LF-only
end-of-lines are to be used.

So the *only* case where we really have to pick and choose is when all
three files contain only one (or no) line that is not terminated by a line
feed.

I briefly considered to choose EOL_NATIVE in that case, but I really do
not like that unnecessary deviation from Linux Git.

Ciao,
Dscho

Re: [PATCH v3 1/2] merge-file: let conflict markers match end-of-line style of the context

[Junio C Hamano]

On Tue, Jan 26, 2016 at 1:04 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Not sure. You might end up with a very long line (containing plenty of LF
> "characters") and the conflict marker *at the end* of said line, with a
> CR/LF after it. I would not call that particularly helpful.
>
> Seeing as we really cannot do anything in this case, I thought it would be
> a good idea to avoid trying (and failing) to be smart here.

Nicely and clearly explained; thanks, and I agree with your reasoning.

[PATCH v3 2/2] merge-file: ensure that conflict sections match eol style

[Johannes Schindelin]

In the previous patch, we made sure that the conflict markers themselves
match the end-of-line style of the input files. However, this still left
out the conflicting text itself: if it lacks a trailing newline, we
add one, and should add a carriage return when appropriate, too.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t6023-merge-file.sh |  3 ++-
 xdiff/xmerge.c        | 61 +++++++++++++++++++++++++++++++--------------------
 2 files changed, 39 insertions(+), 25 deletions(-)

diff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh
index bb20cbc..1390548 100755
--- a/t/t6023-merge-file.sh
+++ b/t/t6023-merge-file.sh
@@ -346,13 +346,14 @@ 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 match existing line endings' '
+test_expect_success 'conflict sections match existing line endings' '
 	printf "1\\r\\n2\\r\\n3" >crlf-orig.txt &&
 	printf "1\\r\\n2\\r\\n4" >crlf-diff1.txt &&
 	printf "1\\r\\n2\\r\\n5" >crlf-diff2.txt &&
 	test_must_fail git -c core.eol=crlf merge-file -p \
 		crlf-diff1.txt crlf-orig.txt crlf-diff2.txt >crlf.txt &&
 	test $(tr "\015" Q <crlf.txt | grep "\\.txtQ$" | wc -l) = 3 &&
+	test $(tr "\015" Q <crlf.txt | grep "[345]Q$" | wc -l) = 3 &&
 	test_must_fail git -c core.eol=crlf merge-file -p \
 		nolf-diff1.txt nolf-orig.txt nolf-diff2.txt >nolf.txt &&
 	test $(tr "\015" Q <nolf.txt | grep "\\.txtQ$" | wc -l) = 0
diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c
index c852acc..d98f430 100644
--- a/xdiff/xmerge.c
+++ b/xdiff/xmerge.c
@@ -109,7 +109,7 @@ static int xdl_merge_cmp_lines(xdfenv_t *xe1, int i1, xdfenv_t *xe2, int i2,
 	return 0;
 }
 
-static int xdl_recs_copy_0(int use_orig, xdfenv_t *xe, int i, int count, int add_nl, char *dest)
+static int xdl_recs_copy_0(int use_orig, xdfenv_t *xe, int i, int count, int needs_cr, int add_nl, char *dest)
 {
 	xrecord_t **recs;
 	int size = 0;
@@ -125,6 +125,12 @@ static int xdl_recs_copy_0(int use_orig, xdfenv_t *xe, int i, int count, int add
 	if (add_nl) {
 		i = recs[count - 1]->size;
 		if (i == 0 || recs[count - 1]->ptr[i - 1] != '\n') {
+			if (needs_cr) {
+				if (dest)
+					dest[size] = '\r';
+				size++;
+			}
+
 			if (dest)
 				dest[size] = '\n';
 			size++;
@@ -133,14 +139,14 @@ static int xdl_recs_copy_0(int use_orig, xdfenv_t *xe, int i, int count, int add
 	return size;
 }
 
-static int xdl_recs_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest)
+static int xdl_recs_copy(xdfenv_t *xe, int i, int count, int needs_cr, int add_nl, char *dest)
 {
-	return xdl_recs_copy_0(0, xe, i, count, add_nl, dest);
+	return xdl_recs_copy_0(0, xe, i, count, needs_cr, add_nl, dest);
 }
 
-static int xdl_orig_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest)
+static int xdl_orig_copy(xdfenv_t *xe, int i, int count, int needs_cr, int add_nl, char *dest)
 {
-	return xdl_recs_copy_0(1, xe, i, count, add_nl, dest);
+	return xdl_recs_copy_0(1, xe, i, count, needs_cr, add_nl, dest);
 }
 
 /*
@@ -172,15 +178,8 @@ static int is_eol_crlf(xdfile_t *file, int i)
 		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,
-			      xdmerge_t *m, char *dest, int marker_size)
+static int is_cr_needed(xdfenv_t *xe1, xdfenv_t *xe2, xdmerge_t *m)
 {
-	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 */
@@ -191,14 +190,25 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 	if (needs_cr)
 		needs_cr = is_eol_crlf(&xe1->xdf1, 0);
 	/* If still undecided, use LF-only */
-	if (needs_cr < 0)
-		needs_cr = 0;
+	return needs_cr < 0 ? 0 : needs_cr;
+}
+
+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,
+			      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 = is_cr_needed(xe1, xe2, m);
 
 	if (marker_size <= 0)
 		marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
 
 	/* Before conflicting part */
-	size += xdl_recs_copy(xe1, i, m->i1 - i, 0,
+	size += xdl_recs_copy(xe1, i, m->i1 - i, 0, 0,
 			      dest ? dest + size : NULL);
 
 	if (!dest) {
@@ -217,7 +227,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 	}
 
 	/* Postimage from side #1 */
-	size += xdl_recs_copy(xe1, m->i1, m->chg1, 1,
+	size += xdl_recs_copy(xe1, m->i1, m->chg1, needs_cr, 1,
 			      dest ? dest + size : NULL);
 
 	if (style == XDL_MERGE_DIFF3) {
@@ -236,7 +246,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 				dest[size++] = '\r';
 			dest[size++] = '\n';
 		}
-		size += xdl_orig_copy(xe1, m->i0, m->chg0, 1,
+		size += xdl_orig_copy(xe1, m->i0, m->chg0, needs_cr, 1,
 				      dest ? dest + size : NULL);
 	}
 
@@ -251,7 +261,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 	}
 
 	/* Postimage from side #2 */
-	size += xdl_recs_copy(xe2, m->i2, m->chg2, 1,
+	size += xdl_recs_copy(xe2, m->i2, m->chg2, needs_cr, 1,
 			      dest ? dest + size : NULL);
 	if (!dest) {
 		size += marker_size + 1 + needs_cr + marker2_size;
@@ -290,21 +300,24 @@ static int xdl_fill_merge_buffer(xdfenv_t *xe1, const char *name1,
 						  marker_size);
 		else if (m->mode & 3) {
 			/* Before conflicting part */
-			size += xdl_recs_copy(xe1, i, m->i1 - i, 0,
+			size += xdl_recs_copy(xe1, i, m->i1 - i, 0, 0,
 					      dest ? dest + size : NULL);
 			/* Postimage from side #1 */
-			if (m->mode & 1)
-				size += xdl_recs_copy(xe1, m->i1, m->chg1, (m->mode & 2),
+			if (m->mode & 1) {
+				int needs_cr = is_cr_needed(xe1, xe2, m);
+
+				size += xdl_recs_copy(xe1, m->i1, m->chg1, needs_cr, (m->mode & 2),
 						      dest ? dest + size : NULL);
+			}
 			/* Postimage from side #2 */
 			if (m->mode & 2)
-				size += xdl_recs_copy(xe2, m->i2, m->chg2, 0,
+				size += xdl_recs_copy(xe2, m->i2, m->chg2, 0, 0,
 						      dest ? dest + size : NULL);
 		} else
 			continue;
 		i = m->i1 + m->chg1;
 	}
-	size += xdl_recs_copy(xe1, i, xe1->xdf2.nrec - i, 0,
+	size += xdl_recs_copy(xe1, i, xe1->xdf2.nrec - i, 0, 0,
 			      dest ? dest + size : NULL);
 	return size;
 }
-- 
2.7.0.windows.1.7.g55a05c8

Re: [PATCH v3 2/2] merge-file: ensure that conflict sections match eol style

[Johannes Schindelin]

Hi,

On Mon, 25 Jan 2016, Johannes Schindelin wrote:

> diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c
> index c852acc..d98f430 100644
> --- a/xdiff/xmerge.c
> +++ b/xdiff/xmerge.c
> @@ -172,15 +178,8 @@ static int is_eol_crlf(xdfile_t *file, int i)
>  		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,
> -			      xdmerge_t *m, char *dest, int marker_size)
> +static int is_cr_needed(xdfenv_t *xe1, xdfenv_t *xe2, xdmerge_t *m)
>  {
> -	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 */
> @@ -191,14 +190,25 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
>  	if (needs_cr)
>  		needs_cr = is_eol_crlf(&xe1->xdf1, 0);
>  	/* If still undecided, use LF-only */
> -	if (needs_cr < 0)
> -		needs_cr = 0;
> +	return needs_cr < 0 ? 0 : needs_cr;
> +}
> +
> +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,
> +			      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 = is_cr_needed(xe1, xe2, m);
>  
>  	if (marker_size <= 0)
>  		marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
>  

Oh bummer. I just realized that I should have refactored that already in
patch 1/2 before sending out v3. Of course it would be true to history to
do the refactoring only as part of 2/2, but who cares about true history
when one can rewrite it?

Will send out v4 in a while (I want to wait for more feedback in case I
need to change more things.) In the meantime, you can always look at my
patch series' current state here:

	https://github.com/git/git/compare/next...dscho:merge-marker-crlf

Ciao,
Dscho

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

[Johannes Schindelin]

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 first commit message.

Triggered by a comment by Junio, I realized that a second patch is
needed: we need to make sure that the conflicting sections are also
augmented by the appropriate line endings if they lack any.

The interdiff vs v3 is empty because I only pulled a refactoring
from 2/2 into 1/2 already, making the series easier to read.


Johannes Schindelin (2):
  merge-file: let conflict markers match end-of-line style of the
    context
  merge-file: ensure that conflict sections match eol style

 t/t6023-merge-file.sh | 13 +++++++
 xdiff/xmerge.c        | 98 +++++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 93 insertions(+), 18 deletions(-)

-- 
2.7.0.windows.1.7.g55a05c8

[PATCH v4 1/2] merge-file: let conflict markers match end-of-line style of the context

[Johannes Schindelin]

When merging files with CR/LF line endings, the conflict markers should
match those, lest the output file has mixed line endings.

This is particularly of interest on Windows, where some editors get
*really* confused by mixed line endings.

The original version of this patch by Beat Bolli respected core.eol, and
a subsequent improvement by this developer also respected gitattributes.
This approach was suboptimal, though: `git merge-file` was invented as a
drop-in replacement for GNU merge and as such has no problem operating
outside of any repository at all!

Another problem with the original approach was pointed out by Junio
Hamano: legacy repositories might have their text files committed using
CR/LF line endings (and core.eol and the gitattributes would give us a
false impression there). Therefore, the much superior approach is to
simply match the context's line endings, if any.

We actually do not have to look at the *entire* context at all: if the
files are all LF-only, or if they all have CR/LF line endings, it is
sufficient to look at just a *single* line to match that style. And if
the line endings are mixed anyway, it is *still* okay to imitate just a
single line's eol: we will just add to the pile of mixed line endings,
and there is nothing we can do about that.

So what we do is: we look at the line preceding the conflict, falling
back to the line preceding that in case it was the last line and had no
line ending, falling back to the first line, first in the first
post-image, then the second post-image, and finally the pre-image.
If we find consistent CR/LF (or undecided) end-of-line style, we match
that, otherwise we use LF-only line endings for the conflict markers.

Note that while it is true that there have to be at least two lines we
can look at (otherwise there would be no conflict), the same is not true
for line *endings*: the three files in question could all consist of a
single line without any line ending, each. In this case we fall back to
using LF-only.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t6023-merge-file.sh | 12 ++++++++++
 xdiff/xmerge.c        | 61 +++++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 69 insertions(+), 4 deletions(-)

diff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh
index 190ee90..bb20cbc 100755
--- a/t/t6023-merge-file.sh
+++ b/t/t6023-merge-file.sh
@@ -346,4 +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 match existing line endings' '
+	printf "1\\r\\n2\\r\\n3" >crlf-orig.txt &&
+	printf "1\\r\\n2\\r\\n4" >crlf-diff1.txt &&
+	printf "1\\r\\n2\\r\\n5" >crlf-diff2.txt &&
+	test_must_fail git -c core.eol=crlf merge-file -p \
+		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 >nolf.txt &&
+	test $(tr "\015" Q <nolf.txt | grep "\\.txtQ$" | wc -l) = 0
+'
+
 test_done
diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c
index 625198e..7b21a6b 100644
--- a/xdiff/xmerge.c
+++ b/xdiff/xmerge.c
@@ -143,6 +143,50 @@ 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 is_cr_needed(xdfenv_t *xe1, xdfenv_t *xe2, xdmerge_t *m)
+{
+	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 */
+	return needs_cr < 0 ? 0 : needs_cr;
+}
+
 static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 			      xdfenv_t *xe2, const char *name2,
 			      const char *name3,
@@ -152,6 +196,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 	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 = is_cr_needed(xe1, xe2, m);
 
 	if (marker_size <= 0)
 		marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
@@ -161,7 +206,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 + needs_cr + marker1_size;
 	} else {
 		memset(dest + size, '<', marker_size);
 		size += marker_size;
@@ -170,6 +215,8 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 			memcpy(dest + size + 1, name1, marker1_size - 1);
 			size += marker1_size;
 		}
+		if (needs_cr)
+			dest[size++] = '\r';
 		dest[size++] = '\n';
 	}
 
@@ -180,7 +227,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 + needs_cr + marker3_size;
 		} else {
 			memset(dest + size, '|', marker_size);
 			size += marker_size;
@@ -189,6 +236,8 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 				memcpy(dest + size + 1, name3, marker3_size - 1);
 				size += marker3_size;
 			}
+			if (needs_cr)
+				dest[size++] = '\r';
 			dest[size++] = '\n';
 		}
 		size += xdl_orig_copy(xe1, m->i0, m->chg0, 1,
@@ -196,10 +245,12 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 	}
 
 	if (!dest) {
-		size += marker_size + 1;
+		size += marker_size + 1 + needs_cr;
 	} else {
 		memset(dest + size, '=', marker_size);
 		size += marker_size;
+		if (needs_cr)
+			dest[size++] = '\r';
 		dest[size++] = '\n';
 	}
 
@@ -207,7 +258,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 + needs_cr + marker2_size;
 	} else {
 		memset(dest + size, '>', marker_size);
 		size += marker_size;
@@ -216,6 +267,8 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 			memcpy(dest + size + 1, name2, marker2_size - 1);
 			size += marker2_size;
 		}
+		if (needs_cr)
+			dest[size++] = '\r';
 		dest[size++] = '\n';
 	}
 	return size;
-- 
2.7.0.windows.1.7.g55a05c8

Re: [PATCH v4 1/2] merge-file: let conflict markers match end-of-line style of the context

[Junio C Hamano]

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

> +static int is_cr_needed(xdfenv_t *xe1, xdfenv_t *xe2, xdmerge_t *m)
> +{
> +	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 */
> +	return needs_cr < 0 ? 0 : needs_cr;
> +}

Retrying with other images when needs_cr is either -1 (unknown) or 1
(known to be true) was tricky; I had to read it twice and think
about it for 30 seconds before convincing myself that the code does
what the log message specifies it should.

That is probably because I was thinking in terms of "do we know that
we need to add a CR?"; if I read "needs_cr" in my head as "we might
want to add a CR", everything becomes much more clear, but perhaps
it is just me.

The return value of this function is definitely "do we need and want
to add a CR", and it is appropriately named.

Thanks.

Re: [PATCH v4 1/2] merge-file: let conflict markers match end-of-line style of the context

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
>> +static int is_cr_needed(xdfenv_t *xe1, xdfenv_t *xe2, xdmerge_t *m)
>> +{
>> +	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 */
>> +	return needs_cr < 0 ? 0 : needs_cr;
>> +}
>
> Retrying with other images when needs_cr is either -1 (unknown) or 1
> (known to be true) was tricky; I had to read it twice and think
> about it for 30 seconds before convincing myself that the code does
> what the log message specifies it should.
>
> That is probably because I was thinking in terms of "do we know that
> we need to add a CR?"; if I read "needs_cr" in my head as "we might
> want to add a CR", everything becomes much more clear, but perhaps
> it is just me.
>
> The return value of this function is definitely "do we need and want
> to add a CR", and it is appropriately named.
>
> Thanks.

Just in case it was unclear, none of the comment above means I want
any part of the patch redone--I am happy with this patch as-is.

Re: [PATCH v4 1/2] merge-file: let conflict markers match end-of-line style of the context

[Johannes Schindelin]

Hi Junio,

On Tue, 26 Jan 2016, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> >
> >> +static int is_cr_needed(xdfenv_t *xe1, xdfenv_t *xe2, xdmerge_t *m)
> >> +{
> >> +	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 */
> >> +	return needs_cr < 0 ? 0 : needs_cr;
> >> +}
> >
> > Retrying with other images when needs_cr is either -1 (unknown) or 1
> > (known to be true) was tricky; I had to read it twice and think
> > about it for 30 seconds before convincing myself that the code does
> > what the log message specifies it should.
> >
> > That is probably because I was thinking in terms of "do we know that
> > we need to add a CR?"; if I read "needs_cr" in my head as "we might
> > want to add a CR", everything becomes much more clear, but perhaps
> > it is just me.
> >
> > The return value of this function is definitely "do we need and want
> > to add a CR", and it is appropriately named.
> >
> > Thanks.
> 
> Just in case it was unclear, none of the comment above means I want
> any part of the patch redone--I am happy with this patch as-is.

Thanks for saying that... I was about to try to make things clearer, but I
could not think of a better term than "needs_cr". The existing code
already has "add_nl" (which means: add a newline *iff* the block to be
copied does not yet have a newline), which I also found confusing.

Oh wait, maybe "eol_is_crlf" instead of "needs_cr"?

Ciao,
Dscho

Re: [PATCH v4 1/2] merge-file: let conflict markers match end-of-line style of the context

[Junio C Hamano]

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

>> Just in case it was unclear, none of the comment above means I want
>> any part of the patch redone--I am happy with this patch as-is.
>
> Thanks for saying that... I was about to try to make things clearer, but I
> could not think of a better term than "needs_cr".

I don't, either ;-).

The primary reason I respond with the "I find this a bit confusing
but it probably is just me" (not just to this patch) is to give an
example of a review comment that demonstrates to the others that the
reviewer understood what is in the patch and the issues around the
change better than a mere unsubstantiated "These look OK to me.",
which does not tell us how carefully the proposed change was
reviewed by the reviewer--such a review does not allow me to "trust
the review that is already done by others" and apply the patches
with minimum cursory scanning and I end up having to carefully read
them myself.

Re: [PATCH v4 1/2] merge-file: let conflict markers match end-of-line style of the context

[Johannes Schindelin]

Hi Junio,

On Wed, 27 Jan 2016, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> Just in case it was unclear, none of the comment above means I want
> >> any part of the patch redone--I am happy with this patch as-is.
> >
> > Thanks for saying that... I was about to try to make things clearer, but I
> > could not think of a better term than "needs_cr".
> 
> I don't, either ;-).
> 
> The primary reason I respond with the "I find this a bit confusing
> but it probably is just me" (not just to this patch) is to give an
> example of a review comment that demonstrates to the others that the
> reviewer understood what is in the patch and the issues around the
> change better than a mere unsubstantiated "These look OK to me.",
> which does not tell us how carefully the proposed change was
> reviewed by the reviewer--such a review does not allow me to "trust
> the review that is already done by others" and apply the patches
> with minimum cursory scanning and I end up having to carefully read
> them myself.

Your response is also an indicator to me that future myself will find the
same code just as confusing as you did, though.

Maybe need_cr -> eol_is_crlf?

Ciao,
Dscho

Re: [PATCH v4 1/2] merge-file: let conflict markers match end-of-line style of the context

[Junio C Hamano]

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

[administrivia: I finally managed to correct e-mail address of peff@,
which has been giving me bounces for all the previous messages in
the thread]

> Your response is also an indicator to me that future myself will find the
> same code just as confusing as you did, though.
>
> Maybe need_cr -> eol_is_crlf?

The crucial question is how well the variable conveys its meaning
when it has -1 as its value.  "need_cr? -- I don't know yet" is as
clear as "eol_is_crlf? -- I don't know yet", if not clearer.  I
personally do not think "eol_is_crlf" is an improvement.  It makes
it unclear _whose_ eol the variable is talking about.  It can be
misread as talking about one or more of the files that are being
merged and mistaken as a statement of a fact (i.e. "we inspected
and know the input file's eol is CRLF").

Compared to that, it is clear that "need_cr" is talking about what
the EOL convention the resulting file should be.

I briefly wondered if the if/if (need_cr)/... cascade that inspects
(conditionally up to) three variants might become cleaner if the
polarity of that variable is flipped (we are allowed to use CRLF
only when we know that among all of the three variants that we can
determine the line termination convention used, all of them use
CRLF), but I didn't think it through seriously.

Re: [PATCH v4 1/2] merge-file: let conflict markers match end-of-line style of the context

[Johannes Schindelin]

Hi Junio,

On Wed, 27 Jan 2016, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> [administrivia: I finally managed to correct e-mail address of peff@,
> which has been giving me bounces for all the previous messages in
> the thread]

Oops! I got those bounces myself, but wrote them off to Eric's account
without checking (because I consistently had problems sending emails to
him in the past). Sorry about that.

> > Your response is also an indicator to me that future myself will find the
> > same code just as confusing as you did, though.
> >
> > Maybe need_cr -> eol_is_crlf?
> 
> The crucial question is how well the variable conveys its meaning
> when it has -1 as its value.  "need_cr? -- I don't know yet" is as
> clear as "eol_is_crlf? -- I don't know yet", if not clearer.  I
> personally do not think "eol_is_crlf" is an improvement.

Okay, then. Let's keep the "needs_cr" name.

Ciao,
Dscho
> it unclear _whose_ eol the variable is talking about.  It can be
> misread as talking about one or more of the files that are being
> merged and mistaken as a statement of a fact (i.e. "we inspected
> and know the input file's eol is CRLF").
> 
> Compared to that, it is clear that "need_cr" is talking about what
> the EOL convention the resulting file should be.
> 
> I briefly wondered if the if/if (need_cr)/... cascade that inspects
> (conditionally up to) three variants might become cleaner if the
> polarity of that variable is flipped (we are allowed to use CRLF
> only when we know that among all of the three variants that we can
> determine the line termination convention used, all of them use
> CRLF), but I didn't think it through seriously.
> 
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH v4 2/2] merge-file: ensure that conflict sections match eol style

[Johannes Schindelin]

In the previous patch, we made sure that the conflict markers themselves
match the end-of-line style of the input files. However, this still left
out the conflicting text itself: if it lacks a trailing newline, we
add one, and should add a carriage return when appropriate, too.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t6023-merge-file.sh |  3 ++-
 xdiff/xmerge.c        | 37 +++++++++++++++++++++++--------------
 2 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh
index bb20cbc..1390548 100755
--- a/t/t6023-merge-file.sh
+++ b/t/t6023-merge-file.sh
@@ -346,13 +346,14 @@ 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 match existing line endings' '
+test_expect_success 'conflict sections match existing line endings' '
 	printf "1\\r\\n2\\r\\n3" >crlf-orig.txt &&
 	printf "1\\r\\n2\\r\\n4" >crlf-diff1.txt &&
 	printf "1\\r\\n2\\r\\n5" >crlf-diff2.txt &&
 	test_must_fail git -c core.eol=crlf merge-file -p \
 		crlf-diff1.txt crlf-orig.txt crlf-diff2.txt >crlf.txt &&
 	test $(tr "\015" Q <crlf.txt | grep "\\.txtQ$" | wc -l) = 3 &&
+	test $(tr "\015" Q <crlf.txt | grep "[345]Q$" | wc -l) = 3 &&
 	test_must_fail git -c core.eol=crlf merge-file -p \
 		nolf-diff1.txt nolf-orig.txt nolf-diff2.txt >nolf.txt &&
 	test $(tr "\015" Q <nolf.txt | grep "\\.txtQ$" | wc -l) = 0
diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c
index 7b21a6b..d98f430 100644
--- a/xdiff/xmerge.c
+++ b/xdiff/xmerge.c
@@ -109,7 +109,7 @@ static int xdl_merge_cmp_lines(xdfenv_t *xe1, int i1, xdfenv_t *xe2, int i2,
 	return 0;
 }
 
-static int xdl_recs_copy_0(int use_orig, xdfenv_t *xe, int i, int count, int add_nl, char *dest)
+static int xdl_recs_copy_0(int use_orig, xdfenv_t *xe, int i, int count, int needs_cr, int add_nl, char *dest)
 {
 	xrecord_t **recs;
 	int size = 0;
@@ -125,6 +125,12 @@ static int xdl_recs_copy_0(int use_orig, xdfenv_t *xe, int i, int count, int add
 	if (add_nl) {
 		i = recs[count - 1]->size;
 		if (i == 0 || recs[count - 1]->ptr[i - 1] != '\n') {
+			if (needs_cr) {
+				if (dest)
+					dest[size] = '\r';
+				size++;
+			}
+
 			if (dest)
 				dest[size] = '\n';
 			size++;
@@ -133,14 +139,14 @@ static int xdl_recs_copy_0(int use_orig, xdfenv_t *xe, int i, int count, int add
 	return size;
 }
 
-static int xdl_recs_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest)
+static int xdl_recs_copy(xdfenv_t *xe, int i, int count, int needs_cr, int add_nl, char *dest)
 {
-	return xdl_recs_copy_0(0, xe, i, count, add_nl, dest);
+	return xdl_recs_copy_0(0, xe, i, count, needs_cr, add_nl, dest);
 }
 
-static int xdl_orig_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest)
+static int xdl_orig_copy(xdfenv_t *xe, int i, int count, int needs_cr, int add_nl, char *dest)
 {
-	return xdl_recs_copy_0(1, xe, i, count, add_nl, dest);
+	return xdl_recs_copy_0(1, xe, i, count, needs_cr, add_nl, dest);
 }
 
 /*
@@ -202,7 +208,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 		marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
 
 	/* Before conflicting part */
-	size += xdl_recs_copy(xe1, i, m->i1 - i, 0,
+	size += xdl_recs_copy(xe1, i, m->i1 - i, 0, 0,
 			      dest ? dest + size : NULL);
 
 	if (!dest) {
@@ -221,7 +227,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 	}
 
 	/* Postimage from side #1 */
-	size += xdl_recs_copy(xe1, m->i1, m->chg1, 1,
+	size += xdl_recs_copy(xe1, m->i1, m->chg1, needs_cr, 1,
 			      dest ? dest + size : NULL);
 
 	if (style == XDL_MERGE_DIFF3) {
@@ -240,7 +246,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 				dest[size++] = '\r';
 			dest[size++] = '\n';
 		}
-		size += xdl_orig_copy(xe1, m->i0, m->chg0, 1,
+		size += xdl_orig_copy(xe1, m->i0, m->chg0, needs_cr, 1,
 				      dest ? dest + size : NULL);
 	}
 
@@ -255,7 +261,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 	}
 
 	/* Postimage from side #2 */
-	size += xdl_recs_copy(xe2, m->i2, m->chg2, 1,
+	size += xdl_recs_copy(xe2, m->i2, m->chg2, needs_cr, 1,
 			      dest ? dest + size : NULL);
 	if (!dest) {
 		size += marker_size + 1 + needs_cr + marker2_size;
@@ -294,21 +300,24 @@ static int xdl_fill_merge_buffer(xdfenv_t *xe1, const char *name1,
 						  marker_size);
 		else if (m->mode & 3) {
 			/* Before conflicting part */
-			size += xdl_recs_copy(xe1, i, m->i1 - i, 0,
+			size += xdl_recs_copy(xe1, i, m->i1 - i, 0, 0,
 					      dest ? dest + size : NULL);
 			/* Postimage from side #1 */
-			if (m->mode & 1)
-				size += xdl_recs_copy(xe1, m->i1, m->chg1, (m->mode & 2),
+			if (m->mode & 1) {
+				int needs_cr = is_cr_needed(xe1, xe2, m);
+
+				size += xdl_recs_copy(xe1, m->i1, m->chg1, needs_cr, (m->mode & 2),
 						      dest ? dest + size : NULL);
+			}
 			/* Postimage from side #2 */
 			if (m->mode & 2)
-				size += xdl_recs_copy(xe2, m->i2, m->chg2, 0,
+				size += xdl_recs_copy(xe2, m->i2, m->chg2, 0, 0,
 						      dest ? dest + size : NULL);
 		} else
 			continue;
 		i = m->i1 + m->chg1;
 	}
-	size += xdl_recs_copy(xe1, i, xe1->xdf2.nrec - i, 0,
+	size += xdl_recs_copy(xe1, i, xe1->xdf2.nrec - i, 0, 0,
 			      dest ? dest + size : NULL);
 	return size;
 }
-- 
2.7.0.windows.1.7.g55a05c8

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

[Johannes Schindelin]

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 first commit message.

Triggered by a comment by Junio, I realized that a second patch is
needed: we need to make sure that the conflicting sections are also
augmented by the appropriate line endings if they lack any.

The change relative to v4 is that I now test the correct conflict
markers: the merge-file call we add to t6023 actually produces four
conflict markers because a previous test switched the conflict style
to `diff3` mode. This is only a side effect, though, therefore I
really wanted to avoid testing for it. However, I managed to test
an *incorrect* subset of three, but this is now fixed. Thanks go
to Ramsay Jones for poking my nose onto that issue.


Johannes Schindelin (2):
  merge-file: let conflict markers match end-of-line style of the
    context
  merge-file: ensure that conflict sections match eol style

 t/t6023-merge-file.sh | 13 +++++++
 xdiff/xmerge.c        | 98 +++++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 93 insertions(+), 18 deletions(-)

Interdiff vs v4:

 diff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh
 index 1390548..20aee43 100755
 --- a/t/t6023-merge-file.sh
 +++ b/t/t6023-merge-file.sh
 @@ -352,11 +352,11 @@ test_expect_success 'conflict sections match existing line endings' '
  	printf "1\\r\\n2\\r\\n5" >crlf-diff2.txt &&
  	test_must_fail git -c core.eol=crlf merge-file -p \
  		crlf-diff1.txt crlf-orig.txt crlf-diff2.txt >crlf.txt &&
 -	test $(tr "\015" Q <crlf.txt | grep "\\.txtQ$" | wc -l) = 3 &&
 +	test $(tr "\015" Q <crlf.txt | grep "^[<=>].*Q$" | wc -l) = 3 &&
  	test $(tr "\015" Q <crlf.txt | grep "[345]Q$" | wc -l) = 3 &&
  	test_must_fail git -c core.eol=crlf merge-file -p \
  		nolf-diff1.txt nolf-orig.txt nolf-diff2.txt >nolf.txt &&
 -	test $(tr "\015" Q <nolf.txt | grep "\\.txtQ$" | wc -l) = 0
 +	test $(tr "\015" Q <nolf.txt | grep "^[<=>].*Q$" | wc -l) = 0
  '
  
  test_done

-- 
2.7.0.windows.1.7.g55a05c8

[PATCH v5 1/2] merge-file: let conflict markers match end-of-line style of the context

[Johannes Schindelin]

When merging files with CR/LF line endings, the conflict markers should
match those, lest the output file has mixed line endings.

This is particularly of interest on Windows, where some editors get
*really* confused by mixed line endings.

The original version of this patch by Beat Bolli respected core.eol, and
a subsequent improvement by this developer also respected gitattributes.
This approach was suboptimal, though: `git merge-file` was invented as a
drop-in replacement for GNU merge and as such has no problem operating
outside of any repository at all!

Another problem with the original approach was pointed out by Junio
Hamano: legacy repositories might have their text files committed using
CR/LF line endings (and core.eol and the gitattributes would give us a
false impression there). Therefore, the much superior approach is to
simply match the context's line endings, if any.

We actually do not have to look at the *entire* context at all: if the
files are all LF-only, or if they all have CR/LF line endings, it is
sufficient to look at just a *single* line to match that style. And if
the line endings are mixed anyway, it is *still* okay to imitate just a
single line's eol: we will just add to the pile of mixed line endings,
and there is nothing we can do about that.

So what we do is: we look at the line preceding the conflict, falling
back to the line preceding that in case it was the last line and had no
line ending, falling back to the first line, first in the first
post-image, then the second post-image, and finally the pre-image.
If we find consistent CR/LF (or undecided) end-of-line style, we match
that, otherwise we use LF-only line endings for the conflict markers.

Note that while it is true that there have to be at least two lines we
can look at (otherwise there would be no conflict), the same is not true
for line *endings*: the three files in question could all consist of a
single line without any line ending, each. In this case we fall back to
using LF-only.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t6023-merge-file.sh | 12 ++++++++++
 xdiff/xmerge.c        | 61 +++++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 69 insertions(+), 4 deletions(-)

diff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh
index 190ee90..a082116 100755
--- a/t/t6023-merge-file.sh
+++ b/t/t6023-merge-file.sh
@@ -346,4 +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 match existing line endings' '
+	printf "1\\r\\n2\\r\\n3" >crlf-orig.txt &&
+	printf "1\\r\\n2\\r\\n4" >crlf-diff1.txt &&
+	printf "1\\r\\n2\\r\\n5" >crlf-diff2.txt &&
+	test_must_fail git -c core.eol=crlf merge-file -p \
+		crlf-diff1.txt crlf-orig.txt crlf-diff2.txt >crlf.txt &&
+	test $(tr "\015" Q <crlf.txt | grep "^[<=>].*Q$" | wc -l) = 3 &&
+	test_must_fail git -c core.eol=crlf merge-file -p \
+		nolf-diff1.txt nolf-orig.txt nolf-diff2.txt >nolf.txt &&
+	test $(tr "\015" Q <nolf.txt | grep "^[<=>].*Q$" | wc -l) = 0
+'
+
 test_done
diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c
index 625198e..7b21a6b 100644
--- a/xdiff/xmerge.c
+++ b/xdiff/xmerge.c
@@ -143,6 +143,50 @@ 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 is_cr_needed(xdfenv_t *xe1, xdfenv_t *xe2, xdmerge_t *m)
+{
+	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 */
+	return needs_cr < 0 ? 0 : needs_cr;
+}
+
 static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 			      xdfenv_t *xe2, const char *name2,
 			      const char *name3,
@@ -152,6 +196,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 	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 = is_cr_needed(xe1, xe2, m);
 
 	if (marker_size <= 0)
 		marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
@@ -161,7 +206,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 + needs_cr + marker1_size;
 	} else {
 		memset(dest + size, '<', marker_size);
 		size += marker_size;
@@ -170,6 +215,8 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 			memcpy(dest + size + 1, name1, marker1_size - 1);
 			size += marker1_size;
 		}
+		if (needs_cr)
+			dest[size++] = '\r';
 		dest[size++] = '\n';
 	}
 
@@ -180,7 +227,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 + needs_cr + marker3_size;
 		} else {
 			memset(dest + size, '|', marker_size);
 			size += marker_size;
@@ -189,6 +236,8 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 				memcpy(dest + size + 1, name3, marker3_size - 1);
 				size += marker3_size;
 			}
+			if (needs_cr)
+				dest[size++] = '\r';
 			dest[size++] = '\n';
 		}
 		size += xdl_orig_copy(xe1, m->i0, m->chg0, 1,
@@ -196,10 +245,12 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 	}
 
 	if (!dest) {
-		size += marker_size + 1;
+		size += marker_size + 1 + needs_cr;
 	} else {
 		memset(dest + size, '=', marker_size);
 		size += marker_size;
+		if (needs_cr)
+			dest[size++] = '\r';
 		dest[size++] = '\n';
 	}
 
@@ -207,7 +258,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 + needs_cr + marker2_size;
 	} else {
 		memset(dest + size, '>', marker_size);
 		size += marker_size;
@@ -216,6 +267,8 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 			memcpy(dest + size + 1, name2, marker2_size - 1);
 			size += marker2_size;
 		}
+		if (needs_cr)
+			dest[size++] = '\r';
 		dest[size++] = '\n';
 	}
 	return size;
-- 
2.7.0.windows.1.7.g55a05c8

[PATCH v5 2/2] merge-file: ensure that conflict sections match eol style

[Johannes Schindelin]

In the previous patch, we made sure that the conflict markers themselves
match the end-of-line style of the input files. However, this still left
out the conflicting text itself: if it lacks a trailing newline, we
add one, and should add a carriage return when appropriate, too.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t6023-merge-file.sh |  3 ++-
 xdiff/xmerge.c        | 37 +++++++++++++++++++++++--------------
 2 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh
index a082116..20aee43 100755
--- a/t/t6023-merge-file.sh
+++ b/t/t6023-merge-file.sh
@@ -346,13 +346,14 @@ 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 match existing line endings' '
+test_expect_success 'conflict sections match existing line endings' '
 	printf "1\\r\\n2\\r\\n3" >crlf-orig.txt &&
 	printf "1\\r\\n2\\r\\n4" >crlf-diff1.txt &&
 	printf "1\\r\\n2\\r\\n5" >crlf-diff2.txt &&
 	test_must_fail git -c core.eol=crlf merge-file -p \
 		crlf-diff1.txt crlf-orig.txt crlf-diff2.txt >crlf.txt &&
 	test $(tr "\015" Q <crlf.txt | grep "^[<=>].*Q$" | wc -l) = 3 &&
+	test $(tr "\015" Q <crlf.txt | grep "[345]Q$" | wc -l) = 3 &&
 	test_must_fail git -c core.eol=crlf merge-file -p \
 		nolf-diff1.txt nolf-orig.txt nolf-diff2.txt >nolf.txt &&
 	test $(tr "\015" Q <nolf.txt | grep "^[<=>].*Q$" | wc -l) = 0
diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c
index 7b21a6b..d98f430 100644
--- a/xdiff/xmerge.c
+++ b/xdiff/xmerge.c
@@ -109,7 +109,7 @@ static int xdl_merge_cmp_lines(xdfenv_t *xe1, int i1, xdfenv_t *xe2, int i2,
 	return 0;
 }
 
-static int xdl_recs_copy_0(int use_orig, xdfenv_t *xe, int i, int count, int add_nl, char *dest)
+static int xdl_recs_copy_0(int use_orig, xdfenv_t *xe, int i, int count, int needs_cr, int add_nl, char *dest)
 {
 	xrecord_t **recs;
 	int size = 0;
@@ -125,6 +125,12 @@ static int xdl_recs_copy_0(int use_orig, xdfenv_t *xe, int i, int count, int add
 	if (add_nl) {
 		i = recs[count - 1]->size;
 		if (i == 0 || recs[count - 1]->ptr[i - 1] != '\n') {
+			if (needs_cr) {
+				if (dest)
+					dest[size] = '\r';
+				size++;
+			}
+
 			if (dest)
 				dest[size] = '\n';
 			size++;
@@ -133,14 +139,14 @@ static int xdl_recs_copy_0(int use_orig, xdfenv_t *xe, int i, int count, int add
 	return size;
 }
 
-static int xdl_recs_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest)
+static int xdl_recs_copy(xdfenv_t *xe, int i, int count, int needs_cr, int add_nl, char *dest)
 {
-	return xdl_recs_copy_0(0, xe, i, count, add_nl, dest);
+	return xdl_recs_copy_0(0, xe, i, count, needs_cr, add_nl, dest);
 }
 
-static int xdl_orig_copy(xdfenv_t *xe, int i, int count, int add_nl, char *dest)
+static int xdl_orig_copy(xdfenv_t *xe, int i, int count, int needs_cr, int add_nl, char *dest)
 {
-	return xdl_recs_copy_0(1, xe, i, count, add_nl, dest);
+	return xdl_recs_copy_0(1, xe, i, count, needs_cr, add_nl, dest);
 }
 
 /*
@@ -202,7 +208,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 		marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
 
 	/* Before conflicting part */
-	size += xdl_recs_copy(xe1, i, m->i1 - i, 0,
+	size += xdl_recs_copy(xe1, i, m->i1 - i, 0, 0,
 			      dest ? dest + size : NULL);
 
 	if (!dest) {
@@ -221,7 +227,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 	}
 
 	/* Postimage from side #1 */
-	size += xdl_recs_copy(xe1, m->i1, m->chg1, 1,
+	size += xdl_recs_copy(xe1, m->i1, m->chg1, needs_cr, 1,
 			      dest ? dest + size : NULL);
 
 	if (style == XDL_MERGE_DIFF3) {
@@ -240,7 +246,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 				dest[size++] = '\r';
 			dest[size++] = '\n';
 		}
-		size += xdl_orig_copy(xe1, m->i0, m->chg0, 1,
+		size += xdl_orig_copy(xe1, m->i0, m->chg0, needs_cr, 1,
 				      dest ? dest + size : NULL);
 	}
 
@@ -255,7 +261,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 	}
 
 	/* Postimage from side #2 */
-	size += xdl_recs_copy(xe2, m->i2, m->chg2, 1,
+	size += xdl_recs_copy(xe2, m->i2, m->chg2, needs_cr, 1,
 			      dest ? dest + size : NULL);
 	if (!dest) {
 		size += marker_size + 1 + needs_cr + marker2_size;
@@ -294,21 +300,24 @@ static int xdl_fill_merge_buffer(xdfenv_t *xe1, const char *name1,
 						  marker_size);
 		else if (m->mode & 3) {
 			/* Before conflicting part */
-			size += xdl_recs_copy(xe1, i, m->i1 - i, 0,
+			size += xdl_recs_copy(xe1, i, m->i1 - i, 0, 0,
 					      dest ? dest + size : NULL);
 			/* Postimage from side #1 */
-			if (m->mode & 1)
-				size += xdl_recs_copy(xe1, m->i1, m->chg1, (m->mode & 2),
+			if (m->mode & 1) {
+				int needs_cr = is_cr_needed(xe1, xe2, m);
+
+				size += xdl_recs_copy(xe1, m->i1, m->chg1, needs_cr, (m->mode & 2),
 						      dest ? dest + size : NULL);
+			}
 			/* Postimage from side #2 */
 			if (m->mode & 2)
-				size += xdl_recs_copy(xe2, m->i2, m->chg2, 0,
+				size += xdl_recs_copy(xe2, m->i2, m->chg2, 0, 0,
 						      dest ? dest + size : NULL);
 		} else
 			continue;
 		i = m->i1 + m->chg1;
 	}
-	size += xdl_recs_copy(xe1, i, xe1->xdf2.nrec - i, 0,
+	size += xdl_recs_copy(xe1, i, xe1->xdf2.nrec - i, 0, 0,
 			      dest ? dest + size : NULL);
 	return size;
 }
-- 
2.7.0.windows.1.7.g55a05c8

Re: [PATCH v5 0/2] Let merge-file write out conflict markers with correct EOLs

[Junio C Hamano]

Thanks, will queue after looking at the interdiff since v4.