[PATCH v1 1/1] convert: git cherry-pick -Xrenormalize did not work

[PATCH v1 1/1] convert: git cherry-pick -Xrenormalize did not work

[tboegi]

From: Torsten Bögershausen <tboegi@web.de>

Working with a repo that used to be all CRLF. At some point it
was changed to all LF, with `text=auto` in .gitattributes.
Trying to cherry-pick a commit from before the switchover fails:

$ git cherry-pick -Xrenormalize <commit>
    fatal: CRLF would be replaced by LF in [path]

Whenever crlf_action is CRLF_TEXT_XXX and not CRLF_AUTO_XXX,
SAFE_CRLF_RENORMALIZE must be turned into CRLF_SAFE_FALSE.

Reported-by: Eevee (Lexy Munroe) <eevee@veekun.com>
Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---

Thanks for reporting.
Here is a less invasive patch.
Please let me know, if the patch is OK for you
(email address, does it work..)

 convert.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/convert.c b/convert.c
index be91358..526ec1d 100644
--- a/convert.c
+++ b/convert.c
@@ -286,7 +286,9 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
 			checksafe = SAFE_CRLF_FALSE;
 		else if (has_cr_in_index(path))
 			convert_crlf_into_lf = 0;
-	}
+	} else if (checksafe == SAFE_CRLF_RENORMALIZE)
+		checksafe = SAFE_CRLF_FALSE;
+
 	if (checksafe && len) {
 		struct text_stat new_stats;
 		memcpy(&new_stats, &stats, sizeof(new_stats));
-- 
2.10.0

Re: [PATCH v1 1/1] convert: git cherry-pick -Xrenormalize did not work

[Junio C Hamano]

tboegi@web.de writes:

> From: Torsten Bögershausen <tboegi@web.de>
>
> Working with a repo that used to be all CRLF. At some point it
> was changed to all LF, with `text=auto` in .gitattributes.
> Trying to cherry-pick a commit from before the switchover fails:
>
> $ git cherry-pick -Xrenormalize <commit>
>     fatal: CRLF would be replaced by LF in [path]

OK.  That's a very clear description of the symptom that can be
observed from the surface.

> Whenever crlf_action is CRLF_TEXT_XXX and not CRLF_AUTO_XXX,
> SAFE_CRLF_RENORMALIZE must be turned into CRLF_SAFE_FALSE.

Aside from needing s/CRLF_SAFE/SAFE_CRLF/, this however lacks
"Otherwise, because of X and Y, Z ends up doing W" to explain
the "must be" part.  Care to explain it a bit more?

Thanks.

> Reported-by: Eevee (Lexy Munroe) <eevee@veekun.com>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
>
> Thanks for reporting.
> Here is a less invasive patch.
> Please let me know, if the patch is OK for you
> (email address, does it work..)
>
>  convert.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/convert.c b/convert.c
> index be91358..526ec1d 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -286,7 +286,9 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
>  			checksafe = SAFE_CRLF_FALSE;
>  		else if (has_cr_in_index(path))
>  			convert_crlf_into_lf = 0;
> -	}
> +	} else if (checksafe == SAFE_CRLF_RENORMALIZE)
> +		checksafe = SAFE_CRLF_FALSE;
> +
>  	if (checksafe && len) {
>  		struct text_stat new_stats;
>  		memcpy(&new_stats, &stats, sizeof(new_stats));

Re: [PATCH v1 1/1] convert: git cherry-pick -Xrenormalize did not work

[Torsten Bögershausen]

On Tue, Nov 29, 2016 at 10:42:18AM -0800, Junio C Hamano wrote:
> tboegi@web.de writes:
> 
> > From: Torsten Bögershausen <tboegi@web.de>
> >
> > Working with a repo that used to be all CRLF. At some point it
> > was changed to all LF, with `text=auto` in .gitattributes.
> > Trying to cherry-pick a commit from before the switchover fails:
> >
> > $ git cherry-pick -Xrenormalize <commit>
> >     fatal: CRLF would be replaced by LF in [path]
> 
> OK.  That's a very clear description of the symptom that can be
> observed from the surface.
> 
> > Whenever crlf_action is CRLF_TEXT_XXX and not CRLF_AUTO_XXX,
> > SAFE_CRLF_RENORMALIZE must be turned into CRLF_SAFE_FALSE.
> 
> Aside from needing s/CRLF_SAFE/SAFE_CRLF/, this however lacks
> "Otherwise, because of X and Y, Z ends up doing W" to explain
> the "must be" part.  Care to explain it a bit more?

Thanks for the review - how about this:




convert: git cherry-pick -Xrenormalize did not work

Working with a repo that used to be all CRLF. At some point it
was changed to all LF, with `text=auto` in .gitattributes.
Trying to cherry-pick a commit from before the switchover fails:

$ git cherry-pick -Xrenormalize <commit>
    fatal: CRLF would be replaced by LF in [path]

Commit 65237284 "unify the "auto" handling of CRLF" introduced
a regression:

Whenever crlf_action is CRLF_TEXT_XXX and not CRLF_AUTO_XXX,
SAFE_CRLF_RENORMALIZE was feed into check_safe_crlf().
This is wrong because here everything else than SAFE_CRLF_WARN is
treated as SAFE_CRLF_FAIL.

Solution: Turn SAFE_CRLF_RENORMALIZE into SAFE_CRLF_FALSE before
calling check_safe_crlf().

Reported-by: Eevee (Lexy Munroe) <eevee@veekun.com>
Signed-off-by: Torsten Bögershausen <tboegi@web.de>

Re: [PATCH v1 1/1] convert: git cherry-pick -Xrenormalize did not work

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> Thanks for the review - how about this:
>
>
> convert: git cherry-pick -Xrenormalize did not work
>
> Working with a repo that used to be all CRLF. At some point it
> was changed to all LF, with `text=auto` in .gitattributes.
> Trying to cherry-pick a commit from before the switchover fails:
>
> $ git cherry-pick -Xrenormalize <commit>
>     fatal: CRLF would be replaced by LF in [path]
>
> Commit 65237284 "unify the "auto" handling of CRLF" introduced
> a regression:
>
> Whenever crlf_action is CRLF_TEXT_XXX and not CRLF_AUTO_XXX,
> SAFE_CRLF_RENORMALIZE was feed into check_safe_crlf().
> This is wrong because here everything else than SAFE_CRLF_WARN is
> treated as SAFE_CRLF_FAIL.

What is still left unsaid is that we shouldn't even bother seeing if
it is safe to do crlf conversion when renormalizing.  Perhaps that
is too obvious to state?

In any case, when you put the rationale that way, the impression I
get from it is that the root cause of the problem is that "here"
(aka "check_safe_crlf()") considers anything other than CRLF_WARN as
a failure, when a newer choice other than CRLF_WARN and CRLF_FAIL
(namely, CRLF_RENORMALIZE) exists.  Which hints me that a sensible
change may be to fix that function.

The patch you sent has the effect of not entering this whole block,
not just "don't call check_safe_crlf() because it misbehaves":

	if (checksafe && len) {
		struct text_stat new_stats;
		memcpy(&new_stats, &stats, sizeof(new_stats));
		/* simulate "git add" */
		if (convert_crlf_into_lf) {
			new_stats.lonelf += new_stats.crlf;
			new_stats.crlf = 0;
		}
		/* simulate "git checkout" */
		if (will_convert_lf_to_crlf(len, &new_stats, crlf_action)) {
			new_stats.crlf += new_stats.lonelf;
			new_stats.lonelf = 0;
		}
		check_safe_crlf(path, crlf_action, &stats, &new_stats, checksafe);
	}

And it is a sensible thing to do, because all the computation that
happens in the block before check_safe_crlf() is called is done ONLY
to prepare the parameter passed to check_safe_crlf(); if we are to
make the function no-op for CRLF_RENORMALIZE, preparing new_stats is
a wasted effort.

However, futzing with the value of checksafe in the function is
ugly.  It is not even unclear if it is safe to do so without reading
the remainder of the function (i.e. later parts of the function may
care--or start caring in the future--what the caller passed in the
variable).  Yes, the function already modifies the variable, but
that can also be fixed.

In other words, I would have expected that a fix that matches your
description to look more like below.  The condition for the "if"
statement may even want to become

	if ((checksafe == SAFE_CRLF_WARN ||
	    (checksafe == SAFE_CRLF_FAIL)) && len)

to clarify it further.

Thanks.

 convert.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/convert.c b/convert.c
index f9f7f5e436..d72e0bf0d7 100644
--- a/convert.c
+++ b/convert.c
@@ -282,12 +282,11 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
 		 * If the file in the index has any CR in it, do not convert.
 		 * This is the new safer autocrlf handling.
 		 */
-		if (checksafe == SAFE_CRLF_RENORMALIZE)
-			checksafe = SAFE_CRLF_FALSE;
-		else if (has_cr_in_index(path))
+		if (checksafe != SAFE_CRLF_RENORMALIZE &&
+		    has_cr_in_index(path))
 			convert_crlf_into_lf = 0;
 	}
