threads / patch / 10173

patchMake git-clean a builtin

Subject: [PATCH] Make git-clean a builtin

## tl;dr

9 messages between Oct 6, 2007 and Oct 8, 2007. Diffs are folded; open one to read it.

replies: 8people: 5as markdown or json

Shawn Bohrer· Oct 6, 2007, 20:54 UTC · lore

This replaces git-clean.sh with builtin-clean.c, and moves git-clean.sh to the examples.

Signed-off-by: Shawn Bohrer <shawn.bohrer@gmail.com>
---
 Makefile                                      |    3 +-
 builtin-clean.c                               |  177 +++++++++++++++++++++++++
 builtin.h                                     |    1 +
 git-clean.sh => contrib/examples/git-clean.sh |    0 
 git.c                                         |    1 +
 5 files changed, 181 insertions(+), 1 deletions(-)
 create mode 100644 builtin-clean.c
 rename git-clean.sh => contrib/examples/git-clean.sh (100%)
Show changes to 5 files +181 −1

Makefile, builtin-clean.c, builtin.h, git-clean.sh, git.c

diff --git a/Makefile b/Makefile
index 8db4dbe..2b3b8fb 100644
--- a/Makefile
+++ b/Makefile
@@ -206,7 +206,7 @@ BASIC_LDFLAGS =
 
 SCRIPT_SH = \
 	git-bisect.sh git-checkout.sh \
-	git-clean.sh git-clone.sh git-commit.sh \
+	git-clone.sh git-commit.sh \
 	git-fetch.sh \
 	git-ls-remote.sh \
 	git-merge-one-file.sh git-mergetool.sh git-parse-remote.sh \
@@ -327,6 +327,7 @@ BUILTIN_OBJS = \
 	builtin-check-attr.o \
 	builtin-checkout-index.o \
 	builtin-check-ref-format.o \
+	builtin-clean.o \
 	builtin-commit-tree.o \
 	builtin-count-objects.o \
 	builtin-describe.o \
