Re: [PATCH v1 1/1] correct apply for files commited with CRLF

From: Torsten Bögershausen <tboegi@web.de>
Date: Aug 4, 2017, 17:31 UTC
Message-ID: <191dc9ea-6ad3-f754-59da-2075a6fd581e@web.de>
In-Reply-To: <xmqqwp6lr7u0.fsf@gitster.mtv.corp.google.com>

On 08/02/2017 11:13 PM, Junio C Hamano wrote:

Show 33 quoted lines

> tboegi@web.de writes:
>
>> From: Torsten Bögershausen <tboegi@web.de>
>>
>> git apply does not find the source lines when files have CRLF in the index
>> and core.autocrlf is true:
>> These files should not get the CRLF converted to LF. Because cmd_apply()
>> does not load the index, this does not work, CRLF are converted into LF
>> and apply fails.
>>
>> Fix this in the spirit of commit a08feb8ef0b6,
>> "correct blame for files commited with CRLF" by loading the index.
>>
>> As an optimization, skip read_cache() when no conversion is specified
>> for this path.
> What happens when the input to apply is a patch to create a new file
> and the patch uses CRLF line endings?  In such a case, shouldn't
> core.autocrlf=true cause the patch to be converted to LF and then
> succeed in applying?  An extra read_cache() would not help as there
> must be no existing index entry for the path in such a case.
>
> If you did "rm .git/index" after you did the "git checkout -- ." in
> your test patch, "git apply" ought to succeed, as it is working as
> a substitute for "patch -p1" and should not be consulting the index
> at all.
>
> I cannot shake this feeling that it is convert_to_git() that needs
> to be fixed so that it does not need to refer to the current
> contents in the index.  Why does it even need to look at the index?
> If it is for the "safe CRLF" thing (which I would think is misguided),
> shouldn't it be checking the original file in the working tree, not
> the index?  After all, that is what we are patching, not the contents
> stored in the index.

Thanks for the review. My understanding is that there are different things possible with `git apply`: working on files in the index ("cached") files and "worktree" files.

If we work on files in the index, the line endings must match for the patch to apply, the patch/apply machinery is not forgiving mismatching line endings. At least not by default. There is one exception: Use "blank-at-eol" to declare the CR of the CRLF as a whitespace, and then tell git apply to ignore white space: `git apply --ignore-whitespace` My feeling is that this is not self-explaining, but this is a different story.

Back to the fix, the read_old_data() from below works on the working tree, yes, but after convert_to_git(). And that is why we need the index, to fix this very case.

appy.c:
static int load_patch_target(struct apply_state *state,
                  struct strbuf *buf,
                  const struct cache_entry *ce,
                  struct stat *st,
                  const char *name,
                  unsigned expected_mode)
{
     if (state->cached || state->check_index) {
         if (read_file_or_gitlink(ce, buf))
             return error(_("failed to read %s"), name);
     } else if (name) {
         if (S_ISGITLINK(expected_mode)) {
             if (ce)
                 return read_file_or_gitlink(ce, buf);
             else
                 return SUBMODULE_PATCH_WITHOUT_INDEX;
         } else if (has_symlink_leading_path(name, strlen(name))) {
             return error(_("reading from '%s' beyond a symbolic link"), 
name);
         } else {
             if (read_old_data(st, name, buf))
                 return error(_("failed to read %s"), name);
         }
     }
     return 0;
}