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

On Tue, Dec 1, 2015 at 4:36 PM, James Rouzier [off-list ref] 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]

On Sun, Nov 29, 2015 at 9:24 PM, Eric Sunshine [off-list ref]
wrote:

quoted

On Thu, Nov 26, 2015 at 9:44 AM, James [off-list ref] wrote:

quoted

From: James Rouzier <redacted>

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.

quoted

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.

quoted

quoted

+               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

quoted

quoted

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

quoted

quoted

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

quoted

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.