Re: [PATCH v2 2/2] clean: new option --exclude-from

[Eric Sunshine]

In addition to Peff's and Junio's review comments...

On Sun, Dec 6, 2015 at 9:58 AM, James  wrote:
> From: James Rouzier 
>
> Specify a file to read for exclude patterns.

Missing Signed-off-by:.

> ---
> diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
> @@ -628,6 +628,66 @@ test_expect_success 'git clean -e' '
> +test_expect_success 'git clean --exclude-from' '
> +   rm -fr repo &&
> +   mkdir repo &&
> +   cd repo &&

See my review comments for patch 1/2 as to why you want to wrap 'cd'
and remaining statements in a subshell.

> +   git init &&
> +   touch known 1 2 3 &&

Likewise, use '>' rather than 'touch' to create empty files when the
timestamp isn't significant.

   >1 &&
   >2 &&
   >3 &&

> +   git add known &&
> +   cat >.git/clean-exclude <<-\EOF &&
> +   1
> +   2
> +   EOF
> +   git clean -f --exclude-from=.git/clean-exclude &&
> +   test_path_is_file 1 &&
> +   test_path_is_file 2 &&
> +   test_path_is_missing 3 &&
> +   test_path_is_file known
> +'
> +
> +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_path_is_file 1 &&
> +   test_path_is_file 2 &&
> +   test_path_is_missing 3 &&
> +   test_path_is_file known
> +'
> +
> +test_expect_success 'git clean --exclude-from --exclude-from' '
> +   rm -fr repo &&
> +   mkdir repo &&
> +   git init &&
> +   touch known 1 2 3 &&
> +   git add known &&
> +   cat >.git/clean-exclude1 <<-\EOF &&
> +   1
> +   EOF
> +   cat >.git/clean-exclude2 <<-\EOF &&
> +   2
> +   EOF

Creation of these single-line files probably would be more readable
using 'echo', as you do in the test just above (for
.git/clean-exclude):

echo 1 >.git/clean-exclude1 &&
echo 2 >.git/clean-exclude2 &&

> +   git clean -f --exclude-from=.git/clean-exclude1 
> --exclude-from=.git/clean-exclude2 &&
> +   test_path_is_file 1 &&
> +   test_path_is_file 2 &&
> +   test_path_is_missing 3 &&
> +   test_path_is_file known
> +'
> +
> +test_expect_success 'git clean --exclude-from=BADFILE' '
> +   rm -fr repo &&
> +   mkdir repo &&
> +   cd repo &&
> +   git init &&
> +   test_expect_code 128 git clean -f 
> --exclude-from=.git/clean-exclude-not-there
> +'
> +
>  test_expect_success SANITY 'git clean -d with an unreadable empty directory' 
> '
> mkdir foo &&
> chmod a= foo &&
> --
> 2.3.6
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] clean: new option --exclude-from

[Junio C Hamano]

James  writes:

> +static int exclude_from_cb(const struct option *opt,
> +  const char *arg, int unset)
> +{
> + struct dir_struct *dir = opt->value;
> + add_excludes_from_file(dir, arg);

I suspect this is wrong.  add_excludes_from_file() creates a
new excludes_list that is separate from the command line level and
pushes that down to the exclude stack.  You'd instead need to add
each line of the input at the same EXC_CMDL level like -e 
option does from the command line, I would think.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] clean: new option --exclude-from

[Jeff King]

On Sun, Dec 06, 2015 at 09:58:26AM -0500, James wrote:

> +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_path_is_file 1 &&
> + test_path_is_file 2 &&
> + test_path_is_missing 3 &&
> + test_path_is_file known
> +'

This checks that we exclude the union of the "-e" and "--exclude-from"
parameters. What happens if one excludes a path and the other negates
the exclusion? How is the precedence handled? Is it based on order on
the command-line, or something else?

I do not have much of a preference myself, but it probably makes sense
to document and test it.

> +test_expect_success 'git clean --exclude-from --exclude-from' '
> + rm -fr repo &&
> + mkdir repo &&
> + git init &&
> + touch known 1 2 3 &&
> + git add known &&
> + cat >.git/clean-exclude1 <<-\EOF &&
> + 1
> + EOF
> + cat >.git/clean-exclude2 <<-\EOF &&
> + 2
> + EOF
> + git clean -f --exclude-from=.git/clean-exclude1 
> --exclude-from=.git/clean-exclude2 &&
> + test_path_is_file 1 &&
> + test_path_is_file 2 &&
> + test_path_is_missing 3 &&
> + test_path_is_file known
> +'

Ditto here (as the same "type", the only precedence that would make any
sense is command-line order here).

> +test_expect_success 'git clean --exclude-from=BADFILE' '
> + rm -fr repo &&
> + mkdir repo &&
> + cd repo &&
> + git init &&
> + test_expect_code 128 git clean -f 
> --exclude-from=.git/clean-exclude-not-there
> +'

Do you actually care about the 128 here, or is just "it should exit with
failure"? If the latter, we usually use test_must_fail.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html