[PATCH] Make git-clean a builtin

[PATCH] Make git-clean a builtin

[Shawn Bohrer]

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

Re: [PATCH] Make git-clean a builtin

[Frank Lichtenheld]

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/

Re: [PATCH] Make git-clean a builtin

[Shawn Bohrer]

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

Re: [PATCH] Make git-clean a builtin

[Jeff King]

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

Re: [PATCH] Make git-clean a builtin

[Jeff King]

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

Re: [PATCH] Make git-clean a builtin

[Linus Torvalds]

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

Re: [PATCH] Make git-clean a builtin

[Jeff King]

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

Re: [PATCH] Make git-clean a builtin

[Johannes Sixt]

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

Re: [PATCH] Make git-clean a builtin

[Linus Torvalds]

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

[PATCH] Make git-clean a builtin

[Shawn Bohrer]

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

Reworked patch with fixes to Frank's suggestions.

 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..4890c2d
--- /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);
+		}
+		chdir("..");
+	}
+	closedir(d);
+	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 = NULL;
+	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 (buf != NULL)
+		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

Re: [PATCH] Make git-clean a builtin

[Linus Torvalds]

On Sat, 6 Oct 2007, Shawn Bohrer wrote:
>
> This replaces git-clean.sh with builtin-clean.c, and moves git-clean.sh to the
> examples.

This looks better, but I think you'd be even better off actually using the 
"read_directory()" interface directly, instead of exec'ing off "git 
ls-files" and parsing the line output.

I also would still worry a bit about 'chdir(x)' and 'chdir("..")', because 
quite frankly, they are *not* mirrors of each other (think symlinks, but 
also error behaviour due to directories that might be non-executable). 
Now, admittedly, if a directory isn't executable, I can imagine other git 
things having problems (anybody want to test?), but that whole pattern is 
just very fragile and not very reliable.

		Linus

Re: [PATCH] Make git-clean a builtin

[Shawn Bohrer]

Thanks for the input.

On Sat, Oct 06, 2007 at 06:31:36PM -0700, Linus Torvalds wrote:
> This looks better, but I think you'd be even better off actually using the 
> "read_directory()" interface directly, instead of exec'ing off "git 
> ls-files" and parsing the line output.

Perhaps, I'll take a look at how git-ls-files does it and see if I can
do that directly.  Since I'm new to git (and C) it will probably take me
a while to re-implement though.

> I also would still worry a bit about 'chdir(x)' and 'chdir("..")', because 
> quite frankly, they are *not* mirrors of each other (think symlinks, but 
> also error behaviour due to directories that might be non-executable). 
> Now, admittedly, if a directory isn't executable, I can imagine other git 
> things having problems (anybody want to test?), but that whole pattern is 
> just very fragile and not very reliable.

Yes it does seem fragile, but 'chdir("-")' doesn't work in C and I
couldn't find any equivalents.  I actually did think about symlinks, and
my code does do the right thing since I test if it is a directory before
doing the 'chdir(x)'. Symlinks are therefore treated as normal files and
removed.

I did not think about non-executable directories, and you are correct
that my code will fail to remove a directory if it is non-executable. I
also tested a git-ls-files with non-executable directories, and it will
fail to show you any files that are more than one level deep for
example:

|-- docs
|   |-- contributing
|   |   `-- patches.txt
|   `-- manual.txt

If docs is non-executable it will only return 'docs/manual.txt'

--
Shawn

Re: [PATCH] Make git-clean a builtin

[rae l]

On 10/7/07, Shawn Bohrer <shawn.bohrer@gmail.com> wrote:
> Thanks for the input.
>
> On Sat, Oct 06, 2007 at 06:31:36PM -0700, Linus Torvalds wrote:
> > This looks better, but I think you'd be even better off actually using the
> > "read_directory()" interface directly, instead of exec'ing off "git
> > ls-files" and parsing the line output.
>
> Perhaps, I'll take a look at how git-ls-files does it and see if I can
> do that directly.  Since I'm new to git (and C) it will probably take me
> a while to re-implement though.
>
> > I also would still worry a bit about 'chdir(x)' and 'chdir("..")', because
> > quite frankly, they are *not* mirrors of each other (think symlinks, but
> > also error behaviour due to directories that might be non-executable).
> > Now, admittedly, if a directory isn't executable, I can imagine other git
> > things having problems (anybody want to test?), but that whole pattern is
> > just very fragile and not very reliable.
>
> Yes it does seem fragile, but 'chdir("-")' doesn't work in C and I
> couldn't find any equivalents.  I actually did think about symlinks, and
> my code does do the right thing since I test if it is a directory before
> doing the 'chdir(x)'. Symlinks are therefore treated as normal files and
> removed.
'chdir -' is just supported by the shell, and the C interface could
use chdir(OLDPWD).

Re: [PATCH] Make git-clean a builtin

[Johannes Schindelin]

Hi,

On Sat, 6 Oct 2007, Shawn Bohrer wrote:

> +static int remove_directory(const char *path)

Please use remove_directory_recursively(), introduced in commit 
7155b727c9baae9ef6d7829370aefc09c4ab64e2 to 'next'.

I have not looked at the rest of your patch yet.

Ciao,
Dscho

[PATCH] Make git-clean a builtin

[Shawn Bohrer]

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

Rewritten to use remove_directory_recursively() per Dscho's suggestion.  Patch
is now based ontop of 'next'. 

 Makefile                                      |    3 +-
 builtin-clean.c                               |  161 +++++++++++++++++++++++++
 builtin.h                                     |    1 +
 git-clean.sh => contrib/examples/git-clean.sh |    0 
 git.c                                         |    1 +
 5 files changed, 165 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 62bdac6..bed4c78 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-ls-remote.sh \
 	git-merge-one-file.sh git-mergetool.sh git-parse-remote.sh \
 	git-pull.sh git-rebase.sh git-rebase--interactive.sh \
