Jonathan Nieder wrote: > Maybe the space-plus-date-plus-newline sequence should be used as a > delimiter.
And here’s how that would work. Patches are attached, because debbugs seems to handle that best. Still to do: * add tests for ‘-p’ value detection * skip tests that require ‘diff -u’ and ‘pr’ when those commands are not present. Or find some other workaround, like including output from those commands with the patch. Thoughts? Jonathan Nieder (3): apply: Split quoted filename handling into new function tests: Add test for handling of weird filenames apply: Handle traditional patches with space in filename builtin/apply.c | 251 ++++++++++++++++++++++++++++++++------ t/t4135-apply-weird-filenames.sh | 73 +++++++++++ 2 files changed, 285 insertions(+), 39 deletions(-) create mode 100755 t/t4135-apply-weird-filenames.sh
From: Jonathan Nieder <[email protected]> Date: Mon, 5 Jul 2010 19:34:17 -0500 Subject: apply: Split quoted filename handling into new function The new find_name_gnu() function handles new-style ‘--- "a/foo"’ patch header lines, leaving find_name() itself a bit less daunting. Functional change: do not clobber the p-value when there are not enough path components in the detected file name to honor it. Signed-off-by: Jonathan Nieder <[email protected]> --- builtin/apply.c | 70 ++++++++++++++++++++++++++++++------------------------ 1 files changed, 39 insertions(+), 31 deletions(-) diff --git a/builtin/apply.c b/builtin/apply.c index 12ef9ea..efc109e 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -416,44 +416,52 @@ static char *squash_slash(char *name) return name; } +static char *find_name_gnu(const char *line, char *def, int p_value) +{ + struct strbuf name = STRBUF_INIT; + char *cp; + + /* + * Proposed "new-style" GNU patch/diff format; see + * http://marc.theaimsgroup.com/?l=git&m=112927316408690&w=2 + */ + if (unquote_c_style(&name, line, NULL)) { + strbuf_release(&name); + return NULL; + } + + for (cp = name.buf; p_value; p_value--) { + cp = strchr(cp, '/'); + if (!cp) { + strbuf_release(&name); + return NULL; + } + cp++; + } + + /* name can later be freed, so we need + * to memmove, not just return cp + */ + strbuf_remove(&name, 0, cp - name.buf); + free(def); + if (root) + strbuf_insert(&name, 0, root, root_len); + return squash_slash(strbuf_detach(&name, NULL)); +} + static char *find_name(const char *line, char *def, int p_value, int terminate) { int len; const char *start = NULL; + if (*line == '"') { + char *name = find_name_gnu(line, def, p_value); + if (name) + return name; + } + if (p_value == 0) start = line; - - if (*line == '"') { - struct strbuf name = STRBUF_INIT; - - /* - * Proposed "new-style" GNU patch/diff format; see - * http://marc.theaimsgroup.com/?l=git&m=112927316408690&w=2 - */ - if (!unquote_c_style(&name, line, NULL)) { - char *cp; - - for (cp = name.buf; p_value; p_value--) { - cp = strchr(cp, '/'); - if (!cp) - break; - cp++; - } - if (cp) { - /* name can later be freed, so we need - * to memmove, not just return cp - */ - strbuf_remove(&name, 0, cp - name.buf); - free(def); - if (root) - strbuf_insert(&name, 0, root, root_len); - return squash_slash(strbuf_detach(&name, NULL)); - } - } - strbuf_release(&name); - } - for (;;) { char c = *line; -- 1.7.2.rc2
From: Jonathan Nieder <[email protected]> Date: Mon, 12 Jul 2010 19:19:17 -0500 Subject: tests: Add test for handling of weird filenames These tests rely on the local ‘diff’ implementation to produce interesting patches, instead of including our own interesting examples. Portability nightmares: diff -u, pr. Signed-off-by: Jonathan Nieder <[email protected]> --- t/t4135-apply-weird-filenames.sh | 75 ++++++++++++++++++++++++++++++++++++++ 1 files changed, 75 insertions(+), 0 deletions(-) create mode 100755 t/t4135-apply-weird-filenames.sh diff --git a/t/t4135-apply-weird-filenames.sh b/t/t4135-apply-weird-filenames.sh new file mode 100755 index 0000000..c3050a2 --- /dev/null +++ b/t/t4135-apply-weird-filenames.sh @@ -0,0 +1,75 @@ +#!/bin/sh + +test_description='git apply with weird postimage filenames' + +. ./test-lib.sh + +test_expect_success setup ' + test_tick && + git commit --allow-empty -m preimage && + git tag preimage && + + reset_preimage() { + git checkout -f preimage^0 && + git read-tree -u --reset HEAD && + git update-index --refresh + } && + + reset_subdirs() { + rm -fr a b && + mkdir a b + } +' + +try_filename() { + desc=$1 + postimage=$2 + exp1=$3 + exp2=$4 + + test_expect_$exp1 "$desc, git-style file creation patch" " + reset_preimage && + echo postimage >'$postimage' && + git add -N '$postimage' && + git diff HEAD >'git-$desc.diff' && + + git rm -f --cached '$postimage' && + mv '$postimage' postimage.saved && + git apply -v 'git-$desc.diff' && + + test_cmp postimage.saved '$postimage' + " + + test_expect_$exp2 "$desc, traditional file creation patch" " + reset_preimage && + reset_subdirs && + echo postimage >b/'$postimage' && + ! diff -pruN a b >'diff-$desc.diff' && + + mv '$postimage' postimage.saved && + git apply -v 'diff-$desc.diff' && + + test_cmp postimage.saved '$postimage' + " +} + +try_filename 'plain' 'postimage.txt' success success +try_filename 'with spaces' 'post image.txt' success failure +try_filename 'with tab' 'post image.txt' success failure +try_filename 'with backslash' 'post\image.txt' success success +try_filename 'with quote' '"postimage".txt' success success + +test_expect_success 'whitespace-damaged traditional patch' ' + reset_preimage && + reset_subdirs && + echo postimage >b/postimage.txt && + ! diff -pruN a b >diff-plain.txt && + pr -tT -e8 diff-plain.diff >damaged.diff && + + mv postimage.txt postimage.saved && + git apply -v damaged.diff && + + test_cmp postimage.saved postimage.txt +' + +test_done -- 1.7.2.rc2
From: Jonathan Nieder <[email protected]> Date: Mon, 12 Jul 2010 20:01:29 -0500 Subject: apply: Handle traditional patches with space in filename To discover filenames from the --- foo +++ bar lines in a traditional unified diff, currently ‘git apply’ scans forward for a whitespace character on each line and stops there. It can’t use the whole line because ‘diff -u’ likes to include timestamps, like so: --- foo 2000-07-12 16:56:50.020000414 -0500 +++ bar 2010-07-12 16:56:50.020000414 -0500 The whitespace-seeking heuristic works great, even when the tab has been converted to spaces by some email + copy-and-paste related corruption. Except for one problem: if the actual filename contains whitespace, the inferred filename will be too short. When Giuseppe ran into this problem, it was for a file creation patch (filename ‘debian/licenses/LICENSE.global BSD-style Chromium’). So one can’t even use the list of files present in the index to deduce an appropriate filename. Instead, look for a timestamp and use that if present to mark the end of the filename. If no timestamp is present, the old heuristic is used, with one exception: the space character \040 is not considered terminating whitespace any more unless it is followed by a timestamp. Reported-by: Giuseppe Iuculano <[email protected]> Analysis-by: Guido Günther <[email protected]> Signed-off-by: Jonathan Nieder <[email protected]> --- builtin/apply.c | 193 +++++++++++++++++++++++++++++++++++--- t/t4135-apply-weird-filenames.sh | 16 ++-- 2 files changed, 186 insertions(+), 23 deletions(-) diff --git a/builtin/apply.c b/builtin/apply.c index efc109e..4874eca 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -449,23 +449,157 @@ static char *find_name_gnu(const char *line, char *def, int p_value) return squash_slash(strbuf_detach(&name, NULL)); } -static char *find_name(const char *line, char *def, int p_value, int terminate) +static size_t tz_len(const char *line, size_t len) +{ + const char *tz, *p; + + if (len < strlen(" +0500") || line[len-strlen(" +0500")] != ' ') + return 0; + tz = line + len - strlen(" +0500"); + + if (tz[1] != '+' && tz[1] != '-') + return 0; + + for (p = tz + 2; p != line + len; p++) + if (!isdigit(*p)) + return 0; + + return line + len - tz; +} + +static size_t date_len(const char *line, size_t len) +{ + const char *date, *p; + + if (len < strlen("72-02-05") || line[len-strlen("-05")] != '-') + return 0; + p = date = line + len - strlen("72-02-05"); + + if (!isdigit(*p++) || !isdigit(*p++) || *p++ != '-' || + !isdigit(*p++) || !isdigit(*p++) || *p++ != '-' || + !isdigit(*p++) || !isdigit(*p++)) /* Not a date. */ + return 0; + + if (date - line >= strlen("19") && + isdigit(date[-1]) && isdigit(date[-2])) /* 4-digit year */ + date -= strlen("19"); + + return line + len - date; +} + +static size_t whole_time_len(const char *line, size_t len) +{ + const char *time, *p; + + if (len < strlen(" 07:01:32") || line[len-strlen(":32")] != ':') + return 0; + p = time = line + len - strlen(" 07:01:32"); + + /* Permit 1-digit hours? */ + if (*p++ != ' ' || + !isdigit(*p++) || !isdigit(*p++) || *p++ != ':' || + !isdigit(*p++) || !isdigit(*p++) || *p++ != ':' || + !isdigit(*p++) || !isdigit(*p++)) /* Not a time. */ + return 0; + + return line + len - time; +} + +static size_t fractional_time_len(const char *line, size_t len) +{ + const char *p; + size_t n; + + /* Expected format: 19:41:17.620000023 */ + if (!len || !isdigit(line[len - 1])) + return 0; + p = line + len - 1; + + /* Fractional seconds. */ + while (p > line && isdigit(*p)) + p--; + if (*p != '.') + return 0; + + /* Hours, minutes, and whole seconds. */ + n = whole_time_len(line, p - line); + if (!n) + return 0; + + return line + len - p + n; +} + +static size_t trailing_spaces_len(const char *line, size_t len) +{ + const char *p; + + /* Expected format: ' ' x (1 or more) */ + if (!len || line[len - 1] != ' ') + return 0; + + p = line + len; + while (p != line) { + p--; + if (*p != ' ') + return line + len - (p + 1); + } + + /* All spaces! */ + return len; +} + +static size_t name_trailer_len(const char *line, size_t len) +{ + const char *end = line + len; + size_t n; + + /* + * Posix: 2010-07-05 19:41:17 + * GNU: 2010-07-05 19:41:17.620000023 -0500 + */ + + if (!isdigit(end[-1])) + return 0; + + n = tz_len(line, end - line); + end -= n; + + n = whole_time_len(line, end - line); + if (!n) + n = fractional_time_len(line, end - line); + end -= n; + + n = date_len(line, end - line); + if (!n) /* No date. Too bad. */ + return 0; + end -= n; + + if (end == line) /* No space before date. */ + return 0; + if (end[-1] == '\t') { /* Success! */ + end--; + return line + len - end; + } + if (end[-1] != ' ') /* No space before date. */ + return 0; + + /* Whitespace damage. */ + end -= trailing_spaces_len(line, end - line); + return line + len - end; +} + +static char *find_name_common(const char *line, char *def, int p_value, + const char *end, int terminate) { int len; const char *start = NULL; - if (*line == '"') { - char *name = find_name_gnu(line, def, p_value); - if (name) - return name; - } - if (p_value == 0) start = line; - for (;;) { + while (line != end) { char c = *line; - if (isspace(c)) { + if (!end && isspace(c)) { if (c == '\n') break; if (name_terminate(start, line-start, c, terminate)) @@ -505,6 +639,37 @@ static char *find_name(const char *line, char *def, int p_value, int terminate) return squash_slash(xmemdupz(start, len)); } +static char *find_name(const char *line, char *def, int p_value, int terminate) +{ + if (*line == '"') { + char *name = find_name_gnu(line, def, p_value); + if (name) + return name; + } + + return find_name_common(line, def, p_value, NULL, terminate); +} + +static char *find_name_traditional(const char *line, char *def, int p_value) +{ + size_t len = strlen(line); + size_t date_len; + + if (*line == '"') { + char *name = find_name_gnu(line, def, p_value); + if (name) + return name; + } + + len = strchrnul(line, '\n') - line; + date_len = name_trailer_len(line, len); + if (!date_len) + return find_name(line, def, p_value, TERM_TAB); + len -= date_len; + + return find_name_common(line, def, p_value, line + len, 0); +} + static int count_slashes(const char *cp) { int cnt = 0; @@ -527,7 +692,7 @@ static int guess_p_value(const char *nameline) if (is_dev_null(nameline)) return -1; - name = find_name(nameline, NULL, 0, TERM_SPACE | TERM_TAB); + name = find_name_traditional(nameline, NULL, 0); if (!name) return -1; cp = strchr(name, '/'); @@ -646,16 +811,16 @@ static void parse_traditional_patch(const char *first, const char *second, struc if (is_dev_null(first)) { patch->is_new = 1; patch->is_delete = 0; - name = find_name(second, NULL, p_value, TERM_SPACE | TERM_TAB); + name = find_name_traditional(second, NULL, p_value); patch->new_name = name; } else if (is_dev_null(second)) { patch->is_new = 0; patch->is_delete = 1; - name = find_name(first, NULL, p_value, TERM_SPACE | TERM_TAB); + name = find_name_traditional(first, NULL, p_value); patch->old_name = name; } else { - name = find_name(first, NULL, p_value, TERM_SPACE | TERM_TAB); - name = find_name(second, name, p_value, TERM_SPACE | TERM_TAB); + name = find_name_traditional(first, NULL, p_value); + name = find_name_traditional(second, name, p_value); if (has_epoch_timestamp(first)) { patch->is_new = 1; patch->is_delete = 0; diff --git a/t/t4135-apply-weird-filenames.sh b/t/t4135-apply-weird-filenames.sh index c3050a2..b0c0426 100755 --- a/t/t4135-apply-weird-filenames.sh +++ b/t/t4135-apply-weird-filenames.sh @@ -24,10 +24,8 @@ test_expect_success setup ' try_filename() { desc=$1 postimage=$2 - exp1=$3 - exp2=$4 - test_expect_$exp1 "$desc, git-style file creation patch" " + test_expect_success "$desc, git-style file creation patch" " reset_preimage && echo postimage >'$postimage' && git add -N '$postimage' && @@ -40,7 +38,7 @@ try_filename() { test_cmp postimage.saved '$postimage' " - test_expect_$exp2 "$desc, traditional file creation patch" " + test_expect_success "$desc, traditional file creation patch" " reset_preimage && reset_subdirs && echo postimage >b/'$postimage' && @@ -53,11 +51,11 @@ try_filename() { " } -try_filename 'plain' 'postimage.txt' success success -try_filename 'with spaces' 'post image.txt' success failure -try_filename 'with tab' 'post image.txt' success failure -try_filename 'with backslash' 'post\image.txt' success success -try_filename 'with quote' '"postimage".txt' success success +try_filename 'plain' 'postimage.txt' +try_filename 'with spaces' 'post image.txt' +try_filename 'with tab' 'post image.txt' +try_filename 'with backslash' 'post\image.txt' +try_filename 'with quote' '"postimage".txt' test_expect_success 'whitespace-damaged traditional patch' ' reset_preimage && -- 1.7.2.rc2
