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

From: Junio C Hamano <gitster@pobox.com>
Date: May 24, 2016, 18:36 UTC
Message-ID: <xmqqy46z482d.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1463824909-10229-1-git-send-email-tboegi@web.de>

tboegi@web.de writes:

Show 27 quoted lines

> 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?