@@ -324,6 +324,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..af61de0
--- /dev/null
+++ b/builtin-clean.c
@@ -0,0 +1,161 @@
+/*
+ * "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"
+#include "dir.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;
+}
+
+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 = NULL;
+	char path[1024];
+	FILE *cmd_fout;
+	struct strbuf dir;
+
+	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");
+
+	strbuf_init(&dir, 0);
+	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))) {
+			strbuf_addstr(&dir, path);
+			if (show_only && remove_directories) {
+				printf("Would remove %s\n", dir.buf);
+			} else if (quiet && remove_directories) {
+				remove_dir_recursively(&dir, 0);
+			} else if (remove_directories) {
+				printf("Removing %s\n", path);
+				remove_dir_recursively(&dir, 0);
+			} else if (show_only) {
+				printf("Would not remove %s\n", dir.buf);
+			} else {
+				printf("Not removing %s\n", dir.buf);
+			}
+			strbuf_reset(&dir);
+		} else {
+			if (show_only) {
+				printf("Would remove %s\n", path);
+				continue;
+			} else if (!quiet) {
+				printf("Removing %s\n", path);
+			}
+			unlink(path);
+		}
+	}
+
+	strbuf_release(&dir);
+	fclose(cmd_fout);
+	finish_command(&cmd);
+	if (buf != NULL)
+		free(buf);
+	free(argv_ls_files);
+	return 0;
+}
diff --git a/builtin.h b/builtin.h
index 65cc0fb..cdefdc0 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 d7c6bca..4e39169 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

Re: [PATCH] Make git-clean a builtin

[Johannes Schindelin]

Hi,

On Sun, 7 Oct 2007, Shawn Bohrer wrote:

> +	if (ignored && ignored_only)
> +		usage(builtin_clean_usage);

Maybe a helpful message in that case, too?  (It is not apparent from the 
usage what the user has done wrong here.)

> +	if (disabled) {
> +		die("clean.requireForce set and -n or -f not given; refusing to clean");
> +	}

Please lose the curly brackets here.  They are absolutely useless, and the 
rest of the git source code avoids unnecessary curly brackets.

> +	/* 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";
> [...]

As Linus already noted, it is probably easy, and much more efficient, to 
call read_directory() here.  The best example how to use 
read_directory() is... builtin-ls-files.c.

> diff --git a/git.c b/git.c
> index d7c6bca..4e39169 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 },

You definitely want to have NEED_WORK_TREE here, too.

Ciao,
Dscho

[RFC] Second attempt at making git-clean a builtin

[Shawn Bohrer]

I've taken all of the comments I received from my previous attempt see:

http://marc.info/?l=git&m=119181975419521&w=2

With these new changes in place my new git-clean passes all of the
original tests as well as the new tests I've added.  While looking at
how git-ls-files walks the tree there were some things that didn't quite
understand, or thought might be unnecessary so there may be some things I
missed.  For example I'm still not quite sure what verify_pathspec()
does.

I did however notice what I would call a bug in the behavior of
git-ls-files and therefore the current git-clean.sh.  With the current
git-clean if you have two directories that contain only untracked files,
for example docs/ and examples/ running:

git clean docs/ examples/

will not remove either directory.  Instead you must use the -d
parameter.  To me this makes sense, however if you run:

git clean docs/

it will remove the docs directory without using the -d parameter.  My
patch is at least consistent in that it requires the -d in both cases.

[PATCH] Add more tests for git-clean

[Shawn Bohrer]

Signed-off-by: Shawn Bohrer <shawn.bohrer@gmail.com>
---
 t/t7300-clean.sh |  109 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 109 insertions(+), 0 deletions(-)

diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index 8697213..d74c11c 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -39,6 +39,97 @@ test_expect_success 'git-clean' '
 
 '
 
+test_expect_success 'git-clean src/' '
+
+	mkdir -p build docs &&
+	touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
+	git-clean src/ &&
+	test -f Makefile &&
+	test -f README &&
+	test -f src/part1.c &&
+	test -f src/part2.c &&
+	test -f a.out &&
+	test ! -f src/part3.c &&
+	test -f docs/manual.txt &&
+	test -f obj.o &&
+	test -f build/lib.so
+
+'
+
+test_expect_success 'git-clean src/ src/' '
+
+	mkdir -p build docs &&
+	touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
+	git-clean src/ src/ &&
+	test -f Makefile &&
+	test -f README &&
+	test -f src/part1.c &&
+	test -f src/part2.c &&
+	test -f a.out &&
+	test ! -f src/part3.c &&
+	test -f docs/manual.txt &&
+	test -f obj.o &&
+	test -f build/lib.so
+
+'
+
+test_expect_success 'git-clean with prefix' '
+
+	mkdir -p build docs &&
+	touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
+	cd src/ &&
+	git-clean &&
+	cd - &&
+	test -f Makefile &&
+	test -f README &&
+	test -f src/part1.c &&
+	test -f src/part2.c &&
+	test -f a.out &&
+	test ! -f src/part3.c &&
+	test -f docs/manual.txt &&
+	test -f obj.o &&
+	test -f build/lib.so
+
+'
+test_expect_success 'git-clean -d with prefix and path' '
+
+	mkdir -p build docs src/feature &&
+	touch a.out src/part3.c src/feature/file.c docs/manual.txt obj.o build/lib.so &&
+	cd src/ &&
+	git-clean -d feature/ &&
+	cd - &&
+	test -f Makefile &&
+	test -f README &&
+	test -f src/part1.c &&
+	test -f src/part2.c &&
+	test -f a.out &&
+	test -f src/part3.c &&
+	test ! -f src/feature/file.c &&
+	test -f docs/manual.txt &&
+	test -f obj.o &&
+	test -f build/lib.so
+
+'
+
+test_expect_success 'git-clean symbolic link' '
+
+	mkdir -p build docs &&
+	touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
+	ln -s docs/manual.txt src/part4.c
+	git-clean &&
+	test -f Makefile &&
+	test -f README &&
+	test -f src/part1.c &&
+	test -f src/part2.c &&
+	test ! -f a.out &&
+	test ! -f src/part3.c &&
+	test ! -f src/part4.c &&
+	test -f docs/manual.txt &&
+	test -f obj.o &&
+	test -f build/lib.so
+
+'
+
 test_expect_success 'git-clean -n' '
 
 	mkdir -p build docs &&
@@ -73,6 +164,24 @@ test_expect_success 'git-clean -d' '
 
 '
 
+test_expect_success 'git-clean -d src/ examples/' '
+
+	mkdir -p build docs examples &&
+	touch a.out src/part3.c docs/manual.txt obj.o build/lib.so examples/1.c &&
+	git-clean -d src/ examples/ &&
+	test -f Makefile &&
+	test -f README &&
+	test -f src/part1.c &&
+	test -f src/part2.c &&
+	test -f a.out &&
+	test ! -f src/part3.c &&
+	test ! -f examples/1.c &&
+	test -f docs/manual.txt &&
+	test -f obj.o &&
+	test -f build/lib.so
+
+'
+
 test_expect_success 'git-clean -x' '
 
 	mkdir -p build docs &&
-- 
1.5.3.GIT

[PATCH] Make git-clean a builtin

[Shawn Bohrer]

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                               |  157 +++++++++++++++++++++++++
 builtin.h                                     |    1 +
 git-clean.sh => contrib/examples/git-clean.sh |    0 
 git.c                                         |    1 +
 5 files changed, 161 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 3ec1876..fad49b2 100644
--- a/Makefile
+++ b/Makefile
@@ -209,7 +209,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-ls-remote.sh \
 	git-merge-one-file.sh git-mergetool.sh git-parse-remote.sh \
 	git-pull.sh git-rebase.sh git-rebase--interactive.sh \
@@ -326,6 +326,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..4141eb4
--- /dev/null
+++ b/builtin-clean.c
@@ -0,0 +1,157 @@
+/*
+ * "git clean" builtin command
+ *
+ * Copyright (C) 2007 Shawn Bohrer
+ *
+ * Based on git-clean.sh by Pavel Roskin
+ */
+
+#include "builtin.h"
+#include "cache.h"
+#include "dir.h"
+
+static int disabled = 1;
+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;
+}
+
+int cmd_clean(int argc, const char **argv, const char *prefix)
+{
+	int i, j;
+	struct strbuf directory;
+	struct dir_struct dir;
+	const char *path = ".";
+	const char *base = "";
+	int baselen = 0;
+	static const char **pathspec;
+
+	memset(&dir, 0, sizeof(dir));
+	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;
+			dir.show_ignored =1;
+			dir.exclude_per_dir = ".gitignore";
+			continue;
+		}
+		usage(builtin_clean_usage);
+	}
+
+	if (ignored && ignored_only)
+		die("-x and -X cannot be used together");
+
+	if (disabled)
+		die("clean.requireForce set and -n or -f not given; refusing to clean");
+
+	dir.show_other_directories = 1;
+
+	if (!ignored) {
+		dir.exclude_per_dir = ".gitignore";
+		if (!access(git_path("info/exclude"), F_OK)) {
+			char *exclude_path = git_path("info/exclude");
+			add_excludes_from_file(&dir, exclude_path);
+		}
+	}
+
+	pathspec = get_pathspec(prefix, argv + i);
+	read_cache();
+	read_directory(&dir, path, base, baselen, pathspec);
+	strbuf_init(&directory, 0);
+
+	for (j = 0; j < dir.nr; ++j) {
+		struct dir_entry *ent = dir.entries[j];
+		int len, pos;
+		struct cache_entry *ce;
+		struct stat st;
+
+		/*
+		 * Remove the '/' at the end that directory
+		 * walking adds for directory entries.
+		 */
+		len = ent->len;
+		if (len && ent->name[len-1] == '/')
+			len--;
+		pos = cache_name_pos(ent->name, len);
+		if (0 <= pos)
+			continue;	/* exact match */
+		pos = -pos - 1;
+		if (pos < active_nr) {
+			ce = active_cache[pos];
+			if (ce_namelen(ce) == len &&
+			    !memcmp(ce->name, ent->name, len))
+				continue; /* Yup, this one exists unmerged */
+		}
+
+		/* remove the files */
+		if (!lstat(ent->name, &st) && (S_ISDIR(st.st_mode))) {
+			strbuf_addstr(&directory, ent->name);
+			if (show_only && remove_directories) {
+				printf("Would remove %s\n", directory.buf);
+			} else if (quiet && remove_directories) {
+				remove_dir_recursively(&directory, 0);
+			} else if (remove_directories) {
+				printf("Removing %s\n", ent->name);
+				remove_dir_recursively(&directory, 0);
+			} else if (show_only) {
+				printf("Would not remove %s\n", directory.buf);
+			} else {
+				printf("Not removing %s\n", directory.buf);
+			}
+			strbuf_reset(&directory);
+		} else {
+			if (show_only) {
+				printf("Would remove %s\n", ent->name);
+				continue;
+			} else if (!quiet) {
+				printf("Removing %s\n", ent->name);
+			}
+			unlink(ent->name);
+		}
+	}
+
+	strbuf_release(&directory);
+	return 0;
+}
diff --git a/builtin.h b/builtin.h
index 2335c01..0cbd685 100644
--- a/builtin.h
+++ b/builtin.h
@@ -24,6 +24,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 19a2172..30b7c22 100644
--- a/git.c
+++ b/git.c
@@ -298,6 +298,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 | NEED_WORK_TREE },
 		{ "commit-tree", cmd_commit_tree, RUN_SETUP },
 		{ "config", cmd_config },
 		{ "count-objects", cmd_count_objects, RUN_SETUP },
