From 3296766eb5531ef051ae392114de5d75556f5613 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 6 Mar 2008 23:18:08 -0800 Subject: [PATCH 1/7] get_pathspec(): die when an out-of-tree path is given An earlier commit d089ebaa (setup: sanitize absolute and funny paths) made get_pathspec() aware of absolute paths, but with a botched interface that forced the callers to count the resulting pathspecs in order to detect an error of giving a path that is outside the work tree. This fixes it, by dying inside the function. We had ls-tree test that relied on a misfeature in the original implementation of its pathspec handling. Leading slashes were silently removed from them. However we allow giving absolute pathnames (people want to cut and paste from elsewhere) that are inside work tree these days, so a pathspec that begin with slash _should_ be treated as a full path. The test is adjusted to match the updated rule for get_pathspec(). Earlier I mistook three tests given by Robin that they should succeed, but these are attempts to add path outside work tree, which should fail loudly. These tests also have been fixed. Signed-off-by: Junio C Hamano --- setup.c | 2 ++ t/t3101-ls-tree-dirname.sh | 2 +- t/t7010-setup.sh | 7 ++++--- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/setup.c b/setup.c index 89c81e54e6..41e298b8f5 100644 --- a/setup.c +++ b/setup.c @@ -202,6 +202,8 @@ const char **get_pathspec(const char *prefix, const char **pathspec) const char *p = prefix_path(prefix, prefixlen, *src); if (p) *(dst++) = p; + else + exit(128); /* error message already given */ src++; } *dst = NULL; diff --git a/t/t3101-ls-tree-dirname.sh b/t/t3101-ls-tree-dirname.sh index 39fe2676dc..70f9ce9d52 100755 --- a/t/t3101-ls-tree-dirname.sh +++ b/t/t3101-ls-tree-dirname.sh @@ -120,7 +120,7 @@ EOF # having 1.txt and path3 test_expect_success \ 'ls-tree filter odd names' \ - 'git ls-tree $tree 1.txt /1.txt //1.txt path3/1.txt /path3/1.txt //path3//1.txt path3 /path3/ path3// >current && + 'git ls-tree $tree 1.txt ./1.txt .//1.txt path3/1.txt path3/./1.txt path3 path3// >current && cat >expected <<\EOF && 100644 blob X 1.txt 100644 blob X path3/1.txt diff --git a/t/t7010-setup.sh b/t/t7010-setup.sh index e809e0e2c9..bc8ab6a619 100755 --- a/t/t7010-setup.sh +++ b/t/t7010-setup.sh @@ -142,15 +142,16 @@ test_expect_success 'setup deeper work tree' ' test_expect_success 'add a directory outside the work tree' '( cd tester && d1="$(cd .. ; pwd)" && - git add "$d1" + test_must_fail git add "$d1" )' + test_expect_success 'add a file outside the work tree, nasty case 1' '( cd tester && f="$(pwd)x" && echo "$f" && touch "$f" && - git add "$f" + test_must_fail git add "$f" )' test_expect_success 'add a file outside the work tree, nasty case 2' '( @@ -158,7 +159,7 @@ test_expect_success 'add a file outside the work tree, nasty case 2' '( f="$(pwd | sed "s/.$//")x" && echo "$f" && touch "$f" && - git add "$f" + test_must_fail git add "$f" )' test_done From 971dfa19598324f49bd3a010ca629c91d6a0f758 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 6 Mar 2008 23:29:40 -0800 Subject: [PATCH 2/7] Revert part of 744dacd (builtin-mv: minimum fix to avoid losing files) When get_pathspec() was originally made absolute-path capable, we botched the interface to it, without dying inside the function when given a path that is outside the work tree, and made it the responsibility of callers to check the condition in a roundabout way. This is made unnecessary with the previous patch. Signed-off-by: Junio C Hamano --- builtin-mv.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/builtin-mv.c b/builtin-mv.c index 68aa2a68bb..94f6dd2aad 100644 --- a/builtin-mv.c +++ b/builtin-mv.c @@ -19,7 +19,6 @@ static const char **copy_pathspec(const char *prefix, const char **pathspec, int count, int base_name) { int i; - int len = prefix ? strlen(prefix) : 0; const char **result = xmalloc((count + 1) * sizeof(const char *)); memcpy(result, pathspec, count * sizeof(const char *)); result[count] = NULL; @@ -33,11 +32,8 @@ static const char **copy_pathspec(const char *prefix, const char **pathspec, if (last_slash) result[i] = last_slash + 1; } - result[i] = prefix_path(prefix, len, result[i]); - if (!result[i]) - exit(1); /* error already given */ } - return result; + return get_pathspec(prefix, result); } static void show_list(const char *label, struct path_list *list) From 6c53e7ac04ecdb8b2697aea4bb9bec8715209e68 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 6 Mar 2008 23:31:29 -0800 Subject: [PATCH 3/7] Revert part of 1abf095 (git-add: adjust to the get_pathspec() changes) When get_pathspec() was originally made absolute-path capable, we botched the interface to it, without dying inside the function when given a path that is outside the work tree, and made it the responsibility of callers to check the condition in a roundabout way. This is made unnecessary with the previous patch. Signed-off-by: Junio C Hamano --- builtin-add.c | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/builtin-add.c b/builtin-add.c index 820110e085..4a91e3eb11 100644 --- a/builtin-add.c +++ b/builtin-add.c @@ -228,18 +228,6 @@ int cmd_add(int argc, const char **argv, const char *prefix) goto finish; } - if (*argv) { - /* Was there an invalid path? */ - if (pathspec) { - int num; - for (num = 0; pathspec[num]; num++) - ; /* just counting */ - if (argc != num) - exit(1); /* error message already given */ - } else - exit(1); /* error message already given */ - } - fill_directory(&dir, pathspec, ignored_too); if (show_only) { From 79418599e7221ef10641cf9cc07018c25b4d0310 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 6 Mar 2008 23:50:51 -0800 Subject: [PATCH 4/7] Revert part of d089eba (setup: sanitize absolute and funny paths in get_pathspec()) When get_pathspec() was originally made absolute-path capable, we botched the interface to it, without dying inside the function when given a path that is outside the work tree, and made it the responsibility of callers to check the condition in a roundabout way. This is made unnecessary with the previous patch. Signed-off-by: Junio C Hamano --- builtin-ls-files.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/builtin-ls-files.c b/builtin-ls-files.c index 25dbfb4499..dc7eab89b3 100644 --- a/builtin-ls-files.c +++ b/builtin-ls-files.c @@ -574,17 +574,8 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix) pathspec = get_pathspec(prefix, argv + i); /* Verify that the pathspec matches the prefix */ - if (pathspec) { - if (argc != i) { - int cnt; - for (cnt = 0; pathspec[cnt]; cnt++) - ; - if (cnt != (argc - i)) - exit(1); /* error message already given */ - } + if (pathspec) prefix = verify_pathspec(prefix); - } else if (argc != i) - exit(1); /* error message already given */ /* Treat unmatching pathspec elements as errors */ if (pathspec && error_unmatch) { From a734d0b10bd0f5554abb3acdf11426040cfc4df0 Mon Sep 17 00:00:00 2001 From: Dmitry Potapov Date: Fri, 7 Mar 2008 05:30:58 +0300 Subject: [PATCH 5/7] Make private quote_path() in wt-status.c available as quote_path_relative() Move quote_path() from wt-status.c to quote.c and rename 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, to make its quoting more consistent with the rest of the system, also honoring core.quotepath specified in configuration. Signed-off-by: Dmitry Potapov Signed-off-by: Junio C Hamano --- quote.c | 42 ++++++++++++++++++++++++++++++++++++++++++ quote.h | 4 ++++ wt-status.c | 47 ++--------------------------------------------- 3 files changed, 48 insertions(+), 45 deletions(-) diff --git a/quote.c b/quote.c index d061626c34..e38ba39b64 100644 --- a/quote.c +++ b/quote.c @@ -260,6 +260,48 @@ 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; + + if (len < 0) + len = strlen(in); + + /* "../" prefix itself does not need quoting, but "in" might. */ + needquote = next_quote_pos(in, len) < len; + strbuf_setlen(out, 0); + strbuf_grow(out, len); + + if (needquote) + strbuf_addch(out, '"'); + 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 4da110ec01..c5eea6f18e 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 32d780af1e..701d13da7c 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, "./"); - - return out->buf; -} +#define quote_path quote_path_relative static void wt_status_print_filepair(struct wt_status *s, int t, struct diff_filepair *p) From 1fb328947c8e3ace9df7d2d5374e26e2510a4e93 Mon Sep 17 00:00:00 2001 From: Dmitry Potapov Date: Fri, 7 Mar 2008 04:13:17 +0300 Subject: [PATCH 6/7] git-clean: correct printing relative path 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 Signed-off-by: Junio C Hamano --- builtin-clean.c | 32 ++++++++++++++------------------ 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/builtin-clean.c b/builtin-clean.c index 3b220d5060..fefec3010c 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++; } } From 5b7570cfb41c34ce585ede3fc1e45fa48febbd8f Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 7 Mar 2008 21:56:56 -0800 Subject: [PATCH 7/7] git-clean: add tests for relative path This adds tests for recent change by Dmitry to fix the report "git clean" gives on removed paths, and also makes sure the command detects paths that is outside working tree. Signed-off-by: Junio C Hamano --- t/t7300-clean.sh | 52 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh index 4037142927..afccfc9973 100755 --- a/t/t7300-clean.sh +++ b/t/t7300-clean.sh @@ -89,6 +89,58 @@ test_expect_success 'git-clean with prefix' ' test -f build/lib.so ' + +test_expect_success 'git-clean with relative prefix' ' + + mkdir -p build docs && + touch a.out src/part3.c docs/manual.txt obj.o build/lib.so && + would_clean=$( + cd docs && + git clean -n ../src | + sed -n -e "s|^Would remove ||p" + ) && + test "$would_clean" = ../src/part3.c || { + echo "OOps <$would_clean>" + false + } +' + +test_expect_success 'git-clean with absolute path' ' + + mkdir -p build docs && + touch a.out src/part3.c docs/manual.txt obj.o build/lib.so && + would_clean=$( + cd docs && + git clean -n $(pwd)/../src | + sed -n -e "s|^Would remove ||p" + ) && + test "$would_clean" = ../src/part3.c || { + echo "OOps <$would_clean>" + false + } +' + +test_expect_success 'git-clean with out of work tree relative path' ' + + mkdir -p build docs && + touch a.out src/part3.c docs/manual.txt obj.o build/lib.so && + ( + cd docs && + test_must_fail git clean -n ../.. + ) +' + +test_expect_success 'git-clean with out of work tree absolute path' ' + + mkdir -p build docs && + touch a.out src/part3.c docs/manual.txt obj.o build/lib.so && + dd=$(cd .. && pwd) && + ( + cd docs && + test_must_fail git clean -n $dd + ) +' + test_expect_success 'git-clean -d with prefix and path' ' mkdir -p build docs src/feature &&