Re: [PATCH v7 2/2] convert: ce_compare_data() checks for a sha1 of a path

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

tboegi@web.de writes:

> From: Torsten Bögershausen <tboegi@web.de>
>
> To compare a file in working tree with the index, convert_to_git() is used,
> the result is hashed and the hash value compared with ce->sha1.
>
> Deep down would_convert_crlf_at_commit() is invoked, to check if CRLF
> are converted or not.
> The "new safer autocrlf handling" checks if CRLF had been in the index before,
> and if, the CRLF in the working tree are not converted.
>
> While in a merge, a file name in the working tree has different blobs
> in the index with different hash values.
> Forwarding ce->sha1 from ce_compare_data() into crlf_to_git() makes sure
> the would_convert_crlf_at_commit() looks at the appropriate blob.
>
> Add a new parameter index_blob_sha1 to convert_to_git(), and forward the
> sha1 from ce_compare_data() into convert_to_git(). Other callers use NULL
> for index_blob_sha1, and the sha1 is determined from path
> using get_sha1_from_cache(path). This is the same handling as before.
>
> In the same spirit, forward the sha1 into would_convert_to_git().
>
> While at it, rename has_cr_in_index() into blob_has_cr() and replace
> 0 with SAFE_CRLF_FALSE.
>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---

Assuming that it is sensible to pass an extra "no, no, do not look
at the index entry at path, but look at this blob to see if the CRLF
conversion must be disabled" parameter all the down the callchain,
this round looks good.

I really hate to say this this late in the reroll cycle, but this
part of the description makes me wonder:

    > While in a merge, a file name in the working tree has
    > different blobs in the index with different hash values.
    > Forwarding ce->sha1 from ce_compare_data() into crlf_to_git()
    > makes sure the would_convert_crlf_at_commit() looks at the
    > appropriate blob.

When we need content-level merges with help from the end user, we
would need to "convert-to-git" the result of conflict resolution by
the user left in the working tree, and that convert-to-git needs to
take our original contents into account, i.e. "did we have CR in the
blob already? if so, disable the CRLF thing".

But we would always have "our" original contents at stage #2 in the
index in such a case, and would_convert_to_git() eventually calls
into read_blob_data_from_cache(), which knows to read from stage #2

Even if we were in a renaming merge conflict, where they renamed
file F to G while we kept file F as file F, the conflicted working
tree file will be made in path G, and stage #2 of the index for path
G would have our original contents we had at path F.

So it is not clear why this "no, no, look at this blob instead" is
necessary in the first place.  What problem does this solve?  Is
this a fix for something that can be easily demonstrated with a new
test case?