[PATCH v3] git-clean: correct printing relative path

[PATCH v3] git-clean: correct printing relative path

[Dmitry Potapov]

When the given path contains '..' then git-clean incorrectly printed names
of files. This patch changes cmd_clean to use quote_path() from wt-status.
Also, "failed to remove ..." message used absolutely path, but not it is
corrected to use relative path.

Signed-off-by: Dmitry Potapov <dpotapov@gmail.com>
---
My previous patch printed an incorrect path in the case of a directory.
This was due to that 'qname' was determined before adding 'entry->name'
to 'directory'.

 builtin-clean.c |   32 ++++++++++++++------------------
 wt-status.c     |    4 ++--
 wt-status.h     |    2 ++
 3 files changed, 18 insertions(+), 20 deletions(-)

diff --git a/builtin-clean.c b/builtin-clean.c
index 3b220d5..4b901fe 100644
--- a/builtin-clean.c
+++ b/builtin-clean.c
@@ -10,6 +10,7 @@
 #include "cache.h"
 #include "dir.h"
 #include "parse-options.h"
+#include "wt-status.h"
 
 static int force = -1; /* unset */
 
@@ -34,7 +35,8 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 	struct dir_struct dir;
 	const char *path, *base;
 	static const char **pathspec;
-	int prefix_offset = 0;
+	struct strbuf buf;
+	const char *qname;
 	char *seen = NULL;
 	struct option options[] = {
 		OPT__QUIET(&quiet),
@@ -56,6 +58,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 
 	argc = parse_options(argc, argv, options, builtin_clean_usage, 0);
 
+	strbuf_init(&buf, 0);
 	memset(&dir, 0, sizeof(dir));
 	if (ignored_only)
 		dir.show_ignored = 1;
@@ -72,8 +75,6 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 	if (!ignored)
 		setup_standard_excludes(&dir);
 
-	if (prefix)
-		prefix_offset = strlen(prefix);
 	pathspec = get_pathspec(prefix, argv);
 	read_cache();
 
@@ -134,39 +135,34 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 
 		if (S_ISDIR(st.st_mode)) {
 			strbuf_addstr(&directory, ent->name);
+			qname = quote_path(directory.buf, directory.len, &buf, prefix);
 			if (show_only && (remove_directories || matches)) {
-				printf("Would remove %s\n",
-				       directory.buf + prefix_offset);
+				printf("Would remove %s\n", qname);
 			} else if (remove_directories || matches) {
 				if (!quiet)
-					printf("Removing %s\n",
-					       directory.buf + prefix_offset);
+					printf("Removing %s\n", qname);
 				if (remove_dir_recursively(&directory, 0) != 0) {
-					warning("failed to remove '%s'",
-						directory.buf + prefix_offset);
+					warning("failed to remove '%s'", qname);
 					errors++;
 				}
 			} else if (show_only) {
-				printf("Would not remove %s\n",
-				       directory.buf + prefix_offset);
+				printf("Would not remove %s\n", qname);
 			} else {
-				printf("Not removing %s\n",
-				       directory.buf + prefix_offset);
+				printf("Not removing %s\n", qname);
 			}
 			strbuf_reset(&directory);
 		} else {
 			if (pathspec && !matches)
 				continue;
+			qname = quote_path(ent->name, -1, &buf, prefix);
 			if (show_only) {
-				printf("Would remove %s\n",
-				       ent->name + prefix_offset);
+				printf("Would remove %s\n", qname);
 				continue;
 			} else if (!quiet) {
-				printf("Removing %s\n",
-				       ent->name + prefix_offset);
+				printf("Removing %s\n", qname);
 			}
 			if (unlink(ent->name) != 0) {
-				warning("failed to remove '%s'", ent->name);
+				warning("failed to remove '%s'", qname);
 				errors++;
 			}
 		}
diff --git a/wt-status.c b/wt-status.c
index 32d780a..712fe91 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -82,8 +82,8 @@ static void wt_status_print_trailer(struct wt_status *s)
 	color_fprintf_ln(s->fp, color(WT_STATUS_HEADER), "#");
 }
 
