# [PATCH] Add more tests for git-clean 15 messages from 2007-11-04 to 2007-11-06. Participants: Shawn Bohrer, Pierre Habouzit, Johannes Schindelin, Junio C Hamano, Carlos Rica. Thread: https://gitlist.dev/t/10649 ## Shawn Bohrer, 2007-11-04 19:02 Subject: [RFC] Second attempt at making git-clean a builtin Message-ID: <1194202941253-git-send-email-shawn.bohrer@gmail.com> URL: https://gitlist.dev/e/1194202941253-git-send-email-shawn.bohrer%40gmail.com ``` 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, 2007-11-04 19:02 Subject: [PATCH] Add more tests for git-clean Message-ID: <11942029442710-git-send-email-shawn.bohrer@gmail.com> URL: https://gitlist.dev/e/11942029442710-git-send-email-shawn.bohrer%40gmail.com In-Reply-To: <1194202941253-git-send-email-shawn.bohrer@gmail.com> ``` Signed-off-by: Shawn Bohrer --- 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 ``` ## Shawn Bohrer, 2007-11-04 19:02 Subject: [PATCH] Make git-clean a builtin Message-ID: <11942029474058-git-send-email-shawn.bohrer@gmail.com> URL: https://gitlist.dev/e/11942029474058-git-send-email-shawn.bohrer%40gmail.com In-Reply-To: <11942029442710-git-send-email-shawn.bohrer@gmail.com> ``` This replaces git-clean.sh with builtin-clean.c, and moves git-clean.sh to the examples. Signed-off-by: Shawn Bohrer --- 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] [--] ..."; + +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, 2007-11-04 19:41 Subject: Re: [PATCH] Make git-clean a builtin Message-ID: <20071104194129.GA4207@artemis.corp> URL: https://gitlist.dev/e/20071104194129.GA4207%40artemis.corp In-Reply-To: <11942029474058-git-send-email-shawn.bohrer@gmail.com> ``` 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 ``` ## Johannes Schindelin, 2007-11-04 20:24 Subject: [PATCH 3/2] Use parse-options in builtin-clean Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0711042023440.4362%40racer.site In-Reply-To: <20071104194129.GA4207@artemis.corp> ``` Signed-off-by: Johannes Schindelin --- 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] [--] ..."; +static const char *const builtin_clean_usage[] = { + "git-clean [-d] [-f] [-n] [-q] [-x | -X] [--] ...", + 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, 2007-11-04 21:16 Subject: Re: [PATCH 3/2] Use parse-options in builtin-clean Message-ID: <20071104211637.GB4207@artemis.corp> URL: https://gitlist.dev/e/20071104211637.GB4207%40artemis.corp In-Reply-To: ``` On Sun, Nov 04, 2007 at 08:24:31PM +0000, Johannes Schindelin wrote: > > Signed-off-by: Johannes Schindelin > --- > > 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, 2007-11-04 23:35 Subject: Re: [PATCH] Add more tests for git-clean Message-ID: <7vve8hr3ch.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vve8hr3ch.fsf%40gitster.siamese.dyndns.org In-Reply-To: <11942029442710-git-send-email-shawn.bohrer@gmail.com> ``` Shawn Bohrer 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. ``` ## Pierre Habouzit, 2007-11-04 23:46 Subject: Re: [PATCH] Add more tests for git-clean Message-ID: <20071104234617.GG4207@artemis.corp> URL: https://gitlist.dev/e/20071104234617.GG4207%40artemis.corp In-Reply-To: <7vve8hr3ch.fsf@gitster.siamese.dyndns.org> ``` On Sun, Nov 04, 2007 at 11:35:42PM +0000, Junio C Hamano wrote: > Shawn Bohrer 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 ``` ## Johannes Schindelin, 2007-11-04 23:49 Subject: Re: [PATCH] Add more tests for git-clean Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0711042348540.4362%40racer.site In-Reply-To: <7vve8hr3ch.fsf@gitster.siamese.dyndns.org> ``` Hi, On Sun, 4 Nov 2007, Junio C Hamano wrote: > Shawn Bohrer 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 ``` ## Junio C Hamano, 2007-11-05 00:17 Subject: Re: [PATCH] Add more tests for git-clean Message-ID: <7vr6j5r1ec.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vr6j5r1ec.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20071104234617.GG4207@artemis.corp> ``` Pierre Habouzit writes: > On Sun, Nov 04, 2007 at 11:35:42PM +0000, Junio C Hamano wrote: >> Shawn Bohrer 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. ``` ## Junio C Hamano, 2007-11-05 21:14 Subject: Re: [PATCH] Make git-clean a builtin Message-ID: <7vejf4pf7r.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vejf4pf7r.fsf%40gitster.siamese.dyndns.org In-Reply-To: <11942029474058-git-send-email-shawn.bohrer@gmail.com> ``` Shawn Bohrer writes: > This replaces git-clean.sh with builtin-clean.c, and moves git-clean.sh to > the examples. > > Signed-off-by: Shawn Bohrer > --- > 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. ``` ## Carlos Rica, 2007-11-05 22:10 Subject: Re: [PATCH] Make git-clean a builtin Message-ID: <1b46aba20711051410h370072e7he9cbebb54a789dac@mail.gmail.com> URL: https://gitlist.dev/e/1b46aba20711051410h370072e7he9cbebb54a789dac%40mail.gmail.com In-Reply-To: <7vejf4pf7r.fsf@gitster.siamese.dyndns.org> ``` 2007/11/5, Junio C Hamano : > Shawn Bohrer 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, 2007-11-05 23:54 Subject: Re: [PATCH] Make git-clean a builtin Message-ID: <7v4pg0meol.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v4pg0meol.fsf%40gitster.siamese.dyndns.org In-Reply-To: <1b46aba20711051410h370072e7he9cbebb54a789dac@mail.gmail.com> ``` "Carlos Rica" writes: > 2007/11/5, Junio C Hamano : >> Shawn Bohrer 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, 2007-11-06 05:05 Subject: Re: [PATCH] Make git-clean a builtin Message-ID: <20071106050512.GA6768@mediacenter.austin.rr.com> URL: https://gitlist.dev/e/20071106050512.GA6768%40mediacenter.austin.rr.com In-Reply-To: <7vejf4pf7r.fsf@gitster.siamese.dyndns.org> ``` On Mon, Nov 05, 2007 at 01:14:32PM -0800, Junio C Hamano wrote: > Shawn Bohrer 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? ``` ## Junio C Hamano, 2007-11-06 05:30 Subject: Re: [PATCH] Make git-clean a builtin Message-ID: <7vbqa8j5yr.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vbqa8j5yr.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20071106050512.GA6768@mediacenter.austin.rr.com> ``` Shawn Bohrer writes: > On Mon, Nov 05, 2007 at 01:14:32PM -0800, Junio C Hamano wrote: >> Shawn Bohrer 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. ```