Re: [PATCH v1 1/1] correct apply for files commited with CRLF
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 2, 2017, 21:13 UTC
- Message-ID
- <xmqqwp6lr7u0.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20170802204203.29484-1-tboegi@web.de>
tboegi@web.de writes:
Show 13 quoted lines
> 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.
Show 42 quoted lines
>
> Reported-by: Anthony Sottile <asottile@umich.edu>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
> apply.c | 2 ++
> t/t0020-crlf.sh | 12 ++++++++++++
> 2 files changed, 14 insertions(+)
>
> diff --git a/apply.c b/apply.c
> index f2d599141d..66b8387360 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -2278,6 +2278,8 @@ static int read_old_data(struct stat *st, const char *path, struct strbuf *buf)
> case S_IFREG:
> if (strbuf_read_file(buf, path, st->st_size) != st->st_size)
> return error(_("unable to open or read %s"), path);
> + if (would_convert_to_git(&the_index, path))
> + read_cache();
> convert_to_git(&the_index, path, buf->buf, buf->len, buf, 0);
> return 0;
> default:
> diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh
> index 71350e0657..6611f8a6f6 100755
> --- a/t/t0020-crlf.sh
> +++ b/t/t0020-crlf.sh
> @@ -386,4 +386,16 @@ test_expect_success 'New CRLF file gets LF in repo' '
> test_cmp alllf alllf2
> '
>
> +test_expect_success 'CRLF in repo, apply with autocrlf=true' '
> + git config core.autocrlf false &&
> + printf "1\r\n2\r\n" >crlf &&
> + git add crlf &&
> + git commit -m "commit crlf with crlf" &&
> + git config core.autocrlf true &&
> + printf "1\r\n2\r\n\r\n\r\n\r\n" >crlf &&
> + git diff >patch &&
> + git checkout -- . &&
> + git apply patch
> +'
> +
> test_done