threads / patch / 10649

patchAdd more tests for git-clean

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

## tl;dr

15 messages between Nov 4, 2007 and Nov 6, 2007. Diffs are folded; open one to read it.

replies: 14people: 5as markdown or json

Shawn Bohrer· Nov 4, 2007, 19:02 UTC · lore

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

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.

Shawn Bohrer· Nov 4, 2007, 19:02 UTC · re: Shawn Bohrer · lore
Signed-off-by: Shawn Bohrer <shawn.bohrer@gmail.com>
---
 t/t7300-clean.sh |  109 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 109 insertions(+), 0 deletions(-)
Show changes to t/t7300-clean.sh +109 −0
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
Shawn Bohrer· Nov 4, 2007, 19:02 UTC · re: Shawn Bohrer · lore

[PATCH] Make git-clean a builtin

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%)
Show changes to 5 files +161 −1

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

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
Pierre Habouzit· Nov 4, 2007, 19:41 UTC · re: Shawn Bohrer · lore

Re: [PATCH] Make git-clean a builtin

On Sun, Nov 04, 2007 at 07:02:21PM +0000, Shawn Bohrer wrote:
Show 37 quoted lines
> +	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
Johannes Schindelin· Nov 4, 2007, 20:24 UTC · re: Pierre Habouzit · lore

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

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(-)
Show changes to builtin-clean.c +26 −45
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
Pierre Habouzit· Nov 4, 2007, 21:16 UTC · re: Johannes Schindelin · lore

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

On Sun, Nov 04, 2007 at 08:24:31PM +0000, Johannes Schindelin wrote:
Show 15 quoted lines
> 
> 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
Junio C Hamano· Nov 5, 2007, 21:14 UTC · re: Shawn Bohrer · lore

Re: [PATCH] Make git-clean a builtin

Shawn Bohrer <shawn.bohrer@gmail.com> writes:
Show 24 quoted lines
> 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.

Show 5 quoted lines
> +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.
Carlos Rica· Nov 5, 2007, 22:10 UTC · re: Junio C Hamano · lore

Re: [PATCH] Make git-clean a builtin

2007/11/5, Junio C Hamano <gitster@pobox.com>:
Show 9 quoted lines
> 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?

Junio C Hamano· Nov 5, 2007, 23:54 UTC · re: Carlos Rica · lore

Re: [PATCH] Make git-clean a builtin

"Carlos Rica" <jasampler@gmail.com> writes:
Show 13 quoted lines
> 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.

Shawn Bohrer· Nov 6, 2007, 05:05 UTC · re: Junio C Hamano · lore

Re: [PATCH] Make git-clean a builtin

On Mon, Nov 05, 2007 at 01:14:32PM -0800, Junio C Hamano wrote:
Show 7 quoted lines
> 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.

Show 7 quoted lines
> > +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?

Junio C Hamano· Nov 6, 2007, 05:30 UTC · re: Shawn Bohrer · lore

Re: [PATCH] Make git-clean a builtin

Shawn Bohrer <shawn.bohrer@gmail.com> writes:
Show 11 quoted lines
> 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.
Show 12 quoted lines
>> > +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).

Show 5 quoted lines
> 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.

Junio C Hamano· Nov 4, 2007, 23:35 UTC · re: Shawn Bohrer · lore

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

Shawn Bohrer <shawn.bohrer@gmail.com> writes:
Show 7 quoted lines
> +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.
Pierre Habouzit· Nov 4, 2007, 23:46 UTC · re: Junio C Hamano · lore

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

On Sun, Nov 04, 2007 at 11:35:42PM +0000, Junio C Hamano wrote:
Show 13 quoted lines
> 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
Junio C Hamano· Nov 5, 2007, 00:17 UTC · re: Pierre Habouzit · lore

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

Pierre Habouzit <madcoder@debian.org> writes:
Show 16 quoted lines
> 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.
Johannes Schindelin· Nov 4, 2007, 23:49 UTC · re: Junio C Hamano · lore

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

Hi,
On Sun, 4 Nov 2007, Junio C Hamano wrote:
Show 16 quoted lines
> 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

← back to recent threads