-	if (checksafe && len) {
+	if (checksafe && checksafe != SAFE_CRLF_RENORMALIZE && len) {
 		struct text_stat new_stats;
 		memcpy(&new_stats, &stats, sizeof(new_stats));
 		/* simulate "git add" */

[PATCH v2 1/1] convert: git cherry-pick -Xrenormalize did not work

[tboegi]

From: Torsten Bögershausen <tboegi@web.de>

Working with a repo that used to be all CRLF. At some point it
was changed to all LF, with `text=auto` in .gitattributes.
Trying to cherry-pick a commit from before the switchover fails:

$ git cherry-pick -Xrenormalize <commit>
    fatal: CRLF would be replaced by LF in [path]

Commit 65237284 "unify the "auto" handling of CRLF" introduced
a regression:

Whenever crlf_action is CRLF_TEXT_XXX and not CRLF_AUTO_XXX,
SAFE_CRLF_RENORMALIZE was feed into check_safe_crlf().
This is wrong because here everything else than SAFE_CRLF_WARN is
treated as SAFE_CRLF_FAIL.

Call check_safe_crlf() only if checksafe is SAFE_CRLF_WARN or SAFE_CRLF_FAIL.

Reported-by: Eevee (Lexy Munroe) <eevee@veekun.com>
Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
 convert.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/convert.c b/convert.c
index be91358..f8e4dfe 100644
--- a/convert.c
+++ b/convert.c
@@ -281,13 +281,13 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
 		/*
 		 * If the file in the index has any CR in it, do not convert.
 		 * This is the new safer autocrlf handling.
+		   - unless we want to renormalize in a merge or cherry-pick
 		 */
-		if (checksafe == SAFE_CRLF_RENORMALIZE)
-			checksafe = SAFE_CRLF_FALSE;
-		else if (has_cr_in_index(path))
+		if ((checksafe != SAFE_CRLF_RENORMALIZE) && has_cr_in_index(path))
 			convert_crlf_into_lf = 0;
 	}
-	if (checksafe && len) {
+	if ((checksafe == SAFE_CRLF_WARN ||
+	    (checksafe == SAFE_CRLF_FAIL)) && len) {
 		struct text_stat new_stats;
 		memcpy(&new_stats, &stats, sizeof(new_stats));
 		/* simulate "git add" */
-- 
2.10.0

Re: [PATCH v2 1/1] convert: git cherry-pick -Xrenormalize did not work

[Junio C Hamano]

tboegi@web.de writes:

> From: Torsten Bögershausen <tboegi@web.de>
>
> Working with a repo that used to be all CRLF. At some point it
> was changed to all LF, with `text=auto` in .gitattributes.
> Trying to cherry-pick a commit from before the switchover fails:
>
> $ git cherry-pick -Xrenormalize <commit>
>     fatal: CRLF would be replaced by LF in [path]
>
> Commit 65237284 "unify the "auto" handling of CRLF" introduced
> a regression:
>
> Whenever crlf_action is CRLF_TEXT_XXX and not CRLF_AUTO_XXX,
> SAFE_CRLF_RENORMALIZE was feed into check_safe_crlf().
> This is wrong because here everything else than SAFE_CRLF_WARN is
> treated as SAFE_CRLF_FAIL.
>
> Call check_safe_crlf() only if checksafe is SAFE_CRLF_WARN or SAFE_CRLF_FAIL.
>
> Reported-by: Eevee (Lexy Munroe) <eevee@veekun.com>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---

I think the description and the code match with each other much
better and the resulting code explains what is going on more
clearly.  Thanks---will queue.

>  convert.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/convert.c b/convert.c
> index be91358..f8e4dfe 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -281,13 +281,13 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
>  		/*
>  		 * If the file in the index has any CR in it, do not convert.
>  		 * This is the new safer autocrlf handling.
> +		   - unless we want to renormalize in a merge or cherry-pick
>  		 */
> -		if (checksafe == SAFE_CRLF_RENORMALIZE)
> -			checksafe = SAFE_CRLF_FALSE;
> -		else if (has_cr_in_index(path))
> +		if ((checksafe != SAFE_CRLF_RENORMALIZE) && has_cr_in_index(path))
>  			convert_crlf_into_lf = 0;
>  	}
> -	if (checksafe && len) {
> +	if ((checksafe == SAFE_CRLF_WARN ||
> +	    (checksafe == SAFE_CRLF_FAIL)) && len) {
>  		struct text_stat new_stats;
>  		memcpy(&new_stats, &stats, sizeof(new_stats));
>  		/* simulate "git add" */

Re: [PATCH v2 1/1] convert: git cherry-pick -Xrenormalize did not work

[Jacob Keller]

On Wed, Nov 30, 2016 at 9:02 AM,  <tboegi@web.de> wrote:
> From: Torsten Bögershausen <tboegi@web.de>
> diff --git a/convert.c b/convert.c
> index be91358..f8e4dfe 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -281,13 +281,13 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
>                 /*
>                  * If the file in the index has any CR in it, do not convert.
>                  * This is the new safer autocrlf handling.
> +                  - unless we want to renormalize in a merge or cherry-pick

Style nit, usually this line should begin with an aligned *? I think
it's not really that big a deal, though.

Thanks,
Jake

Re: [PATCH v2 1/1] convert: git cherry-pick -Xrenormalize did not work

[Junio C Hamano]

Jacob Keller <jacob.keller@gmail.com> writes:

> On Wed, Nov 30, 2016 at 9:02 AM,  <tboegi@web.de> wrote:
>> From: Torsten Bögershausen <tboegi@web.de>
>> diff --git a/convert.c b/convert.c
>> index be91358..f8e4dfe 100644
>> --- a/convert.c
>> +++ b/convert.c
>> @@ -281,13 +281,13 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
>>                 /*
>>                  * If the file in the index has any CR in it, do not convert.
>>                  * This is the new safer autocrlf handling.
>> +                  - unless we want to renormalize in a merge or cherry-pick
>
> Style nit, usually this line should begin with an aligned *? I think
> it's not really that big a deal, though.

Yup, this is what I queued.

-- >8 --
From: Torsten Bögershausen <tboegi@web.de>
Date: Wed, 30 Nov 2016 18:02:32 +0100
Subject: [PATCH] convert: git cherry-pick -Xrenormalize did not work

Working with a repo that used to be all CRLF. At some point it
was changed to all LF, with `text=auto` in .gitattributes.
Trying to cherry-pick a commit from before the switchover fails:

    $ git cherry-pick -Xrenormalize <commit>
    fatal: CRLF would be replaced by LF in [path]

Commit 65237284 "unify the "auto" handling of CRLF" introduced
a regression:

Whenever crlf_action is CRLF_TEXT_XXX and not CRLF_AUTO_XXX,
SAFE_CRLF_RENORMALIZE was feed into check_safe_crlf().  This is
wrong because here everything else than SAFE_CRLF_WARN is treated as
SAFE_CRLF_FAIL.

Call check_safe_crlf() only if checksafe is SAFE_CRLF_WARN or
SAFE_CRLF_FAIL.

Reported-by: Eevee (Lexy Munroe) <eevee@veekun.com>
Signed-off-by: Torsten Bögershausen <tboegi@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 convert.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/convert.c b/convert.c
index 077f5e601e..2f90f363c6 100644
--- a/convert.c
+++ b/convert.c
@@ -274,15 +274,16 @@ static int crlf_to_git(const char *path, const char *src, size_t len,
 		if (convert_is_binary(len, &stats))
 			return 0;
 		/*
-		 * If the file in the index has any CR in it, do not convert.
-		 * This is the new safer autocrlf handling.
+		 * If the file in the index has any CR in it, do not
+		 * convert.  This is the new safer autocrlf handling,
+		 * unless we want to renormalize in a merge or
+		 * cherry-pick.
 		 */
-		if (checksafe == SAFE_CRLF_RENORMALIZE)
-			checksafe = SAFE_CRLF_FALSE;
-		else if (has_cr_in_index(path))
+		if ((checksafe != SAFE_CRLF_RENORMALIZE) && has_cr_in_index(path))
 			convert_crlf_into_lf = 0;
 	}
-	if (checksafe && len) {
+	if ((checksafe == SAFE_CRLF_WARN ||
+	    (checksafe == SAFE_CRLF_FAIL)) && len) {
 		struct text_stat new_stats;
 		memcpy(&new_stats, &stats, sizeof(new_stats));
 		/* simulate "git add" */
-- 
2.11.0-192-gbadfaabe38

Re: [PATCH v2 1/1] convert: git cherry-pick -Xrenormalize did not work

[Torsten Bögershausen]

> Yup, this is what I queued.
>
Looks good, thanks you all.