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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 7, 2016, 22:19 UTC
Message-ID: <xmqqd1mpnl9v.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <xmqqr3b5p9v0.fsf@gitster.mtv.corp.google.com>

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

Show 18 quoted lines

> I am however not convinced passing the full SHA-1 is a good
> solution.  The make_cache_entry() function may be creating a cache
> entry to stuff in a non-default index (i.e. not "the_index"), but
> its caller does not have a way to tell it which index the resulting
> cache entry will go, and calls refresh_cache_entry(), which always
> assumes that the caller is interested in "the_index", so what
> add_cacheinfo() does by calling make_cache_entry() feels wrong in
> the first place.  Also, the call from update_file_flags() knows that
> the working tree is in sync with the resulting cache entry.
>
> Perhaps update_file_flags() should not even call add_cacheinfo()
> there in the first place?  I wonder if it should just call
> add_file_to_index()---after all, even though the current code
> already knows that 99b633 _is_ the result it wants in the index, it
> still goes to the filesystem and rehashes.
>
> I dunno.  I really do not like that extra sha1 argument added all
> over the callchain by this patch.

While the above still stands (i.e. I really want to see us find a solution without the extra argument all over), I do not think add_file_to_index() would work well here, as the stage #2 of the index is contaminated with CRLF in this case, and getting rid of it is the whole point of renormalization, I would think. Of course, you will get a normalized result if you merged in the other direction, as your stage #2 would have normalized 99b633 when you cleanly merge the CRLF version from the other side with renormalization, and "git add" of the cleanly merged result would keep the normalized version.

Perhaps that is an acceptable downside. If your side is not normalized, and if the merge result is the same as what you already have, perhaps you deserve to keep that unnormalized result, and you'd be better off normalizing your tree before doing a merge, if you do not like the fact that your indexed blobs have CRLF.

> Or did you mean other calls to add_cacheinfo()?