Re: [PATCH] Make git-clean a builtin

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 7, 2007, 20:42 UTC
Message-ID: <7vabppbxef.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <11944127311587-git-send-email-shawn.bohrer@gmail.com>

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

Show 6 quoted lines

> 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?