Re: [PATCH] Make git-clean a builtin

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Oct 8, 2007, 03:57 UTC
Message-ID: <Pine.LNX.4.64.0710080450570.4174@racer.site>
In-Reply-To: <11918014664038-git-send-email-shawn.bohrer@gmail.com>

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.

Show 6 quoted lines

> +	/* 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.

Show 9 quoted lines

> 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