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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 29, 2016, 20:45 UTC
Message-ID: <xmqqzikidnhh.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20161129201600.GA14333@tb-raspi>

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

Show 19 quoted lines

> 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" */