patchMake git-clean a builtin

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

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

   git clean dir/

Now requires

   git clean -d dir/

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

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

Signed-off-by: Shawn Bohrer <shawn.bohrer@gmail.com>
---
 Makefile                                      |    3 +-
 builtin-clean.c                               |  134 +++++++++++++++++++++++++
 builtin.h                                     |    1 +
 git-clean.sh => contrib/examples/git-clean.sh |    0 
 git.c                                         |    1 +
 5 files changed, 138 insertions(+), 1 deletions(-)
 create mode 100644 builtin-clean.c
 rename git-clean.sh => contrib/examples/git-clean.sh (100%)

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

On Wednesday, November 7, 2007 at 11:10:45 (+0000) Johannes Schindelin 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();

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

Hi,

On Wed, 7 Nov 2007, Bill Lear wrote:

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.

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

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

Ciao, Dscho

Bill Lear <rael@zopyra.com> writes:

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.

On Wed, 2007-11-07 at 07:29, Bill Lear wrote:

I _totally_ agree with Bill.

jdl

Bill Lear <rael@zopyra.com> writes:

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

On Wed, Nov 07, 2007 at 11:10:45AM +0000, Johannes Schindelin wrote:

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.

Hi,

On Wed, 7 Nov 2007, Shawn Bohrer 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.)

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.

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

On Wed, Nov 07, 2007 at 03:04:52PM +0000, Johannes Schindelin wrote:

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

bdowning@lavos.net (Brian Downing) writes:

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

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

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.

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?

On Wed, Nov 07, 2007 at 12:42:16PM -0800, Junio C Hamano wrote:

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.