{"thread":{"id":"12544","subject":"[PATCH v3] git-clean: correct printing relative path","startedAt":"2008-03-06T17:43:42Z","lastAt":"2008-03-07T02:19:38Z","messageCount":6,"participants":["Dmitry Potapov","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"71259","messageId":"20080306174342.GA32364@dpotapov.dyndns.org","threadId":"12544","inReplyTo":null,"subject":"[PATCH v3] git-clean: correct printing relative path","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2008-03-06T17:43:42Z","receivedAt":"2008-03-06T17:43:42Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"When the given path contains '..' then git-clean incorrectly printed names\nof files. This patch changes cmd_clean to use quote_path() from wt-status.\nAlso, \"failed to remove ...\" message used absolutely path, but not it is\ncorrected to use relative path.\n\nSigned-off-by: Dmitry Potapov <dpotapov@gmail.com>\n---\nMy previous patch printed an incorrect path in the case of a directory.\nThis was due to that 'qname' was determined before adding 'entry->name'\nto 'directory'.\n\n builtin-clean.c |   32 ++++++++++++++------------------\n wt-status.c     |    4 ++--\n wt-status.h     |    2 ++\n 3 files changed, 18 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin-clean.c b/builtin-clean.c\nindex 3b220d5..4b901fe 100644\n--- a/builtin-clean.c\n+++ b/builtin-clean.c\n@@ -10,6 +10,7 @@\n #include \"cache.h\"\n #include \"dir.h\"\n #include \"parse-options.h\"\n+#include \"wt-status.h\"\n \n static int force = -1; /* unset */\n \n@@ -34,7 +35,8 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \tstruct dir_struct dir;\n \tconst char *path, *base;\n \tstatic const char **pathspec;\n-\tint prefix_offset = 0;\n+\tstruct strbuf buf;\n+\tconst char *qname;\n \tchar *seen = NULL;\n \tstruct option options[] = {\n \t\tOPT__QUIET(&quiet),\n@@ -56,6 +58,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \n \targc = parse_options(argc, argv, options, builtin_clean_usage, 0);\n \n+\tstrbuf_init(&buf, 0);\n \tmemset(&dir, 0, sizeof(dir));\n \tif (ignored_only)\n \t\tdir.show_ignored = 1;\n@@ -72,8 +75,6 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \tif (!ignored)\n \t\tsetup_standard_excludes(&dir);\n \n-\tif (prefix)\n-\t\tprefix_offset = strlen(prefix);\n \tpathspec = get_pathspec(prefix, argv);\n \tread_cache();\n \n@@ -134,39 +135,34 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \n \t\tif (S_ISDIR(st.st_mode)) {\n \t\t\tstrbuf_addstr(&directory, ent->name);\n+\t\t\tqname = quote_path(directory.buf, directory.len, &buf, prefix);\n \t\t\tif (show_only && (remove_directories || matches)) {\n-\t\t\t\tprintf(\"Would remove %s\\n\",\n-\t\t\t\t       directory.buf + prefix_offset);\n+\t\t\t\tprintf(\"Would remove %s\\n\", qname);\n \t\t\t} else if (remove_directories || matches) {\n \t\t\t\tif (!quiet)\n-\t\t\t\t\tprintf(\"Removing %s\\n\",\n-\t\t\t\t\t       directory.buf + prefix_offset);\n+\t\t\t\t\tprintf(\"Removing %s\\n\", qname);\n \t\t\t\tif (remove_dir_recursively(&directory, 0) != 0) {\n-\t\t\t\t\twarning(\"failed to remove '%s'\",\n-\t\t\t\t\t\tdirectory.buf + prefix_offset);\n+\t\t\t\t\twarning(\"failed to remove '%s'\", qname);\n \t\t\t\t\terrors++;\n \t\t\t\t}\n \t\t\t} else if (show_only) {\n-\t\t\t\tprintf(\"Would not remove %s\\n\",\n-\t\t\t\t       directory.buf + prefix_offset);\n+\t\t\t\tprintf(\"Would not remove %s\\n\", qname);\n \t\t\t} else {\n-\t\t\t\tprintf(\"Not removing %s\\n\",\n-\t\t\t\t       directory.buf + prefix_offset);\n+\t\t\t\tprintf(\"Not removing %s\\n\", qname);\n \t\t\t}\n \t\t\tstrbuf_reset(&directory);\n \t\t} else {\n \t\t\tif (pathspec && !matches)\n \t\t\t\tcontinue;\n+\t\t\tqname = quote_path(ent->name, -1, &buf, prefix);\n \t\t\tif (show_only) {\n-\t\t\t\tprintf(\"Would remove %s\\n\",\n-\t\t\t\t       ent->name + prefix_offset);\n+\t\t\t\tprintf(\"Would remove %s\\n\", qname);\n \t\t\t\tcontinue;\n \t\t\t} else if (!quiet) {\n-\t\t\t\tprintf(\"Removing %s\\n\",\n-\t\t\t\t       ent->name + prefix_offset);\n+\t\t\t\tprintf(\"Removing %s\\n\", qname);\n \t\t\t}\n \t\t\tif (unlink(ent->name) != 0) {\n-\t\t\t\twarning(\"failed to remove '%s'\", ent->name);\n+\t\t\t\twarning(\"failed to remove '%s'\", qname);\n \t\t\t\terrors++;\n \t\t\t}\n \t\t}\ndiff --git a/wt-status.c b/wt-status.c\nindex 32d780a..712fe91 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -82,8 +82,8 @@ static void wt_status_print_trailer(struct wt_status *s)\n \tcolor_fprintf_ln(s->fp, color(WT_STATUS_HEADER), \"#\");\n }\n \n-static char *quote_path(const char *in, int len,\n-\t\t\tstruct strbuf *out, const char *prefix)\n+char *quote_path(const char *in, int len,\n+\t\tstruct strbuf *out, const char *prefix)\n {\n \tif (len < 0)\n \t\tlen = strlen(in);\ndiff --git a/wt-status.h b/wt-status.h\nindex 02afaa6..4b46dda 100644\n--- a/wt-status.h\n+++ b/wt-status.h\n@@ -32,5 +32,7 @@ int wt_status_use_color;\n int wt_status_relative_paths;\n void wt_status_prepare(struct wt_status *s);\n void wt_status_print(struct wt_status *s);\n+char *quote_path(const char *in, int len,\n+\t\tstruct strbuf *out, const char *prefix);\n \n #endif /* STATUS_H */\n-- \n1.5.4\n\n"},{"id":"71289","messageId":"7vr6en8n7k.fsf@gitster.siamese.dyndns.org","threadId":"12544","inReplyTo":"20080306174342.GA32364@dpotapov.dyndns.org","subject":"Re: [PATCH v3] git-clean: correct printing relative path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-06T23:08:47Z","receivedAt":"2008-03-06T23:08:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dmitry Potapov <dpotapov@gmail.com> writes:\n\n> When the given path contains '..' then git-clean incorrectly printed names\n> of files. This patch changes cmd_clean to use quote_path() from wt-status.\n\nI think making quote_path() available from outside is fine, but as a\npublic function, quote_path() is grossly misnamed.  It was an Ok name in\nthe context of wt-status, where the sole kind of path quoting necessary\nwas to show themrelative to the current working directory.  As a general\nlibrary, it needs much better name, perhaps quote_path_relative(), and it\nshould move to quote.c, not staying in wt-status.c.\n\nThe implementation of quote_path() also needs to be fixed.  For one thing,\nit does not seem to honor core.quotepath configuration, and it also passes\n\"\\t\" unquoted.  I think rewriting the handcrafted loop to treat \\n and \\r\nspecially should be yanked out and replaced with c_style_quote family of\nfunctions in quote.c\n"},{"id":"71297","messageId":"1204852397-20939-1-git-send-email-dpotapov@gmail.com","threadId":"12544","inReplyTo":"7vr6en8n7k.fsf@gitster.siamese.dyndns.org","subject":"[PATCH 1/2] renaming quote_path() as quote_path_relative()","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2008-03-07T01:13:16Z","receivedAt":"2008-03-07T01:13:16Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"Based on Junio's suggestion, this patch moves quote_path() from\nwt-status.c to quote.c and renames it as quote_path_relative(),\nbecause it is a better name for a public function.\n\nAlso, instead of handcrafted quoting, quote_c_style_counted() is now\nused, so it will honor core.quotepath specified in configuration.\n\nSigned-off-by: Dmitry Potapov <dpotapov@gmail.com>\n---\n quote.c     |   33 +++++++++++++++++++++++++++++++++\n quote.h     |    4 ++++\n wt-status.c |   47 ++---------------------------------------------\n 3 files changed, 39 insertions(+), 45 deletions(-)\n\ndiff --git a/quote.c b/quote.c\nindex d061626..84c6f8a 100644\n--- a/quote.c\n+++ b/quote.c\n@@ -260,6 +260,39 @@ extern void write_name_quotedpfx(const char *pfx, size_t pfxlen,\n \tfputc(terminator, fp);\n }\n \n+/* quote path as relative to the given prefix */\n+char *quote_path_relative(const char *in, int len,\n+                          struct strbuf *out, const char *prefix)\n+{\n+\tif (len < 0)\n+\t\tlen = strlen(in);\n+\n+\tstrbuf_grow(out, len);\n+\tstrbuf_setlen(out, 0);\n+\tif (prefix) {\n+\t\tint off = 0;\n+\t\twhile (prefix[off] && off < len && prefix[off] == in[off])\n+\t\t\tif (prefix[off] == '/') {\n+\t\t\t\tprefix += off + 1;\n+\t\t\t\tin += off + 1;\n+\t\t\t\tlen -= off + 1;\n+\t\t\t\toff = 0;\n+\t\t\t} else\n+\t\t\t\toff++;\n+\n+\t\tfor (; *prefix; prefix++)\n+\t\t\tif (*prefix == '/')\n+\t\t\t\tstrbuf_addstr(out, \"../\");\n+\t}\n+\n+\tquote_c_style_counted (in, len, out, NULL, 1);\n+\n+\tif (!out->len)\n+\t\tstrbuf_addstr(out, \"./\");\n+\n+\treturn out->buf;\n+}\n+\n /*\n  * C-style name unquoting.\n  *\ndiff --git a/quote.h b/quote.h\nindex 4da110e..527c152 100644\n--- a/quote.h\n+++ b/quote.h\n@@ -47,6 +47,10 @@ extern void write_name_quoted(const char *name, FILE *, int terminator);\n extern void write_name_quotedpfx(const char *pfx, size_t pfxlen,\n                                  const char *name, FILE *, int terminator);\n \n+/* quote path as relative to the given prefix */\n+char *quote_path_relative(const char *in, int len,\n+                          struct strbuf *out, const char *prefix);\n+\n /* quoting as a string literal for other languages */\n extern void perl_quote_print(FILE *stream, const char *src);\n extern void python_quote_print(FILE *stream, const char *src);\ndiff --git a/wt-status.c b/wt-status.c\nindex 32d780a..701d13d 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -7,6 +7,7 @@\n #include \"diff.h\"\n #include \"revision.h\"\n #include \"diffcore.h\"\n+#include \"quote.h\"\n \n int wt_status_relative_paths = 1;\n int wt_status_use_color = -1;\n@@ -82,51 +83,7 @@ static void wt_status_print_trailer(struct wt_status *s)\n \tcolor_fprintf_ln(s->fp, color(WT_STATUS_HEADER), \"#\");\n }\n \n-static char *quote_path(const char *in, int len,\n-\t\t\tstruct strbuf *out, const char *prefix)\n-{\n-\tif (len < 0)\n-\t\tlen = strlen(in);\n-\n-\tstrbuf_grow(out, len);\n-\tstrbuf_setlen(out, 0);\n-\tif (prefix) {\n-\t\tint off = 0;\n-\t\twhile (prefix[off] && off < len && prefix[off] == in[off])\n-\t\t\tif (prefix[off] == '/') {\n-\t\t\t\tprefix += off + 1;\n-\t\t\t\tin += off + 1;\n-\t\t\t\tlen -= off + 1;\n-\t\t\t\toff = 0;\n-\t\t\t} else\n-\t\t\t\toff++;\n-\n-\t\tfor (; *prefix; prefix++)\n-\t\t\tif (*prefix == '/')\n-\t\t\t\tstrbuf_addstr(out, \"../\");\n-\t}\n-\n-\tfor ( ; len > 0; in++, len--) {\n-\t\tint ch = *in;\n-\n-\t\tswitch (ch) {\n-\t\tcase '\\n':\n-\t\t\tstrbuf_addstr(out, \"\\\\n\");\n-\t\t\tbreak;\n-\t\tcase '\\r':\n-\t\t\tstrbuf_addstr(out, \"\\\\r\");\n-\t\t\tbreak;\n-\t\tdefault:\n-\t\t\tstrbuf_addch(out, ch);\n-\t\t\tcontinue;\n-\t\t}\n-\t}\n-\n-\tif (!out->len)\n-\t\tstrbuf_addstr(out, \"./\");\n"},{"id":"71298","messageId":"1204852397-20939-2-git-send-email-dpotapov@gmail.com","threadId":"12544","inReplyTo":"1204852397-20939-1-git-send-email-dpotapov@gmail.com","subject":"[PATCH 2/2] git-clean: correct printing relative path","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2008-03-07T01:13:17Z","receivedAt":"2008-03-07T01:13:17Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"When the given path contains '..' then git-clean incorrectly printed names\nof files. This patch changes cmd_clean to use quote_path_relative().\nAlso, \"failed to remove ...\" message used absolutely path, but not it is\ncorrected to use relative path.\n\nSigned-off-by: Dmitry Potapov <dpotapov@gmail.com>\n---\n builtin-clean.c |   32 ++++++++++++++------------------\n 1 files changed, 14 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin-clean.c b/builtin-clean.c\nindex 3b220d5..fefec30 100644\n--- a/builtin-clean.c\n+++ b/builtin-clean.c\n@@ -10,6 +10,7 @@\n #include \"cache.h\"\n #include \"dir.h\"\n #include \"parse-options.h\"\n+#include \"quote.h\"\n \n static int force = -1; /* unset */\n \n@@ -34,7 +35,8 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \tstruct dir_struct dir;\n \tconst char *path, *base;\n \tstatic const char **pathspec;\n-\tint prefix_offset = 0;\n+\tstruct strbuf buf;\n+\tconst char *qname;\n \tchar *seen = NULL;\n \tstruct option options[] = {\n \t\tOPT__QUIET(&quiet),\n@@ -56,6 +58,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \n \targc = parse_options(argc, argv, options, builtin_clean_usage, 0);\n \n+\tstrbuf_init(&buf, 0);\n \tmemset(&dir, 0, sizeof(dir));\n \tif (ignored_only)\n \t\tdir.show_ignored = 1;\n@@ -72,8 +75,6 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \tif (!ignored)\n \t\tsetup_standard_excludes(&dir);\n \n-\tif (prefix)\n-\t\tprefix_offset = strlen(prefix);\n \tpathspec = get_pathspec(prefix, argv);\n \tread_cache();\n \n@@ -134,39 +135,34 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \n \t\tif (S_ISDIR(st.st_mode)) {\n \t\t\tstrbuf_addstr(&directory, ent->name);\n+\t\t\tqname = quote_path_relative(directory.buf, directory.len, &buf, prefix);\n \t\t\tif (show_only && (remove_directories || matches)) {\n-\t\t\t\tprintf(\"Would remove %s\\n\",\n-\t\t\t\t       directory.buf + prefix_offset);\n+\t\t\t\tprintf(\"Would remove %s\\n\", qname);\n \t\t\t} else if (remove_directories || matches) {\n \t\t\t\tif (!quiet)\n-\t\t\t\t\tprintf(\"Removing %s\\n\",\n-\t\t\t\t\t       directory.buf + prefix_offset);\n+\t\t\t\t\tprintf(\"Removing %s\\n\", qname);\n \t\t\t\tif (remove_dir_recursively(&directory, 0) != 0) {\n-\t\t\t\t\twarning(\"failed to remove '%s'\",\n-\t\t\t\t\t\tdirectory.buf + prefix_offset);\n+\t\t\t\t\twarning(\"failed to remove '%s'\", qname);\n \t\t\t\t\terrors++;\n \t\t\t\t}\n \t\t\t} else if (show_only) {\n-\t\t\t\tprintf(\"Would not remove %s\\n\",\n-\t\t\t\t       directory.buf + prefix_offset);\n+\t\t\t\tprintf(\"Would not remove %s\\n\", qname);\n \t\t\t} else {\n-\t\t\t\tprintf(\"Not removing %s\\n\",\n-\t\t\t\t       directory.buf + prefix_offset);\n+\t\t\t\tprintf(\"Not removing %s\\n\", qname);\n \t\t\t}\n \t\t\tstrbuf_reset(&directory);\n \t\t} else {\n \t\t\tif (pathspec && !matches)\n \t\t\t\tcontinue;\n+\t\t\tqname = quote_path_relative(ent->name, -1, &buf, prefix);\n \t\t\tif (show_only) {\n-\t\t\t\tprintf(\"Would remove %s\\n\",\n-\t\t\t\t       ent->name + prefix_offset);\n+\t\t\t\tprintf(\"Would remove %s\\n\", qname);\n \t\t\t\tcontinue;\n \t\t\t} else if (!quiet) {\n-\t\t\t\tprintf(\"Removing %s\\n\",\n-\t\t\t\t       ent->name + prefix_offset);\n+\t\t\t\tprintf(\"Removing %s\\n\", qname);\n \t\t\t}\n \t\t\tif (unlink(ent->name) != 0) {\n-\t\t\t\twarning(\"failed to remove '%s'\", ent->name);\n+\t\t\t\twarning(\"failed to remove '%s'\", qname);\n \t\t\t\terrors++;\n \t\t\t}\n \t\t}\n-- \n1.5.4.3.452.g9b9f\n\n"},{"id":"71303","messageId":"7vve3z5mzh.fsf@gitster.siamese.dyndns.org","threadId":"12544","inReplyTo":"1204852397-20939-1-git-send-email-dpotapov@gmail.com","subject":"Re: [PATCH 1/2] renaming quote_path() as quote_path_relative()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-07T01:41:54Z","receivedAt":"2008-03-07T01:41:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dmitry Potapov <dpotapov@gmail.com> writes:\n\n> Based on Junio's suggestion, this patch moves quote_path() from\n> wt-status.c to quote.c and renames it as quote_path_relative(),\n> because it is a better name for a public function.\n>\n> Also, instead of handcrafted quoting, quote_c_style_counted() is now\n> used, so it will honor core.quotepath specified in configuration.\n\nThanks.\n> +\tstrbuf_grow(out, len);\n> +\tstrbuf_setlen(out, 0);\n> +\tif (prefix) {\n> +\t\tint off = 0;\n> +\t\twhile (prefix[off] && off < len && prefix[off] == in[off])\n> +\t\t\tif (prefix[off] == '/') {\n> +\t\t\t\tprefix += off + 1;\n> +\t\t\t\tin += off + 1;\n> +\t\t\t\tlen -= off + 1;\n> +\t\t\t\toff = 0;\n> +\t\t\t} else\n> +\t\t\t\toff++;\n> +\n> +\t\tfor (; *prefix; prefix++)\n> +\t\t\tif (*prefix == '/')\n> +\t\t\t\tstrbuf_addstr(out, \"../\");\n> +\t}\n> +\n> +\tquote_c_style_counted (in, len, out, NULL, 1);\n\nHmmm.  Shouldn't the whole path (including the ../ part you add) be\nsurrounded by a pair of dq iff quote_c_style() finds that the path needs\nto be quoted?\n\nI am reasonably sure that the scripted version of git-status (before\nwt-status part was rewritten in C) acted that way.\n\n\t$ git --version\n\tgit version 1.2.0\n\t$ echo frotz >qf\\\\w\\\"fq\n\t$ ls\n\t./  ../  .git/\tqf\\w\"fq\n\t$ git add qf*q\n\t$ git status\n\t#\n\t# Initial commit\n\t#\n\t#\n\t# Updated but not checked in:\n\t#   (will commit)\n\t#\n\t#\tnew file: \"qf\\\\w\\\"fq\"\n\t#\n"},{"id":"71308","messageId":"20080307021938.GA5305@dpotapov.dyndns.org","threadId":"12544","inReplyTo":"7vve3z5mzh.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/2] renaming quote_path() as quote_path_relative()","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2008-03-07T02:19:38Z","receivedAt":"2008-03-07T02:19:38Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"On Thu, Mar 06, 2008 at 05:41:54PM -0800, Junio C Hamano wrote:\n> \n> Hmmm.  Shouldn't the whole path (including the ../ part you add) be\n> surrounded by a pair of dq iff quote_c_style() finds that the path needs\n> to be quoted?\n> \n\nYes, I think it is better, and especially so for git-clean output.\nI will resend the patch.\n\nDmitry\n"}]}