-- 
1.5.3.GIT

Re: [PATCH] Make git-clean a builtin

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1086 bytes --]

On Sun, Nov 04, 2007 at 07:02:21PM +0000, Shawn Bohrer wrote:

> +	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;
> +			dir.show_ignored =1;
> +			dir.exclude_per_dir = ".gitignore";
> +			continue;
> +		}
> +		usage(builtin_clean_usage);

  Please, parse-options.c is now in next, please use it.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Make git-clean a builtin

[Junio C Hamano]

Shawn Bohrer <shawn.bohrer@gmail.com> writes:

> 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>
> ---
> diff --git a/builtin-clean.c b/builtin-clean.c
> new file mode 100644
> index 0000000..4141eb4
> --- /dev/null
> +++ b/builtin-clean.c
> @@ -0,0 +1,157 @@
> +/*
> + * "git clean" builtin command
> + *
> + * Copyright (C) 2007 Shawn Bohrer
> + *
> + * Based on git-clean.sh by Pavel Roskin
> + */
> +
> +#include "builtin.h"
> +#include "cache.h"
> +#include "dir.h"
> +
> +static int disabled = 1;

This means we are committed to make clean.requireForce default
to true, which is fine by me.  I need to warn the users about
this early.

> +static int show_only = 0;
> +static int remove_directories = 0;
> +static int quiet = 0;
> +static int ignored = 0;
> +static int ignored_only = 0;

