Re: [PATCH] Support pathspec magic :(exclude) and its short form :-

[Junio C Hamano <gitster@pobox.com>]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  This is yet another stab at the negative pathspec thing. It's not
>  ready yet (there are a few XXXs) but I could use some feedback
>  regarding the interface, or the behavior. It looks better this time
>  now that pathspec magic is supported (or maybe I'm just biased).
>
>  For :(glob) or :(icase) you're more likely to enable it for all
>  pathspec, i.e. --glob-pathspecs. But I expect :(exclude) to be typed
>  more often (it does not make sense to add --exclude-pathspecs to
>  exclude everything), which is why I add the short form for it.
>
>  We don't have many options that say "negative" in short form.
>  Either '!', '-' or '~'. '!' is already used for bash history expansion.
>  ~ looks more like $HOME expansion. Which left me '-'.

I agree with your decision to reject ~, but "!not-this-pattern" is
very much consistent with the patterns used in .gitignore (and the
"--exclude <pattern>" option), so avoiding "!" and introducing an
inconsistent "-" only to appease bash leaves somewhat a funny taste
in my mouth.

>  Documentation/glossary-content.txt |  5 ++++
>  builtin/add.c                      |  5 +++-
>  dir.c                              | 50 +++++++++++++++++++++++++++++++-----
>  pathspec.c                         |  9 ++++++-
>  pathspec.h                         |  4 ++-
>  tree-walk.c                        | 52 +++++++++++++++++++++++++++++++++++---
>  6 files changed, 112 insertions(+), 13 deletions(-)
>
> diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt
> index e470661..f7d7d8c 100644
> --- a/Documentation/glossary-content.txt
> +++ b/Documentation/glossary-content.txt
> @@ -377,6 +377,11 @@ full pathname may have special meaning:
>   - Other consecutive asterisks are considered invalid.
>  +
>  Glob magic is incompatible with literal magic.
> +
> +exclude `-`;;
> +	After a path matches any non-exclude pathspec, it will be run
> +	through all exclude pathspec. If it matches, the path is
> +	ignored.
>  --
>  +
>  Currently only the slash `/` is recognized as the "magic signature",

No longer, no?  "magic signature" is a non-alphanumeric that follows
the ':' introducer, as opposed to "magic words" that are in ":(...)".

> diff --git a/builtin/add.c b/builtin/add.c
> index 226f758..0df73ae 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -540,10 +540,13 @@ int cmd_add(int argc, const char **argv, const char *prefix)
>  			       PATHSPEC_FROMTOP |
>  			       PATHSPEC_LITERAL |
>  			       PATHSPEC_GLOB |
> -			       PATHSPEC_ICASE);
> +			       PATHSPEC_ICASE |
> +			       PATHSPEC_EXCLUDE);
>  
>  		for (i = 0; i < pathspec.nr; i++) {
>  			const char *path = pathspec.items[i].match;
> +			if (pathspec.items[i].magic & PATHSPEC_EXCLUDE)
> +				continue;
>  			if (!seen[i] &&
>  			    ((pathspec.items[i].magic &
>  			      (PATHSPEC_GLOB | PATHSPEC_ICASE)) ||

So "git add ':(exclude)junk/' '*.c'" to add all .c files except for
the ones in the 'junk/' directory may find that ':(exclude)junk/'
matched nothing (because there is no .c file in there), and that is
not an error.  It makes sense to me.

> diff --git a/dir.c b/dir.c
> index 23b6de4..e2df82f 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -126,10 +126,13 @@ static size_t common_prefix_len(const struct pathspec *pathspec)
>  		       PATHSPEC_MAXDEPTH |
>  		       PATHSPEC_LITERAL |
>  		       PATHSPEC_GLOB |
> -		       PATHSPEC_ICASE);
> +		       PATHSPEC_ICASE |
> +		       PATHSPEC_EXCLUDE);
>  
>  	for (n = 0; n < pathspec->nr; n++) {
>  		size_t i = 0, len = 0, item_len;
> +		if (pathspec->items[n].magic & PATHSPEC_EXCLUDE)
> +			continue;
>  		if (pathspec->items[n].magic & PATHSPEC_ICASE)
>  			item_len = pathspec->items[n].prefix;
>  		else

Likewise.  Exclusion does not participate in the early culling with
the common prefix.

> @@ -1375,11 +1407,17 @@ int read_directory(struct dir_struct *dir, const char *path, int len, const stru
>  			       PATHSPEC_MAXDEPTH |
>  			       PATHSPEC_LITERAL |
>  			       PATHSPEC_GLOB |
> -			       PATHSPEC_ICASE);
> +			       PATHSPEC_ICASE |
> +			       PATHSPEC_EXCLUDE);
>  
>  	if (has_symlink_leading_path(path, len))
>  		return dir->nr;
>  
> +	/*
> +	 * XXX: exclude patterns are treated like positive ones in
> +	 * create_simplify! This is not wrong, but may make path
> +	 * filtering less efficient.
> +	 */

True, but "git add ':(exclude)a/b/c' a/b" would not suffer.  And
those who do "git add ':(exclude)a/b' a/b/c" deserve it, no ;-)?

> @@ -427,6 +430,10 @@ void parse_pathspec(struct pathspec *pathspec,
>  		pathspec->magic |= item[i].magic;
>  	}
>  
> +	if (nr_exclude == n)
> +		die(_("There is nothing to exclude from by :(exclude) patterns.\n"
> +		      "Perhaps you forgot to add either ':/' or '.' ?"));

;-).

> +enum interesting tree_entry_interesting(const struct name_entry *entry,
> +					struct strbuf *base, int base_offset,
> +					const struct pathspec *ps)
> +{
> +	enum interesting positive, negative;
> +	positive = tree_entry_interesting_1(entry, base, base_offset, ps, 0);
> +
> +	/*
> +	 *   #  | positive | negative | result
> +	 * -----+----------+----------+-------
> +	 * 1..4 |   -1     |    *     |  -1
> +	 * 5..8 |    0     |    *     |   0
> +	 *   9  |    1     |   -1     |   1
> +	 *  10  |    1     |    0     |   1
> +	 *  11  |    1     |    1     |   0
> +	 *  12  |    1     |    2     |   0
> +	 *  13  |    2     |   -1     |   2
> +	 *  14  |    2     |    0     |   2
> +	 *  15  |    2     |    1     |   0
> +	 *  16  |    2     |    2     |  -1
> +	 */

Not sure what this case-table means...

> +	if (!(ps->magic & PATHSPEC_EXCLUDE) ||
> +	    positive <= entry_not_interesting) /* #1..#8 */
> +		return positive;
> +
> +	negative = tree_entry_interesting_1(entry, base, base_offset, ps, 1);
> +
> +	if (negative <= entry_not_interesting)	 /* #9, #10, #13, #14 */
> +		return positive;
> +	if ((positive == entry_interesting &&
> +	     negative >= entry_interesting) || /* #11, #12 */
> +	    (positive == all_entries_interesting &&
> +	     negative == entry_interesting)) /* #15 */
> +		return entry_not_interesting;
> +	return all_entries_not_interesting; /* #16 */
> +}