Re: [PATCH] Add git-clean command

[Pavel Roskin <proski@gnu.org>]

On Mon, 2006-04-03 at 17:06 -0700, Junio C Hamano wrote:
> > diff --git a/git-clean.sh b/git-clean.sh
> >...
> > +for arg in "$@"; do
> 
> 	for arg
>         do
>         	...

I checked other git shell scripts and copied the "while" construct from
one of them - if it's good for other commands, it's good for git-clean.

> > +	if [ "$arg" = "-d" ]; then
> 
> 	case "$arg" in -d)...
> 
> > +excl1=
> > +excl2=
> > +if [ -z "$noexclude" ]; then
> > +	excl1="--exclude-per-directory=.gitignore"
> > +	if [ -f "$GIT_DIR/info/exclude" ]; then
> > +		excl2="--exclude-from=$GIT_DIR/info/exclude"
> > +	fi
> > +fi
> > +
> > +git-ls-files --others --directory "$excl1" "$excl2" |
> > +while read -r file; do
> > ...
> 
> The $noexclude case passes two empty strings to git-ls-files,
> which may happen to be harmless with the current implementation,
> but does not feel quite right.

Good catch.  This is needed since $GIT_DIR can contain spaces.  I
believe ${excl2:+"$excl2"} would do the trick.

> Maybe better to read ls-files -z to be really pathname safe, I
> dunno.

I think "xargs -0" has its own problems (argument length limitations),
and the other solution is to use perl.  While at that, I'd rather
rewrite the whole script in Perl, or maybe in even C.

I think this should eventually happen to all git scripts, but I have no
intention to do it right now unless you really want me to.

> > +		$echo1 "Removing $file"
> > +		[ "$cleandirhard" ] && chmod -R 700 "$file"
> 
> I am not quite sure this chmod -R is a good idea.  If we are
> trying really hard would we need to also make sure we can rmdir
> the "$file" by chmod'ing its parent directory?  But once we
> start doing that where would we stop?

OK, I was undecided on that.  I'm dropping this option.

-- 
Regards,
Pavel Roskin