[PATCH] fast-export: quote paths with spaces

[PATCH] fast-export: quote paths with spaces

[Jay Soffian]

A path containing a space must be quoted when used as an argument to
either the copy or rename commands. 6280dfdc3b (fast-export: quote paths
in output, 2011-08-05) previously attempted to fix fast-export's quoting
by passing all paths through quote_c_style(). However, that function does
not consider the space to be a character which requires quoting, so let's
special-case the space inside print_path(). This will cause
space-containing paths to also be quoted in other commands where such
quoting is not strictly necessary, but it does not hurt to do so.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
Sorry, not test added. I barely had time to get out this patch. :-(

 builtin/fast-export.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index ef7c012094..cc5ef82fe6 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -183,9 +183,11 @@ static int depth_first(const void *a_, const void *b_)
 static void print_path(const char *path)
 {
 	int need_quote = quote_c_style(path, NULL, NULL, 0);
-	if (need_quote)
+	if (need_quote) {
 		quote_c_style(path, NULL, stdout, 0);
-	else
+	} else if (strchr(path, ' ')) {
+		printf("\"%s\"", path);
+	} else
 		printf("%s", path);
 }
 
-- 
1.7.11

Re: [PATCH] fast-export: quote paths with spaces

[Junio C Hamano]

Jay Soffian <jaysoffian@gmail.com> writes:

> A path containing a space must be quoted when used as an argument to
> either the copy or rename commands. 6280dfdc3b (fast-export: quote paths
> in output, 2011-08-05) previously attempted to fix fast-export's quoting
> by passing all paths through quote_c_style(). However, that function does
> not consider the space to be a character which requires quoting, so let's
> special-case the space inside print_path(). This will cause
> space-containing paths to also be quoted in other commands where such
> quoting is not strictly necessary, but it does not hurt to do so.
>
> Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
> ---
> Sorry, not test added. I barely had time to get out this patch. :-(

Thanks.

I have to say it might be less error prone to always c-quote the
string, as the recipient is expected to be able to grok it anyway,
but I'll let fast-import/export experts to comment on it and perhaps
add a test or two ;-)

>  builtin/fast-export.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/fast-export.c b/builtin/fast-export.c
> index ef7c012094..cc5ef82fe6 100644
> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> @@ -183,9 +183,11 @@ static int depth_first(const void *a_, const void *b_)
>  static void print_path(const char *path)
>  {
>  	int need_quote = quote_c_style(path, NULL, NULL, 0);
> -	if (need_quote)
> +	if (need_quote) {
>  		quote_c_style(path, NULL, stdout, 0);
> -	else
> +	} else if (strchr(path, ' ')) {
> +		printf("\"%s\"", path);
> +	} else
>  		printf("%s", path);
>  }

Re: [PATCH] fast-export: quote paths with spaces

[Jeff King]

On Mon, Jun 25, 2012 at 07:21:03PM -0700, Junio C Hamano wrote:

> Jay Soffian <jaysoffian@gmail.com> writes:
> 
> > A path containing a space must be quoted when used as an argument to
> > either the copy or rename commands. 6280dfdc3b (fast-export: quote paths
> > in output, 2011-08-05) previously attempted to fix fast-export's quoting
> > by passing all paths through quote_c_style(). However, that function does
> > not consider the space to be a character which requires quoting, so let's
> > special-case the space inside print_path(). This will cause
> > space-containing paths to also be quoted in other commands where such
> > quoting is not strictly necessary, but it does not hurt to do so.
> >
> > Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
> > ---
> > Sorry, not test added. I barely had time to get out this patch. :-(
> 
> Thanks.
> 
> I have to say it might be less error prone to always c-quote the
> string, as the recipient is expected to be able to grok it anyway,
> but I'll let fast-import/export experts to comment on it and perhaps
> add a test or two ;-)

I am not (and do not want to be) a fast-import/export expert, but since
the bug was in my incomplete fix, here is a slightly cleaned up version
of the patch.

I expanded the commit message a bit to explain why 6280dfdc3b did not
notice this and adjusted the tests to cover this. I also tweaked the
code to fit our usual no-curlies-for-single line style.

I don't think there's anything wrong with just quoting all of the time;
the output is not typically read by humans. But not quoting does make it
easier for lazy scripts to work when there are no magic characters
present.

-- >8 --
From: Jay Soffian <jaysoffian@gmail.com>
Subject: [PATCH] fast-export: quote paths with spaces

A path containing a space must be quoted when used as an
argument to either the copy or rename commands (because
unlike other commands, the path is not the final thing on
the line for those commands).

Commit 6280dfdc3b (fast-export: quote paths in output,
2011-08-05) previously attempted to fix fast-export's
quoting by passing all paths through quote_c_style().
However, that function does not consider the space to be a
character which requires quoting, so let's special-case the
space inside print_path(). This will cause space-containing
paths to also be quoted in other commands where such quoting
is not strictly necessary, but it does not hurt to do so.

The test from 6280dfdc3b did not detect this because, while
it does introduce renames in the export stream, it does not
actually turn on rename detection, so they were presented as
pairs of deletions/adds. Using "-M" reveals the bug.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/fast-export.c  | 2 ++
 t/t9350-fast-export.sh | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index ef7c012..9ab6db3 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -185,6 +185,8 @@ static void print_path(const char *path)
 	int need_quote = quote_c_style(path, NULL, NULL, 0);
 	if (need_quote)
 		quote_c_style(path, NULL, stdout, 0);
+	else if (strchr(path, ' '))
+		printf("\"%s\"", path);
 	else
 		printf("%s", path);
 }
diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
index b00196b..dc99556 100755
--- a/t/t9350-fast-export.sh
+++ b/t/t9350-fast-export.sh
@@ -430,7 +430,7 @@ test_expect_success 'fast-export quotes pathnames' '
 	 git commit -m rename &&
 	 git read-tree --empty &&
 	 git commit -m deletion &&
-	 git fast-export HEAD >export.out &&
+	 git fast-export -M HEAD >export.out &&
 	 git rev-list HEAD >expect &&
 	 git init result &&
 	 cd result &&
-- 
1.7.11.29.g70f3df7