retitle 578752 apply: handle spaces in filenames from traditional patches tags 578752 + upstream thanks Hi, Guido Günther wrote: > On Thu, Apr 22, 2010 at 03:48:12PM +0200, Giuseppe Iuculano wrote: >> error: patch failed: debian/licenses/LICENSE.global:0 >> error: debian/licenses/LICENSE.global: patch does not apply >> Error import /home/debian/chromium/chromium/chromium-browser_5.0.342.9~r43360-0ubuntu2.diff.gz: 256 [...] > dget https://edge.launchpad.net/ubuntu/+archive/primary/+files/chromium-browser_5.0.342.9~r43360-0ubuntu2.dsc [...] > The issue is that git-apply doesn't handle the filenames containing > spaces correctly like > > 'debian/licenses/LICENSE.global BSD-style Chromium' and > 'debian/licenses/LICENSE.Apache (v2.0)'. Thanks, both. The problem is in the parse_traditional_path() function in builtin/apply.c; it simply doesn’t handle paths with spaces. Posix [1] says: | The name and last modification time of each file shall be output in | the following format: | | "---[space]%s %s%s%s", file1, <file1 timestamp>, <file1 frac>, <file1 zone> | "+++[space]%s %s%s%s", file2, <file2 timestamp>, <file2 frac>, <file2 zone> | | Each <file> field shall be the pathname of the corresponding file | being compared, or the single character '-' if standard input is | being compared. However, if the pathname contains a <tab> or a | <newline>, or if it does not consist entirely of characters taken | from the portable character set, the behavior is | implementation-defined. | | Each <timestamp> field shall be equivalent to the output from the | following command: | | date '+%Y-%m-%d%H:%M:%S' | | without the trailing <newline> [...] If this is really describing the format of patches in the wild, that means we should only look for a tab character to terminate the filename. If someone ends up wanting to use a non-git patch to change a file with a tab in its name, well, we can deal with that then. :) A big downside: this does not cope with copy-and-pasted patches with tabs transformed to spaces. The example [2] consists mostly of file-creation patches, so we can’t look to the repository for hints. Maybe the space-plus-date-plus-newline sequence should be used as a delimiter. Here’s a rough patch to give an idea of where to start. [1] http://www.opengroup.org/onlinepubs/9699919799/utilities/diff.html#tag_20_34_10_07 [2] https://edge.launchpad.net/ubuntu/+archive/primary/+files/chromium-browser_5.0.342.9~r43360-0ubuntu2.diff.gz -- 8< -- Subject: apply: handle traditional patches with spaces in filename According to Posix, the --- and +++ lines of a unified diff always include a tab after the filename. By not treating a space as a terminating character, we get support for filenames with spaces automatically. Noticed while patching a program with filenames such as “LICENSE.Apache (v2.0)”. Thanks to Giuseppe Iuculano <iuculano@xxxxxxxxxx> for the report and Guido Günther <agx@xxxxxxxxxxx> for the analysis. Fixes: http://bugs.debian.org/578752 Breaks: copy-and-pasted patches Signed-off-by: Jonathan Nieder <jrnieder@xxxxxxxxx> --- builtin/apply.c | 10 +++++----- 1 files changed, 5 insertions(+), 5 deletions(-) diff --git a/builtin/apply.c b/builtin/apply.c index 771c972..7e1c6b9 100644 --- a/builtin/apply.c +++ b/builtin/apply.c @@ -519,7 +519,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(nameline, NULL, 0, TERM_TAB); if (!name) return -1; cp = strchr(name, '/'); @@ -638,16 +638,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(second, NULL, p_value, TERM_TAB); 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(first, NULL, p_value, TERM_TAB); 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(first, NULL, p_value, TERM_TAB); + name = find_name(second, name, p_value, TERM_TAB); if (has_epoch_timestamp(first)) { patch->is_new = 1; patch->is_delete = 0; -- 1.7.1.rc2.8.g5f4cb -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html