Re: [PATCH] clean: new option --exclude-from
From: Eric Sunshine <hidden>
Date: 2016-06-15 23:07:20
On Thu, Nov 26, 2015 at 9:44 AM, James [off-list ref] wrote:
quoted hunk ↗ jump to hunk
From: James Rouzier <redacted> Specify a file to read for exclude patterns. ---diff --git 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 <pattern>] [-x | -X] [--] <path>... +'git clean' [-d] [-f] [-i] [-n] [-q] [-e <pattern>] [--exclude-from <file>] [-x | -X] [--] <path>...
Common practice seems to be to include the '=' in the synopsis: [--exclude-from=<file>]
quoted hunk ↗ jump to hunk
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=<file>:: + Read exclude patterns from <file>; 1 per line.
s/;/,/ maybe?
quoted hunk ↗ jump to hunk
diff --git a/builtin/clean.c b/builtin/clean.c@@ -875,6 +875,16 @@ static void interactive_main_loop(void) +static int option_parse_exclude_from(const struct option *opt, + const char *arg, int unset)
For consistency with the other callback function in this file, you'd probably want to name this function exclude_from_cb().
+{
+ struct dir_struct *dir = opt->value;
+
+ add_excludes_from_file(dir, arg);
+
+ return 0;Style: You can drop the blank line before 'return'.
quoted hunk ↗ jump to hunk
+}@@ -898,11 +908,15 @@ 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 <pattern> to ignore rules"), PARSE_OPT_NONEG, exclude_cb }, + { OPTION_CALLBACK, 0, "exclude-from", &dir, N_("file"), + N_("exclude patterns are read from <file>"),
The existing descriptions all use imperative mood. This probably ought
to follow suit:
N_("read exclude patterns from file"),
+ 0, option_parse_exclude_from },
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));Style: Leave a blank line after the final declaration (before the memset). 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)?
quoted hunk ↗ jump to hunk
git_config(git_clean_config, NULL); if (force < 0)@@ -913,7 +927,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@@ -630,6 +630,41 @@ test_expect_success 'git clean -e' ' +test_expect_success 'git clean --exclude-from' ' + rm -fr repo && + mkdir repo && + ( + cd repo && + git init && + touch known 1 2 3 &&
Unless the timestamp is significant, we normally avoid using 'touch'
and instead just use redirection to create empty files:
>1
>2
>3
In this case, though, you're merely matching existing style in this
script, so 'touch' may be okay.
+ git add known && + echo 1 >> .git/clean-exclude && + echo 2 >> .git/clean-exclude &&
Style: no space after redirection operator.
A here-doc may be cleaner and easier to maintain:
cat >.git/clean-exclude <<-\EOF
1
2
EOF
+ 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.
+ 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.
+ ) +' + +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? 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?
test_expect_success SANITY 'git clean -d with an unreadable empty directory' '
mkdir foo &&
chmod a= foo &&
--
2.3.6