On Thu, Nov 26, 2015 at 9:44 AM, James <rouzier@xxxxxxxxx> wrote:
> From: James Rouzier <rouzier@xxxxxxxxx>
>
> 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>]
> 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?
> 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'.
> +}
> @@ -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)?
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html