Please do not explicitly initialize static variables to zero.

Re: [PATCH] Make git-clean a builtin

[Carlos Rica]

2007/11/5, Junio C Hamano <gitster@pobox.com>:
> Shawn Bohrer <shawn.bohrer@gmail.com> writes:
>
> > +static int show_only = 0;
> > +static int remove_directories = 0;
> > +static int quiet = 0;
> > +static int ignored = 0;
> > +static int ignored_only = 0;
>
> Please do not explicitly initialize static variables to zero.

Is it really needed to declare those variables outside of a function
in this case? This scheme makes difficult reusing the code from other
builtins, rewriting it for libification, calling it many times, or
even understand if they were declared that way with a purpose or not.
I just don't know why they are that way in this case, is there a
reason for it?

Re: [PATCH] Make git-clean a builtin

[Junio C Hamano]

"Carlos Rica" <jasampler@gmail.com> writes:

> 2007/11/5, Junio C Hamano <gitster@pobox.com>:
>> Shawn Bohrer <shawn.bohrer@gmail.com> writes:
>>
>> > +static int show_only = 0;
>> > +static int remove_directories = 0;
>> > +static int quiet = 0;
>> > +static int ignored = 0;
>> > +static int ignored_only = 0;
>>
>> Please do not explicitly initialize static variables to zero.
>
> Is it really needed to declare those variables outside of a function
> in this case?

I do not think so --- I suspect that this is a simple cut &
paste from the standalone ls-files implementation.

Re: [PATCH] Make git-clean a builtin

[Shawn Bohrer]

On Mon, Nov 05, 2007 at 01:14:32PM -0800, Junio C Hamano wrote:
> Shawn Bohrer <shawn.bohrer@gmail.com> writes:
> [...]
> > +static int disabled = 1;
> 
> This means we are committed to make clean.requireForce default
> to true, which is fine by me.  I need to warn the users about
> this early.

Actually I don't care either way, but in my last rebase on next this
change was already made to git-clean.sh so I adjusted accordingly.

> > +static int show_only = 0;
> > +static int remove_directories = 0;
> > +static int quiet = 0;
> > +static int ignored = 0;
> > +static int ignored_only = 0;
> 
> Please do not explicitly initialize static variables to zero.

I realize that static variables will be automatically initialized to
zero so this is unnecessary, but is there some technical reason not to
initialize explicitly?  If the answer is simply a style preference that
is fine, I'm just here to learn.

Of course as already pointed out these don't actually need to be static
in the first place so I'll simply move them into cmd_clean().  This does
lead me to another question though.  Now that Dscho has converted my
patch to use parse-options, what is the best way to update my patch
while still giving credit to Dscho?

Re: [PATCH] Make git-clean a builtin

[Junio C Hamano]

Shawn Bohrer <shawn.bohrer@gmail.com> writes:

> On Mon, Nov 05, 2007 at 01:14:32PM -0800, Junio C Hamano wrote:
>> Shawn Bohrer <shawn.bohrer@gmail.com> writes:
>> [...]
>> > +static int disabled = 1;
>> 
>> This means we are committed to make clean.requireForce default
>> to true, which is fine by me.  I need to warn the users about
>> this early.
>
> Actually I don't care either way, but in my last rebase on next this
> change was already made to git-clean.sh so I adjusted accordingly.

Oh, that was not a question to you, but a note to me.

>> > +static int show_only = 0;
>> > +static int remove_directories = 0;
>> > +static int quiet = 0;
>> > +static int ignored = 0;
>> > +static int ignored_only = 0;
>> 
>> Please do not explicitly initialize static variables to zero.
>
> I realize that static variables will be automatically initialized to
> zero so this is unnecessary, but is there some technical reason not to
> initialize explicitly?  If the answer is simply a style preference that
> is fine, I'm just here to learn.

Both readability and style have to do much with this.

The style has a historical background which is a slight
technical merit.  It results in a smaller executable file, as C
compilers traditionally placed file-scope static variables that
are not explicitly initialized in the BSS section, instead of
explicitly storing N-bytes of zero as the the initial data in it
(although I do not see a reason for compilers not to do the same
for variables explicitly initialized to zero.  In fact, I think
modern gcc produces the same allocation with or without "= 0"
initialization).

> Of course as already pointed out these don't actually need to be static
> in the first place so I'll simply move them into cmd_clean().  This does
> lead me to another question though.  Now that Dscho has converted my
> patch to use parse-options, what is the best way to update my patch
> while still giving credit to Dscho?

Please send a rewritten replacement version as a single patch
that is cleanly applicable to 'next', and mention people whose
input helped you in polishing the patch in the proposed commit
log message.

[PATCH] Make git-clean a builtin

[Shawn Bohrer]

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

This also introduces a change in behavior where the -d parameter is
required to remove an entire directory of untracked files even when
the directory is passed as a path.  For example:

   git clean dir/

Now requires

   git clean -d dir/

if 'dir' only contains untracked files.  This is consistent with the
old behavior when two or more paths were specified.

Thanks to Johannes Schindelin for the conversion to using the
parse-options API.

Signed-off-by: Shawn Bohrer <shawn.bohrer@gmail.com>
---
 Makefile                                      |    3 +-
 builtin-clean.c                               |  134 +++++++++++++++++++++++++
 builtin.h                                     |    1 +
 git-clean.sh => contrib/examples/git-clean.sh |    0 
 git.c                                         |    1 +
 5 files changed, 138 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 c427fee..932ff08 100644
