threads / bug / 12513

[bug] git-clean

Subject: [bug] git-clean

## tl;dr

7 messages between Mar 5, 2008 and Mar 6, 2008.

replies: 6people: 3as markdown or json

Pierre Habouzit· Mar 5, 2008, 09:40 UTC · lore
  Sorry I just don't have the time to track it right now, I just experienced
that on the current next, but it seems to be the same in master.

$ git st # On branch master # Untracked files: # (use "git add <file>..." to include in what will be committed) # # ../a.diff nothing added to commit but untracked files present (use "git add" to track) $ git clean .. Removing ff

I assume something is wrong in in the display only, because a.diff was actually removed. It's probably a ridiculously easy bug to find.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org
Dmitry Potapov· Mar 5, 2008, 13:47 UTC · re: Pierre Habouzit · lore

[PATCH] 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() 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>
---
 builtin-clean.c |   31 +++++++++++++------------------
 wt-status.c     |    4 ++--
 wt-status.h     |    2 ++
 3 files changed, 17 insertions(+), 20 deletions(-)
diff --git a/builtin-clean.c b/builtin-clean.c
index 3b220d5..3d75ad6 100644
--- a/builtin-clean.c
+++ b/builtin-clean.c
@@ -34,7 +34,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 +57,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 +74,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();
 
@@ -133,40 +133,35 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 		}
 
 		if (S_ISDIR(st.st_mode)) {
+			qname = quote_path(directory.buf, -1, &buf, prefix);
 			strbuf_addstr(&directory, ent->name);
 			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
Dmitry Potapov· Mar 5, 2008, 14:17 UTC · re: Pierre Habouzit · lore

[PATCH] 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() 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>
---
I resend this patch, because it got filtered by vger.kernel.org
when I used git-send-email from 1.5.4.
 builtin-clean.c |   31 +++++++++++++------------------
 wt-status.c     |    4 ++--
 wt-status.h     |    2 ++
 3 files changed, 17 insertions(+), 20 deletions(-)
diff --git a/builtin-clean.c b/builtin-clean.c
index 3b220d5..3d75ad6 100644
--- a/builtin-clean.c
+++ b/builtin-clean.c
@@ -34,7 +34,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 +57,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 +74,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();
 
@@ -133,40 +133,35 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 		}
 
 		if (S_ISDIR(st.st_mode)) {
+			qname = quote_path(directory.buf, -1, &buf, prefix);
 			strbuf_addstr(&directory, ent->name);
 			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
Pierre Habouzit· Mar 5, 2008, 15:53 UTC · re: Dmitry Potapov · lore

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

On Wed, Mar 05, 2008 at 02:17:20PM +0000, Dmitry Potapov wrote:
Show 9 quoted lines
> 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>
> ---
> I resend this patch, because it got filtered by vger.kernel.org
> when I used git-send-email from 1.5.4.
  It didn't FWIW.
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org
Pierre Habouzit· Mar 5, 2008, 15:59 UTC · re: Pierre Habouzit · lore

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

On mer, mar 05, 2008 at 03:53:59 +0000, Pierre Habouzit wrote:
Show 12 quoted lines
> On Wed, Mar 05, 2008 at 02:17:20PM +0000, Dmitry Potapov wrote:
> > 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>
> > ---
> > I resend this patch, because it got filtered by vger.kernel.org
> > when I used git-send-email from 1.5.4.
> 
>   It didn't FWIW.
  And as far as it goes for your patch, it misses a #include
"wt-status.h" and for me it gives:
    git clean ..
    Removing ../
    Removing ../git-clean.patch
    Removing ../stFdw8KY
For a tree that had those unclean entries.
    ../git-clean.patch
    ../stFdw8KY
  Removing ../ is clearly wrong both times: it didn't really did that
(my current dir was the 't/' dir in the git repository for that) and I
didn't specified 'git clean -d'.
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org
Dmitry Potapov· Mar 5, 2008, 23:41 UTC · re: Pierre Habouzit · lore

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

On Wed, Mar 05, 2008 at 04:59:43PM +0100, Pierre Habouzit wrote:
> 
>   And as far as it goes for your patch, it misses a #include
> "wt-status.h" 
My bad... I will correct and resend the patch now.
Show 15 quoted lines
> and for me it gives:
> 
>     git clean ..
>     Removing ../
>     Removing ../git-clean.patch
>     Removing ../stFdw8KY
> 
> For a tree that had those unclean entries.
>     ../git-clean.patch
>     ../stFdw8KY
> 
> 
>   Removing ../ is clearly wrong both times: it didn't really did that
> (my current dir was the 't/' dir in the git repository for that) and I
> didn't specified 'git clean -d'.

Hmm... when I ran 'git clean' inside of the 't/' directory in the git repository, it did not print any message about removing ../. However, when I did the same in a clean repository where the 't/' directory contained only untracked files then the 't/' directory and all files in it were actually removed even I did not specify '-d'. This happens on both 1.5.4 and master versions. IMHO, removing a directory without '-d' is incorrect.

In addition, when I ran 'git clean -n ..' at the top a repository, the current master version of Git though printed the error that '..' is outside the repository, still exited with 0. When I test this with 1.5.4, git clean exists with 128 after printing the error. Bisect blames d089ebaad5315325d67db30176df1bbd7754fda9 for changing the exit code to 0.

I will look at git-clean more tomorrow.
Dmitry
Junio C Hamano· Mar 6, 2008, 22:54 UTC · re: Dmitry Potapov · lore

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

Dmitry Potapov <dpotapov@gmail.com> writes:
Show 5 quoted lines
> In addition, when I ran 'git clean -n ..' at the top a repository, the
> current master version of Git though printed the error that '..' is
> outside the repository, still exited with 0. When I test this with
> 1.5.4, git clean exists with 128 after printing the error. Bisect blames
> d089ebaad5315325d67db30176df1bbd7754fda9 for changing the exit code to 0.

You need a change similar to the one that updates builtin-ls-files.c in d089eba (setup: sanitize absolute and funny paths in get_pathspec()). The commit should have adjusted builtin-clean.c but was overlooked.

When you give a set of paths that includes bogus ones, get_pathspec() issues an error message and gives back a pathspec that does not contain the entries corresponding to the bogus ones, and that is an indication for a caller that wants to refuse to operate upon bogus input. Other callers that want to issue an error diagnosis and handle the remaining valid input do not have to die, but "git-clean" is destructive so it would be sensible to error out the whole operation if you see the user input was bogus.

commit d089ebaad5315325d67db30176df1bbd7754fda9
Author: Junio C Hamano <gitster@pobox.com>
    setup: sanitize absolute and funny paths in get_pathspec()
diff --git a/builtin-ls-files.c b/builtin-ls-files.c
index 0f0ab2d..3801cf4 100644
--- a/builtin-ls-files.c
+++ b/builtin-ls-files.c
@@ -572,8 +572,17 @@ 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 (pathspec) {
+		if (argc != i) {
+			int cnt;
+			for (cnt = 0; pathspec[cnt]; cnt++)
+				;
+			if (cnt != (argc - i))
+				exit(1); /* error message already given */
+		}
 		prefix = verify_pathspec(prefix);
+	} else if (argc != i)
+		exit(1); /* error message already given */
 
 	/* Treat unmatching pathspec elements as errors */
 	if (pathspec && error_unmatch) {

← back to recent threads