diff --git a/builtin-clean.c b/builtin-clean.c
new file mode 100644
index 0000000..534707f
--- /dev/null
+++ b/builtin-clean.c
@@ -0,0 +1,177 @@
+/*
+ * "git clean" builtin command
+ *
+ * Copyright (C) 2007 Shawn Bohrer
+ *
+ * Based on git-clean.sh by Pavel Roskin
+ */
+
+#include "builtin.h"
+#include "cache.h"
+#include "run-command.h"
+
+static int disabled = 0;
+static int show_only = 0;
+static int remove_directories = 0;
+static int quiet = 0;
+static int ignored = 0;
+static int ignored_only = 0;
+
+static const char builtin_clean_usage[] =
+"git-clean [-d] [-f] [-n] [-q] [-x | -X] [--] <paths>...";
+
+static int git_clean_config(const char *var, const char *value)
+{
+	if (!strcmp(var, "clean.requireforce")) {
+		disabled = git_config_bool(var, value);
+	}
+	return 0;
+}
+
+static int remove_directory(const char *path)
+{
+	DIR *d;
+	struct dirent *dir;
+	d = opendir(path);
+	if (d) {
+		chdir(path);
+		while ((dir = readdir(d)) != NULL) {
+			if(strcmp( dir->d_name, ".") == 0 ||
+			   strcmp( dir->d_name, ".." ) == 0 )
+				continue;
+			if (dir->d_type == DT_DIR)
+				remove_directory(dir->d_name);
+			else
+				unlink(dir->d_name);
+		}
+	}
+	closedir(d);
+	chdir("..");
+	return rmdir(path);
+}
+
+int cmd_clean(int argc, const char **argv, const char *prefix)
+{
+	int i;
+	int j;
+	struct child_process cmd;
+	const char **argv_ls_files;
+	char *buf;
+	char path[1024];
+	FILE *cmd_fout;
+
+	git_config(git_clean_config);
+
+	for (i = 1; i < argc; i++) {
+		const char *arg = argv[i];
+
+		if (arg[0] != '-')
+			break;
+		if (!strcmp(arg, "--")) {
+			i++;
+			break;
+		}
+		if (!strcmp(arg, "-n")) {
+			show_only = 1;
+			disabled = 0;
+			continue;
+		}
+		if (!strcmp(arg, "-f")) {
+			disabled = 0;
+			continue;
+		}
+		if (!strcmp(arg, "-d")) {
+			remove_directories = 1;
+			continue;
+		}
+		if (!strcmp(arg, "-q")) {
+			quiet = 1;
+			continue;
+		}
+		if (!strcmp(arg, "-x")) {
+			ignored = 1;
+			continue;
+		}
+		if (!strcmp(arg, "-X")) {
+			ignored_only = 1;
+			continue;
+		}
+		usage(builtin_clean_usage);
+	}
+
+	if (ignored && ignored_only)
+		usage(builtin_clean_usage);
+
+	if (disabled) {
+		die("clean.requireForce set and -n or -f not given; refusing to clean");
+	}
+
+	/* Paths (argc - i) + 8 (Possible arguments)*/
+	argv_ls_files = xmalloc((argc - i + 8) * sizeof(const char *));
+	argv_ls_files[0] = "ls-files";
+	argv_ls_files[1] = "--others";
+	argv_ls_files[2] = "--directory";
+	j = 3;
+	if (!ignored) {
+		argv_ls_files[j++] = "--exclude-per-directory=.gitignore";
+		if (ignored_only)
+			argv_ls_files[j++] = "--ignored";
+		if (!access(git_path("info/exclude"), F_OK)) {
+			char *exclude_path = git_path("info/exclude");
+			int len = strlen(exclude_path);
+			buf = (char*)malloc(len+16);
+			sprintf(buf, "--exclude-from=%s", exclude_path);
+			argv_ls_files[j++] = buf;
+		}
+	}
+	argv_ls_files[j++] = "--";
+	/* Add remaining paths passed in as arguments */
+	if (argc - i)
+		memcpy(argv_ls_files + j++, argv + i, (argc - i) * sizeof(const char *));
+	argv_ls_files[j + argc - i] = NULL;
+
+	memset(&cmd, 0, sizeof(cmd));
+	cmd.argv = argv_ls_files;
+	cmd.git_cmd = 1;
+	cmd.out = -1;
+	if (start_command(&cmd))
+		die("Could not run sub-command: git ls-files");
+
+	cmd_fout = fdopen(cmd.out, "r");
+	while (fgets(path, sizeof(path), cmd_fout) != NULL) {
+		struct stat st;
+		char *p;
+		p = strrchr(path, '\n');
+		if ( p != NULL )
+			*p = '\0';
+		if (!lstat(path, &st) && (S_ISDIR(st.st_mode))) {
+			if (show_only && remove_directories) {
+				printf("Would remove %s\n", path);
+			} else if (quiet && remove_directories) {
+				remove_directory(path);
+			} else if (remove_directories) {
+				printf("Removing %s\n", path);
+				remove_directory(path);
+			} else if (show_only) {
+				printf("Would not remove %s\n", path);
+			} else {
+				printf("Not removing %s\n", path);
+			}
+		} else {
+			if (show_only) {
+				printf("Would remove %s\n", path);
+				continue;
+			} else if (!quiet) {
+				printf("Removing %s\n", path);
+			}
+			unlink(path);
+		}
+	}
+
+	fclose(cmd_fout);
+	finish_command(&cmd);
+	if (!ignored && !access(git_path("info/exclude"), F_OK))
+		free(buf);
+	free(argv_ls_files);
+	return 0;
+}
diff --git a/builtin.h b/builtin.h
index d6f2c76..8c112f3 100644
--- a/builtin.h
+++ b/builtin.h
@@ -23,6 +23,7 @@ extern int cmd_check_attr(int argc, const char **argv, const char *prefix);
 extern int cmd_check_ref_format(int argc, const char **argv, const char *prefix);
 extern int cmd_cherry(int argc, const char **argv, const char *prefix);
 extern int cmd_cherry_pick(int argc, const char **argv, const char *prefix);
+extern int cmd_clean(int argc, const char **argv, const char *prefix);
 extern int cmd_commit_tree(int argc, const char **argv, const char *prefix);
 extern int cmd_count_objects(int argc, const char **argv, const char *prefix);
 extern int cmd_describe(int argc, const char **argv, const char *prefix);
diff --git a/git-clean.sh b/contrib/examples/git-clean.sh
similarity index 100%
rename from git-clean.sh
rename to contrib/examples/git-clean.sh
diff --git a/git.c b/git.c
index 9eaca1d..cda6344 100644
--- a/git.c
+++ b/git.c
@@ -320,6 +320,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "check-attr", cmd_check_attr, RUN_SETUP | NEED_WORK_TREE },
 		{ "cherry", cmd_cherry, RUN_SETUP },
 		{ "cherry-pick", cmd_cherry_pick, RUN_SETUP | NEED_WORK_TREE },
