# [PATCH] Make git-clean a builtin 13 messages from 2007-11-07 to 2007-11-10. Participants: Shawn Bohrer, Johannes Schindelin, Bill Lear, Matthieu Moy, Jon Loeliger, Junio C Hamano, Brian Downing, Miles Bader. Thread: https://gitlist.dev/t/10698 ## Shawn Bohrer, 2007-11-07 05:18 Subject: [PATCH] Make git-clean a builtin Message-ID: <11944127311587-git-send-email-shawn.bohrer@gmail.com> URL: https://gitlist.dev/e/11944127311587-git-send-email-shawn.bohrer%40gmail.com ``` 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 --- 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] [--] ...", + 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 ``` ## Johannes Schindelin, 2007-11-07 11:10 Subject: Re: [PATCH] Make git-clean a builtin Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0711071110040.4362%40racer.site In-Reply-To: <11944127311587-git-send-email-shawn.bohrer@gmail.com> ``` 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 ``` ## Bill Lear, 2007-11-07 13:29 Subject: Re: [PATCH] Make git-clean a builtin Message-ID: <18225.48553.44088.269677@lisa.zopyra.com> URL: https://gitlist.dev/e/18225.48553.44088.269677%40lisa.zopyra.com In-Reply-To: ``` 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 ``` ## Johannes Schindelin, 2007-11-07 14:17 Subject: Re: [PATCH] Make git-clean a builtin Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0711071414140.4362%40racer.site In-Reply-To: <18225.48553.44088.269677@lisa.zopyra.com> ``` 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 ``` ## Matthieu Moy, 2007-11-07 14:45 Subject: Re: [PATCH] Make git-clean a builtin Message-ID: URL: https://gitlist.dev/e/vpq7ikudsi9.fsf%40bauges.imag.fr In-Reply-To: <18225.48553.44088.269677@lisa.zopyra.com> ``` Bill Lear 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 ``` ## Shawn Bohrer, 2007-11-07 14:54 Subject: Re: [PATCH] Make git-clean a builtin Message-ID: <20071107145434.GB6768@mediacenter.austin.rr.com> URL: https://gitlist.dev/e/20071107145434.GB6768%40mediacenter.austin.rr.com In-Reply-To: ``` 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. ``` ## Johannes Schindelin, 2007-11-07 15:04 Subject: Re: [PATCH] Make git-clean a builtin Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0711071501270.4362%40racer.site In-Reply-To: <20071107145434.GB6768@mediacenter.austin.rr.com> ``` 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 ``` ## Jon Loeliger, 2007-11-07 19:46 Subject: Re: [PATCH] Make git-clean a builtin Message-ID: <1194464770.14978.11.camel@ld0161-tx32> URL: https://gitlist.dev/e/1194464770.14978.11.camel%40ld0161-tx32 In-Reply-To: <18225.48553.44088.269677@lisa.zopyra.com> ``` 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 ``` ## Junio C Hamano, 2007-11-07 20:42 Subject: Re: [PATCH] Make git-clean a builtin Message-ID: <7vabppbxef.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vabppbxef.fsf%40gitster.siamese.dyndns.org In-Reply-To: <11944127311587-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. > > 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? ``` ## Brian Downing, 2007-11-07 20:51 Subject: Re: [PATCH] Make git-clean a builtin Message-ID: <20071107205101.GE6212@lavos.net> URL: https://gitlist.dev/e/20071107205101.GE6212%40lavos.net In-Reply-To: ``` 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 ``` ## Junio C Hamano, 2007-11-07 21:49 Subject: Re: [PATCH] Make git-clean a builtin Message-ID: <7v7iktafq2.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v7iktafq2.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20071107205101.GE6212@lavos.net> ``` 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. ``` ## Shawn Bohrer, 2007-11-08 05:37 Subject: Re: [PATCH] Make git-clean a builtin Message-ID: <20071108053750.GC6768@mediacenter.austin.rr.com> URL: https://gitlist.dev/e/20071108053750.GC6768%40mediacenter.austin.rr.com In-Reply-To: <7vabppbxef.fsf@gitster.siamese.dyndns.org> ``` 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. ``` ## Miles Bader, 2007-11-10 22:43 Subject: Re: [PATCH] Make git-clean a builtin Message-ID: <87ir49afhi.fsf@catnip.gol.com> URL: https://gitlist.dev/e/87ir49afhi.fsf%40catnip.gol.com In-Reply-To: <18225.48553.44088.269677@lisa.zopyra.com> ``` Bill Lear 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! ```