--- a/Makefile
+++ b/Makefile
@@ -209,7 +209,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-merge-one-file.sh git-mergetool.sh git-parse-remote.sh \
 	git-pull.sh git-rebase.sh git-rebase--interactive.sh \
 	git-repack.sh git-request-pull.sh \
@@ -325,6 +325,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..fb2feb5
--- /dev/null
+++ b/builtin-clean.c
@@ -0,0 +1,134 @@
+/*
+ * "git clean" builtin command
+ *
+ * Copyright (C) 2007 Shawn Bohrer
+ *
+ * Based on git-clean.sh by Pavel Roskin
+ */
+
+#include "builtin.h"
+#include "cache.h"
+#include "dir.h"
+#include "parse-options.h"
+
+static int force;
+
+static const char *const builtin_clean_usage[] = {
+	"git-clean [-d] [-f] [-n] [-q] [-x | -X] [--] <paths>...",
+	NULL
+};
+
+static int git_clean_config(const char *var, const char *value)
+{
+	if (!strcmp(var, "clean.requireforce")) {
+		force = !git_config_bool(var, value);
+	}
+	return 0;
+}
+
+int cmd_clean(int argc, const char **argv, const char *prefix)
+{
+	int j;
+	int show_only = 0, remove_directories = 0, quiet = 0, ignored = 0;
+	int ignored_only = 0, baselen = 0;
+	struct strbuf directory;
+	struct dir_struct dir;
+	const char *path = ".";
+	const char *base = "";
+	static const char **pathspec;
+	struct option options[] = {
+		OPT__QUIET(&quiet),
+		OPT__DRY_RUN(&show_only),
+		OPT_BOOLEAN('f', NULL, &force, "force"),
+		OPT_BOOLEAN('d', NULL, &remove_directories,
+				"remove whole directories"),
+		OPT_BOOLEAN('x', NULL, &ignored, "remove ignored files, too"),
+		OPT_BOOLEAN('X', NULL, &ignored_only,
+				"remove only ignored files"),
+		OPT_END()
+	};
+
+	git_config(git_clean_config);
+	argc = parse_options(argc, argv, options, builtin_clean_usage, 0);
+
+	memset(&dir, 0, sizeof(dir));
+	if (ignored_only) {
+		dir.show_ignored =1;
+		dir.exclude_per_dir = ".gitignore";
+	}
+
+	if (ignored && ignored_only)
+		die("-x and -X cannot be used together");
+
+	if (!show_only && !force)
+		die("clean.requireForce set and -n or -f not given; refusing to clean");
+
+	dir.show_other_directories = 1;
+
+	if (!ignored) {
+		dir.exclude_per_dir = ".gitignore";
+		if (!access(git_path("info/exclude"), F_OK)) {
+			char *exclude_path = git_path("info/exclude");
+			add_excludes_from_file(&dir, exclude_path);
+		}
+	}
+
+	pathspec = get_pathspec(prefix, argv);
+	read_cache();
+	read_directory(&dir, path, base, baselen, pathspec);
+	strbuf_init(&directory, 0);
+
+	for (j = 0; j < dir.nr; ++j) {
+		struct dir_entry *ent = dir.entries[j];
+		int len, pos;
+		struct cache_entry *ce;
+		struct stat st;
+
+		/*
+		 * Remove the '/' at the end that directory
+		 * walking adds for directory entries.
+		 */
+		len = ent->len;
+		if (len && ent->name[len-1] == '/')
+			len--;
+		pos = cache_name_pos(ent->name, len);
+		if (0 <= pos)
+			continue;	/* exact match */
+		pos = -pos - 1;
+		if (pos < active_nr) {
+			ce = active_cache[pos];
+			if (ce_namelen(ce) == len &&
+			    !memcmp(ce->name, ent->name, len))
+				continue; /* Yup, this one exists unmerged */
+		}
+
+		/* remove the files */
+		if (!lstat(ent->name, &st) && (S_ISDIR(st.st_mode))) {
+			strbuf_addstr(&directory, ent->name);
+			if (show_only && remove_directories) {
+				printf("Would remove %s\n", directory.buf);
+			} else if (quiet && remove_directories) {
+				remove_dir_recursively(&directory, 0);
+			} else if (remove_directories) {
+				printf("Removing %s\n", ent->name);
+				remove_dir_recursively(&directory, 0);
+			} else if (show_only) {
+				printf("Would not remove %s\n", directory.buf);
+			} else {
+				printf("Not removing %s\n", directory.buf);
+			}
+			strbuf_reset(&directory);
+		} else {
+			if (show_only) {
+				printf("Would remove %s\n", ent->name);
+				continue;
+			} else if (!quiet) {
+				printf("Removing %s\n", ent->name);
+			}
+			unlink(ent->name);
+		}
+	}
+
+	strbuf_release(&directory);
+	return 0;
+}
diff --git a/builtin.h b/builtin.h
index 525107f..5476a92 100644
--- a/builtin.h
+++ b/builtin.h
@@ -24,6 +24,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 204a6f7..3fa8e4d 100644
--- a/git.c
+++ b/git.c
@@ -293,6 +293,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 | NEED_WORK_TREE },
 		{ "commit-tree", cmd_commit_tree, RUN_SETUP },
 		{ "config", cmd_config },
 		{ "count-objects", cmd_count_objects, RUN_SETUP },
-- 
1.5.3.GIT

Re: [PATCH] Make git-clean a builtin

[Johannes Schindelin]

Hi,

you still have quite a number of instances where you wrap just one line 
into curly brackets:

	if (bla) {
		[just one line]
	}

Ciao,
Dscho

Re: [PATCH] Make git-clean a builtin

[Bill Lear]

On Wednesday, November 7, 2007 at 11:10:45 (+0000) Johannes Schindelin writes:
>Hi,
>
>you still have quite a number of instances where you wrap just one line 
>into curly brackets:
>
>	if (bla) {
>		[just one line]
>	}

I've always found this a thoughtful practice.  It helps ensure nobody writes:

       if (bla)
           just_one_line();
           /* perhaps a comment, other stuff ... */
           just_another_line();

