Re: [PATCH] Make git-clean a builtin

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

On Sun, 7 Oct 2007, Shawn Bohrer wrote:

> +	if (ignored && ignored_only)
> +		usage(builtin_clean_usage);

Maybe a helpful message in that case, too?  (It is not apparent from the 
usage what the user has done wrong here.)

> +	if (disabled) {
> +		die("clean.requireForce set and -n or -f not given; refusing to clean");
> +	}

Please lose the curly brackets here.  They are absolutely useless, and the 
rest of the git source code avoids unnecessary curly brackets.

> +	/* Paths (argc - i) + 8 (Possible arguments)*/
> +	argv_ls_files = xmalloc((argc - i + 8) * sizeof(const char *));
> +	argv_ls_files[0] = "ls-files";
> +	argv_ls_files[1] = "--others";
> +	argv_ls_files[2] = "--directory";
> [...]

As Linus already noted, it is probably easy, and much more efficient, to 
call read_directory() here.  The best example how to use 
read_directory() is... builtin-ls-files.c.

> diff --git a/git.c b/git.c
> index d7c6bca..4e39169 100644
> --- a/git.c
> +++ b/git.c
> @@ -320,6 +320,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 },

You definitely want to have NEED_WORK_TREE here, too.

Ciao,
Dscho