-static char *quote_path(const char *in, int len,
-			struct strbuf *out, const char *prefix)
+char *quote_path(const char *in, int len,
+		struct strbuf *out, const char *prefix)
 {
 	if (len < 0)
 		len = strlen(in);
diff --git a/wt-status.h b/wt-status.h
index 02afaa6..4b46dda 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -32,5 +32,7 @@ int wt_status_use_color;
 int wt_status_relative_paths;
 void wt_status_prepare(struct wt_status *s);
 void wt_status_print(struct wt_status *s);
+char *quote_path(const char *in, int len,
+		struct strbuf *out, const char *prefix);
 
 #endif /* STATUS_H */
-- 
1.5.4

Re: [PATCH v3] git-clean: correct printing relative path

[Junio C Hamano]

Dmitry Potapov <dpotapov@gmail.com> writes:

> When the given path contains '..' then git-clean incorrectly printed names
> of files. This patch changes cmd_clean to use quote_path() from wt-status.

I think making quote_path() available from outside is fine, but as a
public function, quote_path() is grossly misnamed.  It was an Ok name in
the context of wt-status, where the sole kind of path quoting necessary
was to show themrelative to the current working directory.  As a general
library, it needs much better name, perhaps quote_path_relative(), and it
should move to quote.c, not staying in wt-status.c.

The implementation of quote_path() also needs to be fixed.  For one thing,
it does not seem to honor core.quotepath configuration, and it also passes
"\t" unquoted.  I think rewriting the handcrafted loop to treat \n and \r
specially should be yanked out and replaced with c_style_quote family of
functions in quote.c

[PATCH 1/2] renaming quote_path() as quote_path_relative()

[Dmitry Potapov]

Based on Junio's suggestion, this patch moves quote_path() from
wt-status.c to quote.c and renames it as quote_path_relative(),
because it is a better name for a public function.

Also, instead of handcrafted quoting, quote_c_style_counted() is now
used, so it will honor core.quotepath specified in configuration.

Signed-off-by: Dmitry Potapov <dpotapov@gmail.com>
---
 quote.c     |   33 +++++++++++++++++++++++++++++++++
 quote.h     |    4 ++++
 wt-status.c |   47 ++---------------------------------------------
 3 files changed, 39 insertions(+), 45 deletions(-)

diff --git a/quote.c b/quote.c
index d061626..84c6f8a 100644
--- a/quote.c
+++ b/quote.c
@@ -260,6 +260,39 @@ extern void write_name_quotedpfx(const char *pfx, size_t pfxlen,
 	fputc(terminator, fp);
 }
 
+/* quote path as relative to the given prefix */
+char *quote_path_relative(const char *in, int len,
+                          struct strbuf *out, const char *prefix)
+{
+	if (len < 0)
+		len = strlen(in);
+
+	strbuf_grow(out, len);
+	strbuf_setlen(out, 0);
+	if (prefix) {
+		int off = 0;
+		while (prefix[off] && off < len && prefix[off] == in[off])
+			if (prefix[off] == '/') {
+				prefix += off + 1;
+				in += off + 1;
+				len -= off + 1;
+				off = 0;
+			} else
+				off++;
+
+		for (; *prefix; prefix++)
+			if (*prefix == '/')
+				strbuf_addstr(out, "../");
+	}
+
+	quote_c_style_counted (in, len, out, NULL, 1);
+
+	if (!out->len)
+		strbuf_addstr(out, "./");
+
+	return out->buf;
+}
+
 /*
  * C-style name unquoting.
  *
diff --git a/quote.h b/quote.h
index 4da110e..527c152 100644
--- a/quote.h
+++ b/quote.h
@@ -47,6 +47,10 @@ extern void write_name_quoted(const char *name, FILE *, int terminator);
 extern void write_name_quotedpfx(const char *pfx, size_t pfxlen,
                                  const char *name, FILE *, int terminator);
 
+/* quote path as relative to the given prefix */
+char *quote_path_relative(const char *in, int len,
+                          struct strbuf *out, const char *prefix);
+
 /* quoting as a string literal for other languages */
 extern void perl_quote_print(FILE *stream, const char *src);
 extern void python_quote_print(FILE *stream, const char *src);
diff --git a/wt-status.c b/wt-status.c
index 32d780a..701d13d 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -7,6 +7,7 @@
 #include "diff.h"
 #include "revision.h"
 #include "diffcore.h"
+#include "quote.h"
 
 int wt_status_relative_paths = 1;
 int wt_status_use_color = -1;
@@ -82,51 +83,7 @@ static void wt_status_print_trailer(struct wt_status *s)
 	color_fprintf_ln(s->fp, color(WT_STATUS_HEADER), "#");
 }
 
