Re: [PATCH] clean: new option --exclude-from

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Dec 1, 2015, 22:25 UTC
Message-ID: <CAPig+cRa31uriO4qkZUydooNx0V+dNrUgFvTUxoLL9gCjq9AHQ@mail.gmail.com>
In-Reply-To: <CAGjXF72PgdjBw03ERVYxj+atvsBXK0LeJ6O3zTZgi3-kv9BWsw@mail.gmail.com>

On Tue, Dec 1, 2015 at 4:36 PM, James Rouzier <rouzier@gmail.com> wrote:

> Eric thank you for the feedback.

[re-adding git@vger.kernel.org to recipient list since this response was likely intended to be public]

Show 20 quoted lines

> On Sun, Nov 29, 2015 at 9:24 PM, Eric Sunshine <sunshine@sunshineco.com>
> wrote:
>> On Thu, Nov 26, 2015 at 9:44 AM, James <rouzier@gmail.com> wrote:
>> > From: James Rouzier <rouzier@gmail.com>
>> >
>> > Specify a file to read for exclude patterns.
>> > ---
>> > @@ -61,6 +61,9 @@ OPTIONS
>> >         $GIT_DIR/info/exclude, also consider these patterns to be in the
>> >         set of the ignore rules in effect.
>> >
>> > +--exclude-from=<file>::
>> > +       Read exclude patterns from <file>; 1 per line.
>>
>> s/;/,/ maybe?
>
> I copied this from Documentation/git-ls-files.txt to try and keep the
> documentation style consistent.
> However if it is believed to be better I will change it here and also in a
> separate patch for Documentation/git-ls-files.txt

I don't feel strongly about it. Existing precedence may be a good argument in its favor.

Show 28 quoted lines

>> Also, why move the memset() all the way up here as opposed, say, to
>> moving it just before the parse_options() invocation? Is it just to
>> make it easier for the next person who comes along wanting to
>> manipulate 'dir' early on (before git_config(), for instance)?
>
> Yes I want to make sure that the 'dir' is initialized before any usage.
>
>> > +               git clean -f --exclude-from=.git/clean-exclude &&
>> > +               test -e 1 &&
>> > +               test -e 2 &&
>> > +               ! (test -e 3) &&
>>
>> I see that you copied this from the "git clean -e" test, but it's not
>> obvious why parentheses are needed or wanted, and none of the other
>> tests use parentheses when negating the return of 'test', thus they
>> probably ought to be dropped.
>
> Ok will do
>
>> > +               test -e known
>>
>> Modern scripts would normally use test_path_is_file() and
>> test_path_is_missing() instead of 'test -e', however, you are again
>> matching existing style in this script, so 'test -e' may be
>> reasonable.
>
> Since it is the standard I could just take the time to upgrade 'test -e' in
> this test file to use newer standard.

This test script is probably relatively quiescent right now, so such cleanup may be reasonable. Since it is conceptually distinct from the purpose of the current patch, you would want to do the cleanup as a preparatory patch, thus making this a 2-patch series.

Show 30 quoted lines

>> > +test_expect_success 'git clean -e --exclude-from' '
>> > +       rm -fr repo &&
>> > +       mkdir repo &&
>> > +       (
>> > +               cd repo &&
>> > +               git init &&
>> > +               touch known 1 2 3 &&
>> > +               git add known &&
>> > +               echo 1 >> .git/clean-exclude &&
>> > +               git clean -f -e 2 --exclude-from=.git/clean-exclude &&
>> > +               test -e 1 &&
>> > +               test -e 2 &&
>> > +               ! (test -e 3) &&
>> > +               test -e known
>> > +       )
>> > +'
>>
>> Should a test be added which uses --exclude-from multiple times in the
>> same git-clean invocation?
>
> That does make sense will do.
>
>> Would it make sense add a test checking the behavior when the file
>> named by --exclude-from doesn't exist or is otherwise unusable as an
>> exclusion file?
>
> At the moment the add_excludes_from_file function will exit the program if
> there is a problem loading the exclude file.
> I could add a test for that behavior. In case in the future this behavior is
> changed.