+		{ "clean", cmd_clean, RUN_SETUP },
 		{ "commit-tree", cmd_commit_tree, RUN_SETUP },
 		{ "config", cmd_config },
 		{ "count-objects", cmd_count_objects, RUN_SETUP },
-- 
1.5.3.GIT
Frank Lichtenheld· Oct 6, 2007, 21:52 UTC · re: Shawn Bohrer · lore

Re: [PATCH] Make git-clean a builtin

On Sat, Oct 06, 2007 at 03:54:06PM -0500, Shawn Bohrer wrote:
Show 21 quoted lines
> +static int remove_directory(const char *path)
> +{
> +	DIR *d;
> +	struct dirent *dir;
> +	d = opendir(path);
> +	if (d) {
> +		chdir(path);
> +		while ((dir = readdir(d)) != NULL) {
> +			if(strcmp( dir->d_name, ".") == 0 ||
> +			   strcmp( dir->d_name, ".." ) == 0 )
> +				continue;
> +			if (dir->d_type == DT_DIR)
> +				remove_directory(dir->d_name);
> +			else
> +				unlink(dir->d_name);
> +		}
> +	}
> +	closedir(d);
> +	chdir("..");
> +	return rmdir(path);
> +}

The unconditional chdir(..) after the conditional chdir(path) seems like asking for trouble to me...

Show 6 quoted lines
> +	while (fgets(path, sizeof(path), cmd_fout) != NULL) {
> +		struct stat st;
> +		char *p;
> +		p = strrchr(path, '\n');
> +		if ( p != NULL )
> +			*p = '\0';

What happens in case p == NULL? It simply tries to remove the partial path?

> +	fclose(cmd_fout);
> +	finish_command(&cmd);
> +	if (!ignored && !access(git_path("info/exclude"), F_OK))
> +		free(buf);

There is a race condition here of the value of access() changes between the two calls. Not one likely to trigger but it should be easy to avoid alltogether.

> +	free(argv_ls_files);
> +	return 0;
> +}
Gruesse,
-- 
Frank Lichtenheld <frank@lichtenheld.de>
www: http://www.djpig.de/
Shawn Bohrer· Oct 7, 2007, 01:13 UTC · re: Frank Lichtenheld · lore

Re: [PATCH] Make git-clean a builtin

