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

[Junio C Hamano <gitster@pobox.com>]

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

>> I dunno.  I really do not like that extra sha1 argument added all
>> over the callchain by this patch.
>>
>> Or did you mean other calls to add_cacheinfo()?
>
> I didn't mean too much - the whole call chain touches code where I
> am not able to comment on details.
> I'm happy to test other implementations, if someone suggests a
> path, so to say.

I did a bit of experiment.

When 1/3 alone is applied, and then only changes for t/t6038 from
3/3 is picked, (i.e. we do not add the extra "don't look at index,
check this contents"), your "Merge addition of text=auto eol=CRLF"
test would fail.

And then with this further on top:

diff --git a/merge-recursive.c b/merge-recursive.c
index b880ae5..628c8ed 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -202,6 +202,9 @@ static int add_cacheinfo(unsigned int mode, const unsigned char *sha1,
 		const char *path, int stage, int refresh, int options)
 {
 	struct cache_entry *ce;
+
+	if (!stage)
+		remove_file_from_cache(path);
 	ce = make_cache_entry(mode, sha1 ? sha1 : null_sha1, path, stage,
 			      (refresh ? (CE_MATCH_REFRESH |
 					  CE_MATCH_IGNORE_MISSING) : 0 ));

I get the renomalized result registered to the index.

The root cause of the "bug", I think, is that the "safer crlf" is
fundamentally a flawed mechanism and does not mesh well with the
rest of the system.  If you start from an index without the path and
add a CRLF file in the working tree, you get LF blob (as expected),
but if you happen to already have CRLF file for the path in the
index, you get CRLF blob (as claimed to be "safer"), which
essentially means that with that mechanism in place, you do not know
blob with what object name you would get, given the same working
tree file with the same configuration setting.

In this codepath, we already have three stages in the index, and the
stage #2 happens to be a CRLF blob in your test.  make_cache_entry()
wants to refresh, which involves comparing the working tree file to
see if it hashes down to the same object name as ce->sha1, which is
the result of the "normalizing" merge that uses LF.  But if you
merge the other way, the stage #2 would have LF blob, and that would
not prevent the working tree file with CRLF written from the result
of the "normalizing" merge to be cleaned again to LF blob to
round-trip.

The "this is a minimum workaround" patch above disables the "safer
crlf" conversion by removing the stages that will get in the way.
When we are recording the cleanly resolved result of a merge, we
know what the resulting ce->sha1 should be, and we also know the
higher stages for the path will be removed, so there is no point
keeping the higher stages in the index and get them looked at by the
"safer crlf" codepath.