# [PATCH] Make git-clean a builtin

9 messages from 2007-10-06 to 2007-10-08. Participants: Shawn Bohrer, Frank Lichtenheld, Jeff King, Linus Torvalds, Johannes Sixt.
Thread: https://gitlist.dev/t/10173

## Shawn Bohrer, 2007-10-06 20:54

Subject: [PATCH] Make git-clean a builtin
Message-ID: <11917040461528-git-send-email-shawn.bohrer@gmail.com>
URL: https://gitlist.dev/e/11917040461528-git-send-email-shawn.bohrer%40gmail.com

```
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%)

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, 2007-10-06 21:52

Subject: Re: [PATCH] Make git-clean a builtin
Message-ID: <20071006215253.GX31659@planck.djpig.de>
URL: https://gitlist.dev/e/20071006215253.GX31659%40planck.djpig.de
In-Reply-To: <11917040461528-git-send-email-shawn.bohrer@gmail.com>

```
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...

> +	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, 2007-10-07 01:13

Subject: Re: [PATCH] Make git-clean a builtin
Message-ID: <20071007011331.GC5642@mediacenter.austin.rr.com>
URL: https://gitlist.dev/e/20071007011331.GC5642%40mediacenter.austin.rr.com
In-Reply-To: <20071006215253.GX31659@planck.djpig.de>

```
On Sat, Oct 06, 2007 at 11:52:53PM +0200, Frank Lichtenheld wrote:
> 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.

> > +	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.

> > +	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, 2007-10-08 02:04

Subject: Re: [PATCH] Make git-clean a builtin
Message-ID: <20071008020435.GA20050@coredump.intra.peff.net>
URL: https://gitlist.dev/e/20071008020435.GA20050%40coredump.intra.peff.net
In-Reply-To: <11917040461528-git-send-email-shawn.bohrer@gmail.com>

```
On Sat, Oct 06, 2007 at 03:54:06PM -0500, Shawn Bohrer wrote:

> +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, 2007-10-08 02:08

Subject: Re: [PATCH] Make git-clean a builtin
Message-ID: <20071008020815.GB20050@coredump.intra.peff.net>
URL: https://gitlist.dev/e/20071008020815.GB20050%40coredump.intra.peff.net
In-Reply-To: <20071008020435.GA20050@coredump.intra.peff.net>

```
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, 2007-10-08 02:17

Subject: Re: [PATCH] Make git-clean a builtin
Message-ID: <alpine.LFD.0.999.0710071916510.23684@woody.linux-foundation.org>
URL: https://gitlist.dev/e/alpine.LFD.0.999.0710071916510.23684%40woody.linux-foundation.org
In-Reply-To: <20071008020435.GA20050@coredump.intra.peff.net>

```


On Sun, 7 Oct 2007, Jeff King wrote:
> 
>   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, 2007-10-08 02:22

Subject: Re: [PATCH] Make git-clean a builtin
Message-ID: <20071008022205.GA21277@coredump.intra.peff.net>
URL: https://gitlist.dev/e/20071008022205.GA21277%40coredump.intra.peff.net
In-Reply-To: <alpine.LFD.0.999.0710071916510.23684@woody.linux-foundation.org>

```
On Sun, Oct 07, 2007 at 07:17:50PM -0700, Linus Torvalds wrote:

> 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, 2007-10-08 06:37

Subject: Re: [PATCH] Make git-clean a builtin
Message-ID: <4709D027.3090003@viscovery.net>
URL: https://gitlist.dev/e/4709D027.3090003%40viscovery.net
In-Reply-To: <20071008022205.GA21277@coredump.intra.peff.net>

```
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. ;)

-- Hannes

```

## Linus Torvalds, 2007-10-08 18:27

Subject: Re: [PATCH] Make git-clean a builtin
Message-ID: <alpine.LFD.0.999.0710081126340.4964@woody.linux-foundation.org>
URL: https://gitlist.dev/e/alpine.LFD.0.999.0710081126340.4964%40woody.linux-foundation.org
In-Reply-To: <4709D027.3090003@viscovery.net>

```


On Mon, 8 Oct 2007, Johannes Sixt wrote:

> 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

```