On Sat, Oct 06, 2007 at 11:52:53PM +0200, Frank Lichtenheld wrote:
Show 25 quoted lines
> On Sat, Oct 06, 2007 at 03:54:06PM -0500, Shawn Bohrer wrote:
> > +static int remove_directory(const char *path)
> > +{
> > +	DIR *d;
> > +	struct dirent *dir;
> > +	d = opendir(path);
> > +	if (d) {
> > +		chdir(path);
> > +		while ((dir = readdir(d)) != NULL) {
> > +			if(strcmp( dir->d_name, ".") == 0 ||
> > +			   strcmp( dir->d_name, ".." ) == 0 )
> > +				continue;
> > +			if (dir->d_type == DT_DIR)
> > +				remove_directory(dir->d_name);
> > +			else
> > +				unlink(dir->d_name);
> > +		}
> > +	}
> > +	closedir(d);
> > +	chdir("..");
> > +	return rmdir(path);
> > +}
> 
> The unconditional chdir(..) after the conditional chdir(path) seems like
> asking for trouble to me...
Agreed, I'm not sure what I was thinking there.
Show 9 quoted lines
> > +	while (fgets(path, sizeof(path), cmd_fout) != NULL) {
> > +		struct stat st;
> > +		char *p;
> > +		p = strrchr(path, '\n');
> > +		if ( p != NULL )
> > +			*p = '\0';
> 
> What happens in case p == NULL? It simply tries to remove the partial
> path?

If p == NULL then the path didn't have a EOL character. This shouldn't ever really happen since fgets() leaves the EOL character as part of the string, and it is processing the output of git-ls-files which will provide one path per line. If it does happen for some reason then either we will happily remove the file/directory, or if the path is garbage then we will simply fail to remove anything.

Show 8 quoted lines
> > +	fclose(cmd_fout);
> > +	finish_command(&cmd);
> > +	if (!ignored && !access(git_path("info/exclude"), F_OK))
> > +		free(buf);
> 
> There is a race condition here of the value of access() changes between
> the two calls. Not one likely to trigger but it should be easy to avoid
> alltogether.

Yes, though unlikely I agree this wasn't very smart in the first place so I fixed it and will send reworked patch soon.

-- Shawn

Jeff King· Oct 8, 2007, 02:04 UTC · re: Shawn Bohrer · lore

Re: [PATCH] Make git-clean a builtin

On Sat, Oct 06, 2007 at 03:54:06PM -0500, Shawn Bohrer wrote:
Show 6 quoted lines
> +static int remove_directory(const char *path)
> +{
> [...]
> +		chdir(path);
> [...]
> +	chdir("..");

This doesn't always put you back where you started, due to symlinks. For example:

cat >foo.c <<'EOF'
int main(int argc, char **argv) {
  chdir(argv[1]);
  chdir("..");
  execlp("ls", 0);
}
EOF
gcc -o foo foo.c
ln -s /tmp sub
./foo sub

will show that you end up in the root directory. Something like this is more robust:

  fd = open(".", O_RDONLY);
  chdir(path);
  ...
  fchdir(fd);

In general, you shouldn't end up there because you don't actually recurse for symlinks, but there is a race condition (and losing it means you start recursively removing unintended directories -- oops).

On top of which, this line from the same function isn't very portable:
+                       if (dir->d_type == DT_DIR)

since POSIX specifies nothing but dir->d_name (Solaris, for example, doesn't define d_type). You need to stat the file.

All of that being said, I think a lot of this is already done in dir.[ch]. At the very least, you should be able to use remove_dir_recursively, and for bonus points you can get rid of the start_command call to ls-files by just walking the dir tree yourself. I don't know if the latter is required, but it's nice when the C-ification actually cleans up a bit and uses the internal C interfaces (which are more efficient and often more clear to read) rather than just converting shell to C.

-Peff
Jeff King· Oct 8, 2007, 02:08 UTC · re: Jeff King · lore

Re: [PATCH] Make git-clean a builtin

On Sun, Oct 07, 2007 at 10:04:35PM -0400, Jeff King wrote:
> This doesn't always put you back where you started, due to symlinks. For
> example:
> [and other complaints]

Oops, didn't see that there was another thread for the reworked patch. My comments still stand, but it looks like others have weighed in as well. Sorry.

-Peff
Linus Torvalds· Oct 8, 2007, 02:17 UTC · re: Jeff King · lore

Re: [PATCH] Make git-clean a builtin

On Sun, 7 Oct 2007, Jeff King wrote:
Show 5 quoted lines
> 
>   fd = open(".", O_RDONLY);
>   chdir(path);
>   ...
>   fchdir(fd);
fchdir() is not portable.

I think it would be better to not chdir() at all. Yes, that means having to prepend the prefix to the names, but that is what git generally does (for that - and other - reasons).

		Linus
Jeff King· Oct 8, 2007, 02:22 UTC · re: Linus Torvalds · lore

Re: [PATCH] Make git-clean a builtin

On Sun, Oct 07, 2007 at 07:17:50PM -0700, Linus Torvalds wrote:
Show 5 quoted lines
> fchdir() is not portable.
> 
> I think it would be better to not chdir() at all. Yes, that means having 
> to prepend the prefix to the names, but that is what git generally does 
> (for that - and other - reasons).

I was using the "even Solaris has it!" test, but yes, it's not POSIX. I don't know how common it actually is (for curiosity's sake, do you know of a common platform that lacks it?). But I do agree that just building up the path and avoiding the chdir at all is simple and portable.

-Peff
Johannes Sixt· Oct 8, 2007, 06:37 UTC · re: Jeff King · lore

Re: [PATCH] Make git-clean a builtin

Jeff King schrieb:
Show 7 quoted lines
> On Sun, Oct 07, 2007 at 07:17:50PM -0700, Linus Torvalds wrote:
> 
>> fchdir() is not portable.
> 
> I was using the "even Solaris has it!" test, but yes, it's not POSIX. I
> don't know how common it actually is (for curiosity's sake, do you know
> of a common platform that lacks it?).
Windows. ;)
-- Hannes
Linus Torvalds· Oct 8, 2007, 18:27 UTC · re: Johannes Sixt · lore

Re: [PATCH] Make git-clean a builtin

On Mon, 8 Oct 2007, Johannes Sixt wrote:
Show 10 quoted lines
> Jeff King schrieb:
> > On Sun, Oct 07, 2007 at 07:17:50PM -0700, Linus Torvalds wrote:
> > 
> > > fchdir() is not portable.
> > 
> > I was using the "even Solaris has it!" test, but yes, it's not POSIX. I
> > don't know how common it actually is (for curiosity's sake, do you know
> > of a common platform that lacks it?).
> 
> Windows. ;)

Not just windows. Almost no "older" Unix has it. Including older versions of Linux.

		Linus

← back to recent threads