-static char *quote_path(const char *in, int len,
-			struct strbuf *out, const char *prefix)
-{
-	if (len < 0)
-		len = strlen(in);
-
-	strbuf_grow(out, len);
-	strbuf_setlen(out, 0);
-	if (prefix) {
-		int off = 0;
-		while (prefix[off] && off < len && prefix[off] == in[off])
-			if (prefix[off] == '/') {
-				prefix += off + 1;
-				in += off + 1;
-				len -= off + 1;
-				off = 0;
-			} else
-				off++;
-
-		for (; *prefix; prefix++)
-			if (*prefix == '/')
-				strbuf_addstr(out, "../");
-	}
-
-	for ( ; len > 0; in++, len--) {
-		int ch = *in;
-
-		switch (ch) {
-		case '\n':
-			strbuf_addstr(out, "\\n");
-			break;
-		case '\r':
-			strbuf_addstr(out, "\\r");
-			break;
-		default:
-			strbuf_addch(out, ch);
-			continue;
-		}
-	}
-
-	if (!out->len)
-		strbuf_addstr(out, "./");

[PATCH 2/2] git-clean: correct printing relative path

[Dmitry Potapov]

When the given path contains '..' then git-clean incorrectly printed names
of files. This patch changes cmd_clean to use quote_path_relative().
Also, "failed to remove ..." message used absolutely path, but not it is
corrected to use relative path.

Signed-off-by: Dmitry Potapov <dpotapov@gmail.com>
---
 builtin-clean.c |   32 ++++++++++++++------------------
 1 files changed, 14 insertions(+), 18 deletions(-)

diff --git a/builtin-clean.c b/builtin-clean.c
index 3b220d5..fefec30 100644
--- a/builtin-clean.c
+++ b/builtin-clean.c
@@ -10,6 +10,7 @@
 #include "cache.h"
 #include "dir.h"
 #include "parse-options.h"
+#include "quote.h"
 
 static int force = -1; /* unset */
 
@@ -34,7 +35,8 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 	struct dir_struct dir;
 	const char *path, *base;
 	static const char **pathspec;
-	int prefix_offset = 0;
+	struct strbuf buf;
+	const char *qname;
 	char *seen = NULL;
 	struct option options[] = {
 		OPT__QUIET(&quiet),
@@ -56,6 +58,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 
 	argc = parse_options(argc, argv, options, builtin_clean_usage, 0);
 
+	strbuf_init(&buf, 0);
 	memset(&dir, 0, sizeof(dir));
 	if (ignored_only)
 		dir.show_ignored = 1;
@@ -72,8 +75,6 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 	if (!ignored)
 		setup_standard_excludes(&dir);
 
-	if (prefix)
-		prefix_offset = strlen(prefix);
 	pathspec = get_pathspec(prefix, argv);
 	read_cache();
 
@@ -134,39 +135,34 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 
 		if (S_ISDIR(st.st_mode)) {
 			strbuf_addstr(&directory, ent->name);
+			qname = quote_path_relative(directory.buf, directory.len, &buf, prefix);
 			if (show_only && (remove_directories || matches)) {
-				printf("Would remove %s\n",
-				       directory.buf + prefix_offset);
+				printf("Would remove %s\n", qname);
 			} else if (remove_directories || matches) {
 				if (!quiet)
-					printf("Removing %s\n",
-					       directory.buf + prefix_offset);
+					printf("Removing %s\n", qname);
 				if (remove_dir_recursively(&directory, 0) != 0) {
-					warning("failed to remove '%s'",
-						directory.buf + prefix_offset);
+					warning("failed to remove '%s'", qname);
 					errors++;
 				}
 			} else if (show_only) {
-				printf("Would not remove %s\n",
-				       directory.buf + prefix_offset);
+				printf("Would not remove %s\n", qname);
 			} else {
-				printf("Not removing %s\n",
-				       directory.buf + prefix_offset);
+				printf("Not removing %s\n", qname);
 			}
 			strbuf_reset(&directory);
 		} else {
 			if (pathspec && !matches)
 				continue;
+			qname = quote_path_relative(ent->name, -1, &buf, prefix);
 			if (show_only) {
-				printf("Would remove %s\n",
-				       ent->name + prefix_offset);
+				printf("Would remove %s\n", qname);
 				continue;
 			} else if (!quiet) {
-				printf("Removing %s\n",
-				       ent->name + prefix_offset);
+				printf("Removing %s\n", qname);
 			}
 			if (unlink(ent->name) != 0) {
-				warning("failed to remove '%s'", ent->name);
+				warning("failed to remove '%s'", qname);
 				errors++;
 			}
 		}
-- 
1.5.4.3.452.g9b9f

Re: [PATCH 1/2] renaming quote_path() as quote_path_relative()

[Junio C Hamano]

Dmitry Potapov <dpotapov@gmail.com> writes:

> Based on Junio's suggestion, this patch moves quote_path() from
> wt-status.c to quote.c and renames it as quote_path_relative(),
> because it is a better name for a public function.
>
> Also, instead of handcrafted quoting, quote_c_style_counted() is now
> used, so it will honor core.quotepath specified in configuration.

Thanks.
> +	strbuf_grow(out, len);
> +	strbuf_setlen(out, 0);
> +	if (prefix) {
> +		int off = 0;
> +		while (prefix[off] && off < len && prefix[off] == in[off])
> +			if (prefix[off] == '/') {
> +				prefix += off + 1;
> +				in += off + 1;
> +				len -= off + 1;
> +				off = 0;
> +			} else
> +				off++;
> +
> +		for (; *prefix; prefix++)
> +			if (*prefix == '/')
> +				strbuf_addstr(out, "../");
> +	}
> +
> +	quote_c_style_counted (in, len, out, NULL, 1);

Hmmm.  Shouldn't the whole path (including the ../ part you add) be
surrounded by a pair of dq iff quote_c_style() finds that the path needs
to be quoted?

I am reasonably sure that the scripted version of git-status (before
wt-status part was rewritten in C) acted that way.

	$ git --version
	git version 1.2.0
	$ echo frotz >qf\\w\"fq
	$ ls
	./  ../  .git/	qf\w"fq
	$ git add qf*q
	$ git status
	#
	# Initial commit
	#
	#
	# Updated but not checked in:
	#   (will commit)
	#
	#	new file: "qf\\w\"fq"
	#

Re: [PATCH 1/2] renaming quote_path() as quote_path_relative()

[Dmitry Potapov]

On Thu, Mar 06, 2008 at 05:41:54PM -0800, Junio C Hamano wrote:
> 
> Hmmm.  Shouldn't the whole path (including the ../ part you add) be
> surrounded by a pair of dq iff quote_c_style() finds that the path needs
> to be quoted?
> 

Yes, I think it is better, and especially so for git-clean output.
I will resend the patch.

Dmitry

[PATCH 1/2 v3] renaming quote_path() as quote_path_relative()

[Dmitry Potapov]

Based on Junio's suggestion, this patch moves quote_path() from
wt-status.c to quote.c and renames it as quote_path_relative(),
because it is a better name for a public function.

Also, instead of handcrafted quoting, quote_c_style_counted() is now
used, so it will honor core.quotepath specified in configuration.
The whole path is quoted now (as it was before in the scripted version
of git status before it was rewritten in C).

I have put strbuf_setlen() before strbuf_grow(), because otherwise
strbuf_grow() will increase the buffer on top of the context of the
buffer that we discard by setting the buffer length to zero.

Signed-off-by: Dmitry Potapov <dpotapov@gmail.com>
---

This version of patch has been changed to put in double quotes only
those names that have escaped characters in them.  Names containing
spaces or single quotes are not quoted, but it is consistent with
how all other functions work (for instance, write_name_quotedpfx).

If we want quote names consisting of spaces and single quotes, I will
prepare a separate patch.

 quote.c     |   45 +++++++++++++++++++++++++++++++++++++++++++++
 quote.h     |    4 ++++
 wt-status.c |   47 ++---------------------------------------------
 3 files changed, 51 insertions(+), 45 deletions(-)

diff --git a/quote.c b/quote.c
index 40702f6..133f714 100644
--- a/quote.c
+++ b/quote.c
@@ -260,6 +260,51 @@ extern void write_name_quotedpfx(const char *pfx, size_t pfxlen,
 	fputc(terminator, fp);
 }
 
+/* quote path as relative to the given prefix */
+char *quote_path_relative(const char *in, int len,
+                          struct strbuf *out, const char *prefix)
+{
+	int needquote = 0;
+
+	if (len < 0)
+		len = strlen(in);
+
+	strbuf_setlen(out, 0);
+	needquote = next_quote_pos (in, len) != len;
+	if (needquote)
+	{
+		strbuf_grow(out, len*3/2);
+		strbuf_addch(out, '"');
+	}
+	else
+		strbuf_grow(out, len);
+
+	if (prefix) {
+		int off = 0;
+		while (prefix[off] && off < len && prefix[off] == in[off])
+			if (prefix[off] == '/') {
+				prefix += off + 1;
+				in += off + 1;
+				len -= off + 1;
+				off = 0;
+			} else
+				off++;
+
+		for (; *prefix; prefix++)
+			if (*prefix == '/')
+				strbuf_addstr(out, "../");
+	}
+
+	quote_c_style_counted (in, len, out, NULL, 1);
+	if (needquote)
+		strbuf_addch(out, '"');
+
+	if (!out->len)
+		strbuf_addstr(out, "./");
+
+	return out->buf;
+}
+
 /*
  * C-style name unquoting.
  *
diff --git a/quote.h b/quote.h
index 4da110e..527c152 100644
--- a/quote.h
+++ b/quote.h
@@ -47,6 +47,10 @@ extern void write_name_quoted(const char *name, FILE *, int terminator);
 extern void write_name_quotedpfx(const char *pfx, size_t pfxlen,
                                  const char *name, FILE *, int terminator);
 
+/* quote path as relative to the given prefix */
+char *quote_path_relative(const char *in, int len,
+                          struct strbuf *out, const char *prefix);
+
 /* quoting as a string literal for other languages */
 extern void perl_quote_print(FILE *stream, const char *src);
 extern void python_quote_print(FILE *stream, const char *src);
diff --git a/wt-status.c b/wt-status.c
index 32d780a..701d13d 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -7,6 +7,7 @@
 #include "diff.h"
 #include "revision.h"
 #include "diffcore.h"
+#include "quote.h"
 
 int wt_status_relative_paths = 1;
 int wt_status_use_color = -1;
@@ -82,51 +83,7 @@ static void wt_status_print_trailer(struct wt_status *s)
 	color_fprintf_ln(s->fp, color(WT_STATUS_HEADER), "#");
 }
 
-static char *quote_path(const char *in, int len,
-			struct strbuf *out, const char *prefix)
-{
-	if (len < 0)
-		len = strlen(in);
-
-	strbuf_grow(out, len);
-	strbuf_setlen(out, 0);
-	if (prefix) {
-		int off = 0;
-		while (prefix[off] && off < len && prefix[off] == in[off])
-			if (prefix[off] == '/') {
-				prefix += off + 1;
-				in += off + 1;
-				len -= off + 1;
-				off = 0;
-			} else
-				off++;
-
-		for (; *prefix; prefix++)
-			if (*prefix == '/')
-				strbuf_addstr(out, "../");
-	}
-
-	for ( ; len > 0; in++, len--) {
-		int ch = *in;
-
-		switch (ch) {
-		case '\n':
-			strbuf_addstr(out, "\\n");
-			break;
-		case '\r':
-			strbuf_addstr(out, "\\r");
-			break;
-		default:
-			strbuf_addch(out, ch);
-			continue;
-		}
-	}
-
-	if (!out->len)
-		strbuf_addstr(out, "./");

[PATCH 2/2] git-clean: correct printing relative path

[Dmitry Potapov]

When the given path contains '..' then git-clean incorrectly printed names
of files. This patch changes cmd_clean to use quote_path_relative().
Also, "failed to remove ..." message used absolutely path, but not it is
corrected to use relative path.

Signed-off-by: Dmitry Potapov <dpotapov@gmail.com>
---
 builtin-clean.c |   32 ++++++++++++++------------------
 1 files changed, 14 insertions(+), 18 deletions(-)

diff --git a/builtin-clean.c b/builtin-clean.c
index 3b220d5..fefec30 100644
--- a/builtin-clean.c
+++ b/builtin-clean.c
@@ -10,6 +10,7 @@
 #include "cache.h"
 #include "dir.h"
 #include "parse-options.h"
+#include "quote.h"
 
 static int force = -1; /* unset */
 
@@ -34,7 +35,8 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 	struct dir_struct dir;
 	const char *path, *base;
 	static const char **pathspec;
-	int prefix_offset = 0;
+	struct strbuf buf;
+	const char *qname;
 	char *seen = NULL;
 	struct option options[] = {
 		OPT__QUIET(&quiet),
@@ -56,6 +58,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 
 	argc = parse_options(argc, argv, options, builtin_clean_usage, 0);
 
+	strbuf_init(&buf, 0);
 	memset(&dir, 0, sizeof(dir));
 	if (ignored_only)
 		dir.show_ignored = 1;
@@ -72,8 +75,6 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 	if (!ignored)
 		setup_standard_excludes(&dir);
 
-	if (prefix)
-		prefix_offset = strlen(prefix);
 	pathspec = get_pathspec(prefix, argv);
 	read_cache();
 
@@ -134,39 +135,34 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 
 		if (S_ISDIR(st.st_mode)) {
 			strbuf_addstr(&directory, ent->name);
+			qname = quote_path_relative(directory.buf, directory.len, &buf, prefix);
 			if (show_only && (remove_directories || matches)) {
-				printf("Would remove %s\n",
-				       directory.buf + prefix_offset);
+				printf("Would remove %s\n", qname);
 			} else if (remove_directories || matches) {
 				if (!quiet)
-					printf("Removing %s\n",
-					       directory.buf + prefix_offset);
+					printf("Removing %s\n", qname);
 				if (remove_dir_recursively(&directory, 0) != 0) {
-					warning("failed to remove '%s'",
-						directory.buf + prefix_offset);
+					warning("failed to remove '%s'", qname);
 					errors++;
 				}
 			} else if (show_only) {
-				printf("Would not remove %s\n",
-				       directory.buf + prefix_offset);
+				printf("Would not remove %s\n", qname);
 			} else {
-				printf("Not removing %s\n",
-				       directory.buf + prefix_offset);
+				printf("Not removing %s\n", qname);
 			}
 			strbuf_reset(&directory);
 		} else {
 			if (pathspec && !matches)
 				continue;
+			qname = quote_path_relative(ent->name, -1, &buf, prefix);
 			if (show_only) {
-				printf("Would remove %s\n",
-				       ent->name + prefix_offset);
+				printf("Would remove %s\n", qname);
 				continue;
 			} else if (!quiet) {
-				printf("Removing %s\n",
-				       ent->name + prefix_offset);
+				printf("Removing %s\n", qname);
 			}
 			if (unlink(ent->name) != 0) {
-				warning("failed to remove '%s'", ent->name);
+				warning("failed to remove '%s'", qname);
 				errors++;
 			}
 		}
-- 
1.5.4.3.486.g3d6a2