which I've seen happen countless times.  It also is nice for others who
come along and extend the branch from just one line to multiple ones,
as the brackets are already in place.

Why do you find it objectionable?


Bill

Re: [PATCH] Make git-clean a builtin

[Johannes Schindelin]

Hi,

On Wed, 7 Nov 2007, Bill Lear wrote:

> On Wednesday, November 7, 2007 at 11:10:45 (+0000) Johannes Schindelin writes:
>
> > you still have quite a number of instances where you wrap just one 
> > line into curly brackets:
> >
> >	if (bla) {
> >		[just one line]
> >	}
> 
> I've always found this a thoughtful practice.  It helps ensure nobody 
> writes:
> 
>        if (bla)
>            just_one_line();
>            /* perhaps a comment, other stuff ... */
>            just_another_line();

But if there is only one line and you fail to add curly brackets when 
adding a second line, well, uhm, then I cannot help you with anything.

BTW I was talking about _one_ line, not a line and another one with a 
comment.

> It also is nice for others who come along and extend the branch from 
> just one line to multiple ones, as the brackets are already in place.

The fact is: these lines will stay single lines most likely for eternity.

> Why do you find it objectionable?

It distracts.  It's ugly.  It's unnecessary.

Ciao,
Dscho

Re: [PATCH] Make git-clean a builtin

[Matthieu Moy]

Bill Lear <rael@zopyra.com> writes:

> I've always found this a thoughtful practice.  It helps ensure nobody writes:
>
>        if (bla)
>            just_one_line();
>            /* perhaps a comment, other stuff ... */
>            just_another_line();

It also simplify patches for cases like

 	if (bla) {
 		just_one_line();
+		another_added_line();
 	}

instead of

- 	if (bla)
+ 	if (bla) {
 		just_one_line();
+		another_added_line();
+	}

But it seems people here prefer not putting the braces in this case.

-- 
Matthieu

Re: [PATCH] Make git-clean a builtin

[Jon Loeliger]

On Wed, 2007-11-07 at 07:29, Bill Lear wrote:
> On Wednesday, November 7, 2007 at 11:10:45 (+0000) Johannes Schindelin writes:
> >Hi,
> >
> >you still have quite a number of instances where you wrap just one line 
> >into curly brackets:
> >
> >	if (bla) {
> >		[just one line]
> >	}
> 
> I've always found this a thoughtful practice.  It helps ensure nobody writes:
> 
>        if (bla)
>            just_one_line();
>            /* perhaps a comment, other stuff ... */
>            just_another_line();
> 
> which I've seen happen countless times.  It also is nice for others who
> come along and extend the branch from just one line to multiple ones,
> as the brackets are already in place.
> 
> Why do you find it objectionable?

I _totally_ agree with Bill.

jdl

Re: [PATCH] Make git-clean a builtin

[Miles Bader]

Bill Lear <rael@zopyra.com> writes:
> Why do you find it objectionable?

It bloats the code, and makes it less readable.

[My conjecture is that the latter happens because braces are so visually
striking that they attract the eye; for _long_ blocks, this property of
braces helps, because it makes it easier to find them amongst the rest
of the code, but for _short_ blocks, it hurts, because it draws the eye
away from the actual code, and emphasizes structure at a time when
structure doesn't need emphasizing (because it's utterly obvious already
with such short blocks).]

-Miles

-- 
Run away!  Run away!

Re: [PATCH] Make git-clean a builtin

[Shawn Bohrer]

On Wed, Nov 07, 2007 at 11:10:45AM +0000, Johannes Schindelin wrote:
> 
> you still have quite a number of instances where you wrap just one line 
> into curly brackets:
> 
> 	if (bla) {
> 		[just one line]
> 	}

Crap.  OK I count one instance unless you count:

	if (foo) {
		one_line();
	} else if (bar) {
		one_line();
		two_lines();
	} else {
		something_else();
	}

Now I suppose I can get rid of the curly braces here as well but I
personally find that strange and ugly.  So is there an official guideline
on if else statements?

Of course I'll fix the other one I missed and send a new patch.

Re: [PATCH] Make git-clean a builtin

[Johannes Schindelin]

Hi,

On Wed, 7 Nov 2007, Shawn Bohrer wrote:

> On Wed, Nov 07, 2007 at 11:10:45AM +0000, Johannes Schindelin wrote:
> > 
> > you still have quite a number of instances where you wrap just one line 
> > into curly brackets:
> > 
> > 	if (bla) {
> > 		[just one line]
> > 	}
> 
> Crap.  OK I count one instance unless you count:
> 
> 	if (foo) {
> 		one_line();
> 	} else if (bar) {
> 		one_line();
> 		two_lines();
> 	} else {
> 		something_else();
> 	}

I do count them.  Personally, I find it highly distracting and ugly.  
Besides, we have the convention of putting the "}" not into the same line 
as "else".  (See keyword "uncuddling" in the list archives.)

While it may be true that some parts of the code follow these rules less 
strictly, it does not mean that we should introduce more of that kind.

BTW there are plenty of examples in the existing code which illustrate our 
implicit coding conventions.

> Now I suppose I can get rid of the curly braces here as well but I 
> personally find that strange and ugly.  So is there an official 
> guideline on if else statements?

Not yet ;-)  I can add it to the tentative v3 of Documentation/CodingStyle 
or CodingConventions or however the list would like to name it.

Ciao,
Dscho

Re: [PATCH] Make git-clean a builtin

[Brian Downing]

On Wed, Nov 07, 2007 at 03:04:52PM +0000, Johannes Schindelin wrote:
> I do count them.  Personally, I find it highly distracting and ugly.  
> Besides, we have the convention of putting the "}" not into the same line 
> as "else".  (See keyword "uncuddling" in the list archives.)

I was under the impression that Git followed the kernel coding standards,
which seem to want "cuddled" else statements:

136 Note that the closing brace is empty on a line of its own, _except_ in
137 the cases where it is followed by a continuation of the same statement,
138 ie a "while" in a do-statement or an "else" in an if-statement, like
139 this:
140 
141         do {
142                 body of do-loop
143         } while (condition);
144 
145 and
146 
147         if (x == y) {
148                 ..
149         } else if (x > y) {
150                 ...
151         } else {
152                 ....
153         }
154 
155 Rationale: K&R.

