filenames with spaces in traditional patches (Re: git-import-dsc: Error importing chromium-browser dsc)

[Jonathan Nieder <jrnieder@gmail.com>]

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@debian.org> for the report
and Guido Günther <agx@sigxcpu.org> for the analysis.

Fixes: http://bugs.debian.org/578752
Breaks: copy-and-pasted patches
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 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