Re: [PATCH v3 3/3] correct ce_compare_data() in a middle of a merge

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 1, 2016, 22:11 UTC
Message-ID: <xmqq8txlvwip.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <62eb3d75-126e-427b-8701-d490e80e3501@web.de>

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

Show 14 quoted lines

>> The checkout process hopefully does not blindly turn LF into CRLF,
>> making it "first line \r\r\nsame line\r\rn".  Instead the (virtual)
>> working tree file will have "first line\r\nsame line\r\n".
> Yes
>> 
>> Then "git add" should turn that CRLF into LF, which would give us
>> "first line\nsame line\n", but because the "safer autocrlf" rule
>> prevents it from happening.  The (real) index already has CR in it
>> in the blob in stage #2, so the check-in process would (this is not
>> describing the ideal, but what is done by today's code) disable
>> CRLF->LF conversion.
>> 
>> Is that the problem you are trying to solve?
> Not sure, if that problem isn't already solved.

OK, so in short, renormalizing three blobs in the conflicted index is done correcte because you do not allow safer-crlf kicks in during the renormalization?

So how does ce_compare_data() judge "first line\r\nsame line\r\n" in stage #2 and working tree (both with CRLF) not to match? The CRLF in working tree data, when run convert_to_git(), will be kept as CRLF because of safer-crlf rule, and would match, no? Or do you disable safer-crlf for that case, too?

> This is my understanding:
> - git checkout and git add are working as expected:
>   LF in blob gives CRLF in the working tree at checkout (if attr says "auto")
>   CRLF in blob gives CRLF in the working tree at checkout.

OK.

>   CRLF in the working tree gives CRLF in the blob at "git add", when
>   the new safer CRLF handling says so.

OK.

> - Merges
>  First the working tree is checked, if it is clean.
>  I haven't digged deep enough to follow the whole code path,
>  but that is what ce_compare_data() does, and it fails in a merge.

OK, I misunderstood what you are trying to fix. I somehow thought that you wanted to tweak the renormalization of three blobs.

Show 9 quoted lines

>  Next thing is that the blobs are merged, and when a blob with CRLF
>  is merged with a blob with LF, there are only conflicts :-(
>  (Remember that both may have CRLF in the worktree)
>  And here renormalize_buffer() comes in (only when "merge.renormalize" is true):
>  - convert the blob with convert_to_working_tree_internal() into the working
>     tree format (but do it in memory), and back to the "blob":
>  - convert_to_git(path, src, len, dst, SAFE_CRLF_RENORMALIZE, NULL);
>
>  Note the the "new safer autocrlf" is suppressed here, because of SAFE_CRLF_RENORMALIZE.

That makes sense.

> - check for a clean worktree in ce_compare_data() # fails -> this is the problem to be solved

But it is unclear to me how that need an extra "no, do not look at the indexed blob, but look at this blob instead"?

The relevant part is this:

diff --git a/read-cache.c b/read-cache.c
index a3ef967..c109b6d 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -163,7 +163,9 @@ static int ce_compare_data(const struct cache_entry *ce, struct stat *st)
 
 	if (fd >= 0) {
 		unsigned char sha1[20];
-		if (!index_fd(sha1, fd, st, OBJ_BLOB, ce->name, 0))
+		unsigned flags = HASH_USE_SHA_NOT_PATH;
+		memcpy(sha1, ce->sha1, sizeof(sha1));
+		if (!index_fd(sha1, fd, st, OBJ_BLOB, ce->name, flags))
 			match = hashcmp(sha1, ce->sha1);
 		/* index_fd() closed the file descriptor already */
 	}

I do not quite see how "Don't use ce->sha1, but use sha1 given to
you" flag affects this call, when sha1 and ce->sha1 are the same due
to memcpy() just before the call?