Searching the MARC list archives for "uncuddling" only yields the message
I am replying to.

In addition, the kernel style seems to want braces for all branches of
a conditional if any branch needs it:

163 Do not unnecessarily use braces where a single statement will do.
164 
165 if (condition)
166         action();
167 
168 This does not apply if one branch of a conditional statement is a single
169 statement. Use braces in both branches.
170 
171 if (condition) {
172         do_this();
173         do_that();
174 } else {
175         otherwise();
176 }

This makes sense (to me), as at most you're only adding one extra line
for the final closing brace, and it makes the whole conditional look more
"balanced", IMHO.

But regardless, whatever the actual style for Git should be followed.
Life's too short for arguments about coding style (even if divergence
from K&R brace style is just plain wrong.  :)

-bcd

Re: [PATCH] Make git-clean a builtin

[Junio C Hamano]

bdowning@lavos.net (Brian Downing) writes:

> This makes sense (to me), as at most you're only adding one extra line
> for the final closing brace, and it makes the whole conditional look more
> "balanced", IMHO.
>
> But regardless, whatever the actual style for Git should be followed.
> Life's too short for arguments about coding style (even if divergence
> from K&R brace style is just plain wrong.  :)

Ok.  We do not have any particularly strong technical reason to
deviate from the kernel style.  Let's follow that.

Re: [PATCH] Make git-clean a builtin

[Junio C Hamano]

Shawn Bohrer <shawn.bohrer@gmail.com> writes:

> This replaces git-clean.sh with builtin-clean.c, and moves
> git-clean.sh to the examples.
>
> This also introduces a change in behavior where the -d parameter is
> required to remove an entire directory of untracked files even when
> the directory is passed as a path.

The updated behaviour may be better, but this description at the
first read makes one wonder if it is describing a regression as
if it is a feature.

> ... For example ...
> ...
> if 'dir' only contains untracked files.  This is consistent with the
> old behavior when two or more paths were specified.

I think what you fixed are two inconsistencies in the original
implementation.  If you spelled out the existing inconsistency
and described what your implementation does differently, the
proposal would start looking like a real improvement, like this:

    1. When dir has only untracked files, these two behave differently:

        $ git clean -n dir
        $ git clean -n dir/

    the former says "Would not remove dir/", while the latter would
    say "Would remove dir/untracked" for all paths under it.

    With -d, the former would stop refusing, but the difference in
    reporting is still there.  The latter lists all paths under the
    directory.

    2. When there are more parameters, the latter behave differently:

        $ git clean -n dir/ foo

    refuses to remove dir/.  This is inconsistent.

    My reimplementation changes the behaviour by always
    requiring the -d option with or without the trailing slash.

Having said that, I do not particularly agree with the way the
new implementation resolves the existing inconsistencies.  

Wouldn't it be better to remove "dir" when the user explicitly
told you to clean "dir", with or without the trailing slash?
That's what the user asked you to do, isn't it?

Re: [PATCH] Make git-clean a builtin

[Shawn Bohrer]

On Wed, Nov 07, 2007 at 12:42:16PM -0800, Junio C Hamano wrote:
> 
> Having said that, I do not particularly agree with the way the
> new implementation resolves the existing inconsistencies.  
> 
> Wouldn't it be better to remove "dir" when the user explicitly
> told you to clean "dir", with or without the trailing slash?
> That's what the user asked you to do, isn't it?

Yes I suppose I agree.  Of course I need to spend some more time staring
at the code to figure out how to do so.  Perhaps I can figure out what
is causing the original inconsistency in git-ls-files while I'm at it.

[PATCH] Make git-clean a builtin

[Shawn Bohrer]

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

This also introduces a change in behavior when removing directories
explicitly specified as a path.  For example currently:

1. When dir has only untracked files, these two behave differently:

    $ git clean -n dir
    $ git clean -n dir/

the former says "Would not remove dir/", while the latter would say
"Would remove dir/untracked" for all paths under it, but not the
directory itself.

With -d, the former would stop refusing, however since the user
explicitly asked to remove the directory the -d is no longer required.

2. When there are more parameters:

    $ git clean -n dir foo
    $ git clean -n dir/ foo

both cases refuse to remove dir/ unless -d is specified.  Once again
since both cases requested to remove dir the -d is no longer required.

Thanks to Johannes Schindelin for the conversion to using the
parse-options API.

Signed-off-by: Shawn Bohrer <shawn.bohrer@gmail.com>
---

On Wed, Nov 07, 2007 at 11:37:50PM -0600, Shawn Bohrer wrote:
> On Wed, Nov 07, 2007 at 12:42:16PM -0800, Junio C Hamano wrote:
> >
> > Having said that, I do not particularly agree with the way the
> > new implementation resolves the existing inconsistencies.
> >
> > Wouldn't it be better to remove "dir" when the user explicitly
> > told you to clean "dir", with or without the trailing slash?
> > That's what the user asked you to do, isn't it?
>
> Yes I suppose I agree.  Of course I need to spend some more time staring
> at the code to figure out how to do so.  Perhaps I can figure out what
> is causing the original inconsistency in git-ls-files while I'm at it.

OK, I failed to fix the inconsistency between a single and multiple
paths so I'm not sure if this patch is much better.  The problem appears
to be that when dir.show_others_directories is set directories that only
contain untracked files are not recursed, which is fine with a single
path because you can adjust the path, base and baselen to
read_directory().  With multiple paths you need to find a common prefix
to all of the paths before calling read_directory() which causes the
inconsistency.

If anyone as a suggestion on how to fix this I'm all for it, but even
with the inconsistency we are no worse than we were with git-clean.sh

 Makefile                                      |    3 +-
 builtin-clean.c                               |  154 +++++++++++++++++++++++++
 builtin.h                                     |    1 +
 git-clean.sh => contrib/examples/git-clean.sh |    0 
 git.c                                         |    1 +
 5 files changed, 158 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 a2fcdb8..ef7420d 100644
