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

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

[PATCH 3/2] Use parse-options in builtin-clean

[Johannes Schindelin]

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	On Sun, 4 Nov 2007, Pierre Habouzit wrote:

	> On Sun, Nov 04, 2007 at 07:02:21PM +0000, Shawn Bohrer wrote:
	> 
	> > +	for (i = 1; i < argc; i++) {
	> > +		const char *arg = argv[i];
	> > [...]
	> 
	>   Please, parse-options.c is now in next, please use it.

	Something like this?

 builtin-clean.c |   71 ++++++++++++++++++++----------------------------------
 1 files changed, 26 insertions(+), 45 deletions(-)

diff --git a/builtin-clean.c b/builtin-clean.c
index 4141eb4..d6fc2ad 100644
--- a/builtin-clean.c
+++ b/builtin-clean.c
@@ -9,81 +9,62 @@
 #include "builtin.h"
 #include "cache.h"
 #include "dir.h"
+#include "parse-options.h"
 
-static int disabled = 1;
+static int force = 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 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")) {
-		disabled = git_config_bool(var, value);
+		force = !git_config_bool(var, value);
 	}
 	return 0;
 }
 
 int cmd_clean(int argc, const char **argv, const char *prefix)
 {
-	int i, j;
+	int j;
 	struct strbuf directory;
 	struct dir_struct dir;
 	const char *path = ".";
 	const char *base = "";
 	int baselen = 0;
 	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()
+	};
 
-	memset(&dir, 0, sizeof(dir));
 	git_config(git_clean_config);
+	argc = parse_options(argc, argv, options, builtin_clean_usage, 0);
 
-	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);
+	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 (disabled)
+	if (!show_only && !force)
 		die("clean.requireForce set and -n or -f not given; refusing to clean");
 
 	dir.show_other_directories = 1;
@@ -96,7 +77,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 		}
 	}
 
-	pathspec = get_pathspec(prefix, argv + i);
+	pathspec = get_pathspec(prefix, argv);
 	read_cache();
 	read_directory(&dir, path, base, baselen, pathspec);
 	strbuf_init(&directory, 0);
-- 
1.5.3.5.1549.g91a3

Re: [PATCH 3/2] Use parse-options in builtin-clean

[Pierre Habouzit]

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

On Sun, Nov 04, 2007 at 08:24:31PM +0000, Johannes Schindelin wrote:
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> 
> 	On Sun, 4 Nov 2007, Pierre Habouzit wrote:
> 
> 	> On Sun, Nov 04, 2007 at 07:02:21PM +0000, Shawn Bohrer wrote:
> 	> 
> 	> > +	for (i = 1; i < argc; i++) {
> 	> > +		const char *arg = argv[i];
> 	> > [...]
> 	> 
> 	>   Please, parse-options.c is now in next, please use it.
> 
> 	Something like this?

  something like this works for me :)

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

Re: [PATCH] Add more tests for git-clean

[Junio C Hamano]

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

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

This is wrong for two reasons.

 - Is "cd -" portable?

 - What happens when git-clean fails?  This test fails, and then
   it goes on to the next test without cd'ing back.

Re: [PATCH] Add more tests for git-clean

[Pierre Habouzit]

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

On Sun, Nov 04, 2007 at 11:35:42PM +0000, Junio C Hamano wrote:
> Shawn Bohrer <shawn.bohrer@gmail.com> writes:
> 
> > +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 - &&
> 
> This is wrong for two reasons.
> 
>  - Is "cd -" portable?

  this is POSIX:

8910 − When a hyphen is used as the operand, this shall be equivalent to the command:
8911   cd "$OLDPWD" && pwd
8912   which changes to the previous working directory and then writes its name.

  Meaning that cd $OLDPWD should work, and won't print $OLDPWD.

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

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

Re: [PATCH] Add more tests for git-clean

[Junio C Hamano]

Pierre Habouzit <madcoder@debian.org> writes:

> On Sun, Nov 04, 2007 at 11:35:42PM +0000, Junio C Hamano wrote:
>> Shawn Bohrer <shawn.bohrer@gmail.com> writes:
>> 
>> > +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 - &&
>> 
>> This is wrong for two reasons.
>> 
>>  - Is "cd -" portable?
>
>   this is POSIX:

That actually doesn't matter.  What the real world shells do
matters more.

In addition, "cd -" is a nice shorthand for interactive use but
it is a bad discipline to use it in a script anyway.

	...
	( cd src && git-clean ) &&
	...

would be the best way to write this.

Re: [PATCH] Add more tests for git-clean

[Johannes Schindelin]

Hi,

On Sun, 4 Nov 2007, Junio C Hamano wrote:

> Shawn Bohrer <shawn.bohrer@gmail.com> writes:
> 
> > +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 - &&
> 
> This is wrong for two reasons.
> 
>  - Is "cd -" portable?
> 
>  - What happens when git-clean fails?  This test fails, and then
>    it goes on to the next test without cd'ing back.

So it should be

	(cd src/ && git clean) &&

right?  (Note that I also removed the dash, since it will be a builtin 
after the next commit.)

Ciao,
Dscho