Re: [PATCH v2 2/2] clean: new option --exclude-from
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
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
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
[PATCH v2 2/2] clean: new option --exclude-from
From: James Rouzier
Specify a file to read for exclude patterns.
---
Documentation/git-clean.txt | 5 +++-
builtin/clean.c | 15 ++--
t/t7300-clean.sh| 60 +
3 files changed, 77 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt
index 641681f..ef5dc41 100644
--- a/Documentation/git-clean.txt
+++ b/Documentation/git-clean.txt
@@ -8,7 +8,7 @@ git-clean - Remove untracked files from the working tree
SYNOPSIS
[verse]
-'git clean' [-d] [-f] [-i] [-n] [-q] [-e ] [-x | -X] [--] ...
+'git clean' [-d] [-f] [-i] [-n] [-q] [-e ] [--exclude-from=]
[-x | -X] [--] ...
DESCRIPTION
---
@@ -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=::
+ Read exclude patterns from ; 1 per line.
+
-x::
Don't use the standard ignore rules read from .gitignore (per
directory) and $GIT_DIR/info/exclude, but do still use the ignore
diff --git a/builtin/clean.c b/builtin/clean.c
index d7acb94..8e652f8 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -22,7 +22,7 @@ static struct string_list del_list = STRING_LIST_INIT_DUP;
static unsigned int colopts;
static const char *const builtin_clean_usage[] = {
- N_("git clean [-d] [-f] [-i] [-n] [-q] [-e ] [-x | -X] [--]
..."),
+ N_("git clean [-d] [-f] [-i] [-n] [-q] [-e ]
[--exclude-from=] [-x | -X] [--] ..."),
NULL
};
@@ -875,6 +875,14 @@ static void interactive_main_loop(void)
}
}
+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);
+ return 0;
+}
+
int cmd_clean(int argc, const char **argv, const char *prefix)
{
int i, res;
@@ -898,12 +906,16 @@ int cmd_clean(int argc, const char **argv, const char
*prefix)
N_("remove whole directories")),
{ OPTION_CALLBACK, 'e', "exclude", &exclude_list, N_("pattern"),
N_("add to ignore rules"), PARSE_OPT_NONEG,
exclude_cb },
+ { OPTION_CALLBACK, 0, "exclude-from", &dir, N_("file"),
+ N_("read exclude patterns from "),
+ 0, exclude_from_cb },
OPT_BOOL('x', NULL, &ignored, N_("remove ignored files, too")),
OPT_BOOL('X', NULL, &ignored_only,
N_("remove only ignored files")),
OPT_END()
};
+ memset(&dir, 0, sizeof(dir));
git_config(git_clean_config, NULL);
if (force < 0)
force = 0;
@@ -913,7 +925,6 @@ int cmd_clean(int argc, const char **argv, const char
*prefix)
argc = parse_options(argc, argv, prefix, options, builtin_clean_usage,
0);
- memset(&dir, 0, sizeof(dir));
if (ignored_only)
dir.flags |= DIR_SHOW_IGNORED;
diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index d555bb6..c6bfdda 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -628,6 +628,66 @@ test_expect_success 'git clean -e' '
test_path_is_file known
'
+test_expect_success 'git clean --exclude-from' '
+ rm -fr repo &&
+ mkdir repo &&
+ cd repo &&
+ git init &&
+ touch known 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
+ 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-th