--- a/Makefile
+++ b/Makefile
@@ -213,7 +213,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-merge-one-file.sh git-mergetool.sh git-parse-remote.sh \
 	git-pull.sh git-rebase.sh git-rebase--interactive.sh \
 	git-repack.sh git-request-pull.sh \
@@ -329,6 +329,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..55658e7
--- /dev/null
+++ b/builtin-clean.c
@@ -0,0 +1,154 @@
+/*
+ * "git clean" builtin command
+ *
+ * Copyright (C) 2007 Shawn Bohrer
+ *
+ * Based on git-clean.sh by Pavel Roskin
+ */
+
+#include "builtin.h"
+#include "cache.h"
+#include "dir.h"
+#include "parse-options.h"
+
+static int force;
+
+static const char *const builtin_clean_usage[] = {
+	"git-clean [-d] [-f] [-n] [-q] [-x | -X] [--] <paths>...",
+	NULL
+};
+
+static int git_clean_config(const char *var, const char *value)
+{
+	if (!strcmp(var, "clean.requireforce"))
+		force = !git_config_bool(var, value);
+	return 0;
+}
+
+int cmd_clean(int argc, const char **argv, const char *prefix)
+{
+	int j;
+	int show_only = 0, remove_directories = 0, quiet = 0, ignored = 0;
+	int ignored_only = 0, baselen = 0;
+	struct strbuf directory;
+	struct dir_struct dir;
+	const char *path, *base;
+	static const char **pathspec;
+	struct option options[] = {
+		OPT__QUIET(&quiet),
+		OPT__DRY_RUN(&show_only),
+		OPT_BOOLEAN('f', NULL, &force, "force"),
+		OPT_BOOLEAN('d', NULL, &remove_directories,
+				"remove whole directories"),
+		OPT_BOOLEAN('x', NULL, &ignored, "remove ignored files, too"),
+		OPT_BOOLEAN('X', NULL, &ignored_only,
+				"remove only ignored files"),
+		OPT_END()
+	};
+
+	git_config(git_clean_config);
+	argc = parse_options(argc, argv, options, builtin_clean_usage, 0);
+
+	memset(&dir, 0, sizeof(dir));
+	if (ignored_only) {
+		dir.show_ignored =1;
+		dir.exclude_per_dir = ".gitignore";
+	}
+
+	if (ignored && ignored_only)
+		die("-x and -X cannot be used together");
+
+	if (!show_only && !force)
+		die("clean.requireForce set and -n or -f not given; refusing to clean");
+
+	dir.show_other_directories = 1;
+
+	if (!ignored) {
+		dir.exclude_per_dir = ".gitignore";
+		if (!access(git_path("info/exclude"), F_OK)) {
+			char *exclude_path = git_path("info/exclude");
+			add_excludes_from_file(&dir, exclude_path);
+		}
+	}
+
+	pathspec = get_pathspec(prefix, argv);
+	read_cache();
+
+	/*
+	 * Calculate common prefix for the pathspec, and
+	 * use that to optimize the directory walk
+	 */
+	baselen = common_prefix(pathspec);
+	path = ".";
+	base = "";
+	if (baselen)
+		path = base = xmemdupz(*pathspec, baselen);
+	read_directory(&dir, path, base, baselen, pathspec);
+	strbuf_init(&directory, 0);
+
+	for (j = 0; j < dir.nr; ++j) {
+		struct dir_entry *ent = dir.entries[j];
+		int len, pos, specs;
+		struct cache_entry *ce;
+		struct stat st;
+		char *seen;
+
+		/*
+		 * Remove the '/' at the end that directory
+		 * walking adds for directory entries.
+		 */
+		len = ent->len;
+		if (len && ent->name[len-1] == '/')
+			len--;
+		pos = cache_name_pos(ent->name, len);
+		if (0 <= pos)
+			continue;	/* exact match */
+		pos = -pos - 1;
+		if (pos < active_nr) {
+			ce = active_cache[pos];
+			if (ce_namelen(ce) == len &&
+			    !memcmp(ce->name, ent->name, len))
+				continue; /* Yup, this one exists unmerged */
+		}
+
+		if (!lstat(ent->name, &st) && (S_ISDIR(st.st_mode))) {
+			int matched_path = 0;
+			strbuf_addstr(&directory, ent->name);
+			if (pathspec) {
+				for (specs =0; pathspec[specs]; ++specs)
+					/* nothing */;
+				seen = xcalloc(specs, 1);
+				/* Check if directory was explictly passed as
+				 * pathspec.  If so we want to remove it */
+				if (match_pathspec(pathspec, ent->name, ent->len,
+						   baselen, seen))
+					matched_path = 1;
+				free(seen);
+			}
+			if (show_only && (remove_directories || matched_path)) {
+				printf("Would remove %s\n", directory.buf);
+			} else if (quiet && (remove_directories || matched_path)) {
+				remove_dir_recursively(&directory, 0);
+			} else if (remove_directories || matched_path) {
+				printf("Removing %s\n", directory.buf);
+				remove_dir_recursively(&directory, 0);
+			} else if (show_only) {
+				printf("Would not remove %s\n", directory.buf);
+			} else {
+				printf("Not removing %s\n", directory.buf);
+			}
+			strbuf_reset(&directory);
+		} else {
+			if (show_only) {
+				printf("Would remove %s\n", ent->name);
+				continue;
+			} else if (!quiet) {
+				printf("Removing %s\n", ent->name);
+			}
+			unlink(ent->name);
+		}
+	}
+
+	strbuf_release(&directory);
+	return 0;
+}
diff --git a/builtin.h b/builtin.h
index 525107f..5476a92 100644
--- a/builtin.h
+++ b/builtin.h
@@ -24,6 +24,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 204a6f7..3fa8e4d 100644
--- a/git.c
+++ b/git.c
@@ -293,6 +293,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 | NEED_WORK_TREE },
 		{ "commit-tree", cmd_commit_tree, RUN_SETUP },
 		{ "config", cmd_config },
 		{ "count-objects", cmd_count_objects, RUN_SETUP },
-- 
1.5.3.GIT