Re: [PATCH] Support pathspec magic :(exclude) and its short form :-
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 20, 2013, 23:48 UTC
- Message-ID
- <xmqqhab663ef.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <1384911691-11664-1-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
Show 15 quoted lines
> 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.
Show 24 quoted lines
> 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 ":(...)".
Show 19 quoted lines
> 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.
Show 19 quoted lines
> 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;
> elseLikewise. Exclusion does not participate in the early culling with the common prefix.
Show 16 quoted lines
> @@ -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 ;-)?
Show 7 quoted lines
> @@ -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 '.' ?"));;-).
Show 21 quoted lines
> +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...
Show 15 quoted lines
> + 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 */ > +}