@matheusbsbs / matheus-bagliano / commits / c24f3abace

apply: file commited with CRLF should roundtrip diff and apply

When a file had been commited with CRLF but now .gitattributes say "* text=auto" (or core.autocrlf is true), the following does not roundtrip, `git apply` fails: printf "Added line\r\n" >>file && git diff >patch && git checkout -- . && git apply patch Before applying the patch, the file from working tree is converted into the index format (clean filter, CRLF conversion, ...). Here, when commited with CRLF, the line endings should not be converted. Note that `git apply --index` or `git apply --cache` doesn't call convert_to_git() because the source material is already in index format. Analyze the patch if there is a) any context line with CRLF, or b) if any line with CRLF is to be removed. In this case the patch file `patch` has mixed line endings, for a) it looks like this: diff --git a/one b/one index 533790e..c30dea8 100644 --- a/one +++ b/one @@ -1 +1,2 @@ a\r +b\r And for b) it looks like this: diff --git a/one b/one index 533790e..485540d 100644 --- a/one +++ b/one @@ -1 +1 @@ -a\r +b\r If `git apply` detects that the patch itself has CRLF, (look at the line " a\r" or "-a\r" above), the new flag crlf_in_old is set in "struct patch" and two things will happen: - read_old_data() will not convert CRLF into LF by calling convert_to_git(..., SAFE_CRLF_KEEP_CRLF); - The WS_CR_AT_EOL bit is set in the "white space rule", CRLF are no longer treated as white space. While at there, make it clear that read_old_data() in apply.c knows what it wants convert_to_git() to do with respect to CRLF. In fact, this codepath is about applying a patch to a file in the filesystem, which may not exist in the index, or may exist but may not match what is recorded in the index, or in the extreme case, we may not even be in a Git repository. If convert_to_git() peeked at the index while doing its work, it *would* be a bug. Pass NULL instead of &the_index to convert_to_git() to make sure we catch future bugs to clarify this. Update the test in t4124: split one test case into 3: - Detect the " a\r" line in the patch - Detect the "-a\r" line in the patch - Use LF in repo and CLRF in the worktree. Reported-by: Anthony Sottile <asottile@umich.edu> Signed-off-by: Torsten Bögershausen <tboegi@web.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Torsten Bögershausen committed Aug 19, 2017 at 13:28 UTC c24f3abaceabb590125751a67ec0e32946780ac7

2 files changed +63 -11

apply.c

+36 -5

