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

tboegi@web.de writes:

From: Torsten Bögershausen <redacted>

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.

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

tboegi@web.de writes:

quoted

From: Torsten Bögershausen <redacted>

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;
}