@@ -220,6 +220,7 @@ struct patch {

220 unsigned int recount:1;

221 unsigned int conflicted_threeway:1;

222 unsigned int direct_to_threeway:1;

223 + unsigned int crlf_in_old:1;

224 struct fragment *fragments;

225 char *result;

226 size_t resultsize;

@@ -1662,6 +1663,19 @@ static void check_whitespace(struct apply_state *state,

1663 record_ws_error(state, result, line + 1, len - 2, state->linenr);

1664 }

1665

1666 +/*

1667 + * Check if the patch has context lines with CRLF or

1668 + * the patch wants to remove lines with CRLF.

1669 + */

1670 +static void check_old_for_crlf(struct patch *patch, const char *line, int len)

1671 +{

1672 + if (len >= 2 && line[len-1] == '\n' && line[len-2] == '\r') {

1673 + patch->ws_rule |= WS_CR_AT_EOL;

1674 + patch->crlf_in_old = 1;

1675 + }

1676 +}

1677 +

1678 +

1679 /*

1680 * Parse a unified diff. Note that this really needs to parse each

1681 * fragment separately, since the only way to know the difference

@@ -1712,11 +1726,14 @@ static int parse_fragment(struct apply_state *state,

1726 if (!deleted && !added)

1727 leading++;

1728 trailing++;

1729 + check_old_for_crlf(patch, line, len);

1730 if (!state->apply_in_reverse &&

1731 state->ws_error_action == correct_ws_error)

1732 check_whitespace(state, line, len, patch->ws_rule);

1733 break;

1734 case '-':

1735 + if (!state->apply_in_reverse)

1736 + check_old_for_crlf(patch, line, len);

1737 if (state->apply_in_reverse &&

1738 state->ws_error_action != nowarn_ws_error)

1739 check_whitespace(state, line, len, patch->ws_rule);

@@ -1725,6 +1742,8 @@ static int parse_fragment(struct apply_state *state,

1742 trailing = 0;

1743 break;

1744 case '+':

1745 + if (state->apply_in_reverse)

1746 + check_old_for_crlf(patch, line, len);

1747 if (!state->apply_in_reverse &&

1748 state->ws_error_action != nowarn_ws_error)

1749 check_whitespace(state, line, len, patch->ws_rule);

@@ -2268,8 +2287,11 @@ static void show_stats(struct apply_state *state, struct patch *patch)

2287 add, pluses, del, minuses);

2288 }

2289

2271 -static int read_old_data(struct stat *st, const char *path, struct strbuf *buf)

2290 +static int read_old_data(struct stat *st, struct patch *patch,

2291 + const char *path, struct strbuf *buf)

2292 {

2293 + enum safe_crlf safe_crlf = patch->crlf_in_old ?

2294 + SAFE_CRLF_KEEP_CRLF : SAFE_CRLF_RENORMALIZE;

2295 switch (st->st_mode & S_IFMT) {

2296 case S_IFLNK:

2297 if (strbuf_readlink(buf, path, st->st_size) < 0)

@@ -2278,7 +2300,15 @@ static int read_old_data(struct stat *st, const char *path, struct strbuf *buf)

2300 case S_IFREG:

2301 if (strbuf_read_file(buf, path, st->st_size) != st->st_size)

2302 return error(_("unable to open or read %s"), path);

2281 - convert_to_git(&the_index, path, buf->buf, buf->len, buf, 0);

2303 + /*

2304 + * "git apply" without "--index/--cached" should never look

2305 + * at the index; the target file may not have been added to

2306 + * the index yet, and we may not even be in any Git repository.

2307 + * Pass NULL to convert_to_git() to stress this; the function

2308 + * should never look at the index when explicit crlf option

2309 + * is given.

2310 + */

2311 + convert_to_git(NULL, path, buf->buf, buf->len, buf, safe_crlf);

2312 return 0;

2313 default:

2314 return -1;

@@ -3384,6 +3414,7 @@ static int load_patch_target(struct apply_state *state,

3414 struct strbuf *buf,

3415 const struct cache_entry *ce,

3416 struct stat *st,

3417 + struct patch *patch,

3418 const char *name,

3419 unsigned expected_mode)

3420 {

@@ -3399,7 +3430,7 @@ static int load_patch_target(struct apply_state *state,

3430 } else if (has_symlink_leading_path(name, strlen(name))) {

3431 return error(_("reading from '%s' beyond a symbolic link"), name);

3432 } else {

3402 - if (read_old_data(st, name, buf))

3433 + if (read_old_data(st, patch, name, buf))

3434 return error(_("failed to read %s"), name);

3435 }

3436 }

@@ -3432,7 +3463,7 @@ static int load_preimage(struct apply_state *state,

3463 /* We have a patched copy in memory; use that. */

3464 strbuf_add(&buf, previous->result, previous->resultsize);

3465 } else {

3435 - status = load_patch_target(state, &buf, ce, st,

3466 + status = load_patch_target(state, &buf, ce, st, patch,

3467 patch->old_name, patch->old_mode);

3468 if (status < 0)

3469 return status;

@@ -3520,7 +3551,7 @@ static int load_current(struct apply_state *state,

3551 if (verify_index_match(ce, &st))

3552 return error(_("%s: does not match index"), name);

3553

3523 - status = load_patch_target(state, &buf, ce, &st, name, mode);

3554 + status = load_patch_target(state, &buf, ce, &st, patch, name, mode);

3555 if (status < 0)

3556 return status;

3557 else if (status)

t/t4124-apply-ws-rule.sh

+27 -6

@@ -467,21 +467,42 @@ test_expect_success 'same, but with CR-LF line endings && cr-at-eol set' '

467 test_cmp one expect

468 '

469

470 -test_expect_success 'same, but with CR-LF line endings && cr-at-eol unset' '

470 +test_expect_success 'CR-LF line endings && add line && text=auto' '

471 git config --unset core.whitespace &&

472 printf "a\r\n" >one &&

473 + cp one save-one &&

474 + git add one &&

475 printf "b\r\n" >>one &&

474 - printf "c\r\n" >>one &&

476 + cp one expect &&

477 + git diff -- one >patch &&

478 + mv save-one one &&

479 + echo "one text=auto" >.gitattributes &&

480 + git apply patch &&

481 + test_cmp one expect

482 +'

483 +

484 +test_expect_success 'CR-LF line endings && change line && text=auto' '

485 + printf "a\r\n" >one &&

486 cp one save-one &&

476 - printf " \r\n" >>one &&

487 git add one &&

488 + printf "b\r\n" >one &&

489 cp one expect &&

479 - printf "d\r\n" >>one &&

490 git diff -- one >patch &&

491 mv save-one one &&

482 - echo d >>expect &&

492 + echo "one text=auto" >.gitattributes &&

493 + git apply patch &&

494 + test_cmp one expect

495 +'

496

484 - git apply --ignore-space-change --whitespace=fix patch &&

497 +test_expect_success 'LF in repo, CRLF in worktree && change line && text=auto' '

498 + printf "a\n" >one &&

499 + git add one &&

500 + printf "b\r\n" >one &&

501 + git diff -- one >patch &&

502 + printf "a\r\n" >one &&

503 + echo "one text=auto" >.gitattributes &&

504 + git -c core.eol=CRLF apply patch &&

505 + printf "b\r\n" >expect &&

506 test_cmp one expect

507 '

508