Skip to content

Refactor excludes library #329

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from
Closed

Refactor excludes library #329

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8cdffbd
treewide: rename 'struct exclude' to 'struct path_pattern'
derrickstolee Sep 3, 2019
908c922
treewide: rename 'struct exclude_list' to 'struct pattern_list'
derrickstolee Sep 3, 2019
7972c55
treewide: rename 'EXCL_FLAG_' to 'PATTERN_FLAG_'
derrickstolee Sep 3, 2019
53ca8ec
treewide: rename 'exclude' methods to 'pattern'
derrickstolee Sep 3, 2019
d9a62c7
unpack-trees: rename 'is_excluded_from_list()'
derrickstolee Sep 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Documentation/RelNotes/2.7.1.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ Fixes since v2.7
setting GIT_WORK_TREE environment themselves.

* The "exclude_list" structure has the usual "alloc, nr" pair of
fields to be used by ALLOC_GROW(), but clear_exclude_list() forgot
fields to be used by ALLOC_GROW(), but clear_pattern_list() forgot
to reset 'alloc' to 0 when it cleared 'nr' to discard the managed
array.

Expand Down
2 changes: 1 addition & 1 deletion Documentation/RelNotes/2.8.0.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,7 @@ notes for details).
setting GIT_WORK_TREE environment themselves.

* The "exclude_list" structure has the usual "alloc, nr" pair of
fields to be used by ALLOC_GROW(), but clear_exclude_list() forgot
fields to be used by ALLOC_GROW(), but clear_pattern_list() forgot
to reset 'alloc' to 0 when it cleared 'nr' to discard the managed
array.

Expand Down
6 changes: 3 additions & 3 deletions Documentation/technical/api-directory-listing.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,11 +111,11 @@ marked. If you to exclude files, make sure you have loaded index first.
* Prepare `struct dir_struct dir` and clear it with `memset(&dir, 0,
sizeof(dir))`.

* To add single exclude pattern, call `add_exclude_list()` and then
`add_exclude()`.
* To add single exclude pattern, call `add_pattern_list()` and then
`add_pattern()`.

* To add patterns from a file (e.g. `.git/info/exclude`), call
`add_excludes_from_file()` , and/or set `dir.exclude_per_dir`. A
`add_patterns_from_file()` , and/or set `dir.exclude_per_dir`. A
short-hand function `setup_standard_excludes()` can be used to set
up the standard set of exclude settings.

Expand Down
10 changes: 5 additions & 5 deletions attr.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ struct pattern {
const char *pattern;
int patternlen;
int nowildcardlen;
unsigned flags; /* EXC_FLAG_* */
unsigned flags; /* PATTERN_FLAG_* */
};

/*
Expand Down Expand Up @@ -400,11 +400,11 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
char *p = (char *)&(res->state[num_attr]);
memcpy(p, name, namelen);
res->u.pat.pattern = p;
parse_exclude_pattern(&res->u.pat.pattern,
parse_path_pattern(&res->u.pat.pattern,
&res->u.pat.patternlen,
&res->u.pat.flags,
&res->u.pat.nowildcardlen);
if (res->u.pat.flags & EXC_FLAG_NEGATIVE) {
if (res->u.pat.flags & PATTERN_FLAG_NEGATIVE) {
warning(_("Negative patterns are ignored in git attributes\n"
"Use '\\!' for literal leading exclamation."));
goto fail_return;
Expand Down Expand Up @@ -991,10 +991,10 @@ static int path_matches(const char *pathname, int pathlen,
int prefix = pat->nowildcardlen;
int isdir = (pathlen && pathname[pathlen - 1] == '/');

if ((pat->flags & EXC_FLAG_MUSTBEDIR) && !isdir)
if ((pat->flags & PATTERN_FLAG_MUSTBEDIR) && !isdir)
return 0;

if (pat->flags & EXC_FLAG_NODIR) {
if (pat->flags & PATTERN_FLAG_NODIR) {
return match_basename(pathname + basename_offset,
pathlen - basename_offset - isdir,
pattern, prefix,
Expand Down
34 changes: 17 additions & 17 deletions builtin/check-ignore.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,19 +32,19 @@ static const struct option check_ignore_options[] = {
OPT_END()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Jeff King wrote (reply to this):

On Tue, Sep 03, 2019 at 11:04:55AM -0700, Derrick Stolee via GitGitGadget wrote:

> From: Derrick Stolee <[email protected]>
> 
> The first consumer of pattern-matching filenames was the
> .gitignore feature. In that context, storing a list of patterns
> as a list of 'struct exclude' items makes sense. However, the
> sparse-checkout feature then adopted these structures and methods,
> but with the opposite meaning: these patterns match the files
> that should be included!
> 
> It would be clearer to rename this entire library as a "pattern
> matching" library, and the callers apply exclusion/inclusion
> logic accordingly based on their needs.
> 
> This commit renames 'struct exclude' to 'struct path_pattern'
> and renames several variable names to match. 'struct pattern'
> was already taken by attr.c, and this more completely describes
> that the patterns are specific to file paths.

I agree that the current name is overly restrictive, and this is a step
in the right direction. However, when I see path_pattern that makes me
think of our command-line pathspecs.

I wonder if there's a name that could more clearly distinguish the two.
Or if it's sufficient to just become Git jargon that "pathspec" is the
command-line one and "path_pattern" is the file-based one (we're at
least pretty consistent about the former already).

I think one could also make an argument that the name collision is a
sign that these two things should actually share both syntax and
implementation, since we're exposing too similar-but-not-quite versions
of the same idea to users. But given the compatibility issues, it's
probably not worth changing the user facing parts at this point (and I
also haven't thought too hard about it; there may be reasons why the two
_should_ differ).

-Peff

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Jeff King <[email protected]> writes:

> I wonder if there's a name that could more clearly distinguish the two.
> Or if it's sufficient to just become Git jargon that "pathspec" is the
> command-line one and "path_pattern" is the file-based one (we're at
> least pretty consistent about the former already).
>
> I think one could also make an argument that the name collision is a
> sign that these two things should actually share both syntax and
> implementation, since we're exposing too similar-but-not-quite versions
> of the same idea to users. But given the compatibility issues, it's
> probably not worth changing the user facing parts at this point (and I
> also haven't thought too hard about it; there may be reasons why the two
> _should_ differ).

Hmph.  I did not realize there are so many differences X-<.  

A pathspec is relative to $CWD, and there is a syntax, i.e.
prefixing with ":(top)", to make it relative to the root level.  An
entry in a .gitignore file will never affect paths outside the
directory the file appears in.  And there should never be such a
mechanism to allow it.

An entry without slash in .gitignore is a basename match, and there
is a syntax i.e. prefixing with "/", to anchor it to a single
directory.  A pathspec without slash also can be a basename match
(e.g. "*.c" matches "a/b.c" as well as "d.c").  A pathspec with a
slash can be made to tail-match (e.g. "**/*.c" matches "a/b.c",
"a/b/c.c", etc.) but I do not think of a way to make an entry with a
slash in a .gitignore file a tail-match the same way.  I do not think
this is intended but merely a missing feature.

So, yes, eventually we may want to make them more similar, but I
suspect that there are some things that should be in one but never
be in the other.

};

static void output_exclude(const char *path, struct exclude *exclude)
static void output_pattern(const char *path, struct path_pattern *pattern)
{
char *bang = (exclude && exclude->flags & EXC_FLAG_NEGATIVE) ? "!" : "";
char *slash = (exclude && exclude->flags & EXC_FLAG_MUSTBEDIR) ? "/" : "";
char *bang = (pattern && pattern->flags & PATTERN_FLAG_NEGATIVE) ? "!" : "";
char *slash = (pattern && pattern->flags & PATTERN_FLAG_MUSTBEDIR) ? "/" : "";
if (!nul_term_line) {
if (!verbose) {
write_name_quoted(path, stdout, '\n');
} else {
if (exclude) {
quote_c_style(exclude->el->src, NULL, stdout, 0);
if (pattern) {
quote_c_style(pattern->pl->src, NULL, stdout, 0);
printf(":%d:%s%s%s\t",
exclude->srcpos,
bang, exclude->pattern, slash);
pattern->srcpos,
bang, pattern->pattern, slash);
}
else {
printf("::\t");
Expand All @@ -56,11 +56,11 @@ static void output_exclude(const char *path, struct exclude *exclude)
if (!verbose) {
printf("%s%c", path, '\0');
} else {
if (exclude)
if (pattern)
printf("%s%c%d%c%s%s%s%c%s%c",
exclude->el->src, '\0',
exclude->srcpos, '\0',
bang, exclude->pattern, slash, '\0',
pattern->pl->src, '\0',
pattern->srcpos, '\0',
bang, pattern->pattern, slash, '\0',
path, '\0');
else
printf("%c%c%c%s%c", '\0', '\0', '\0', path, '\0');
Expand All @@ -74,7 +74,7 @@ static int check_ignore(struct dir_struct *dir,
const char *full_path;
char *seen;
int num_ignored = 0, i;
struct exclude *exclude;
struct path_pattern *pattern;
struct pathspec pathspec;

if (!argc) {
Expand Down Expand Up @@ -103,15 +103,15 @@ static int check_ignore(struct dir_struct *dir,
seen = find_pathspecs_matching_against_index(&pathspec, &the_index);
for (i = 0; i < pathspec.nr; i++) {
full_path = pathspec.items[i].match;
exclude = NULL;
pattern = NULL;
if (!seen[i]) {
int dtype = DT_UNKNOWN;
exclude = last_exclude_matching(dir, &the_index,
pattern = last_matching_pattern(dir, &the_index,
full_path, &dtype);
}
if (!quiet && (exclude || show_non_matching))
output_exclude(pathspec.items[i].original, exclude);
if (exclude)
if (!quiet && (pattern || show_non_matching))
output_pattern(pathspec.items[i].original, pattern);
if (pattern)
num_ignored++;
}
free(seen);
Expand Down
12 changes: 6 additions & 6 deletions builtin/clean.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -647,7 +647,7 @@ static int filter_by_patterns_cmd(void)
struct strbuf confirm = STRBUF_INIT;
struct strbuf **ignore_list;
struct string_list_item *item;
struct exclude_list *el;
struct pattern_list *pl;
int changed = -1, i;

for (;;) {
Expand All @@ -670,15 +670,15 @@ static int filter_by_patterns_cmd(void)
break;

memset(&dir, 0, sizeof(dir));
el = add_exclude_list(&dir, EXC_CMDL, "manual exclude");
pl = add_pattern_list(&dir, EXC_CMDL, "manual exclude");
ignore_list = strbuf_split_max(&confirm, ' ', 0);

for (i = 0; ignore_list[i]; i++) {
strbuf_trim(ignore_list[i]);
if (!ignore_list[i]->len)
continue;

add_exclude(ignore_list[i]->buf, "", 0, el, -(i+1));
add_pattern(ignore_list[i]->buf, "", 0, pl, -(i+1));
}

changed = 0;
Expand Down Expand Up @@ -900,7 +900,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
struct pathspec pathspec;
struct strbuf buf = STRBUF_INIT;
struct string_list exclude_list = STRING_LIST_INIT_NODUP;
struct exclude_list *el;
struct pattern_list *pl;
struct string_list_item *item;
const char *qname;
struct option options[] = {
Expand Down Expand Up @@ -957,9 +957,9 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
if (!ignored)
setup_standard_excludes(&dir);

el = add_exclude_list(&dir, EXC_CMDL, "--exclude option");
pl = add_pattern_list(&dir, EXC_CMDL, "--exclude option");
for (i = 0; i < exclude_list.nr; i++)
add_exclude(exclude_list.items[i].string, "", 0, el, -(i+1));
add_pattern(exclude_list.items[i].string, "", 0, pl, -(i+1));

parse_pathspec(&pathspec, 0,
PATHSPEC_PREFER_CWD,
Expand Down
8 changes: 4 additions & 4 deletions builtin/ls-files.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -492,7 +492,7 @@ static int option_parse_exclude_from(const struct option *opt,
BUG_ON_OPT_NEG(unset);

exc_given = 1;
add_excludes_from_file(dir, arg);
add_patterns_from_file(dir, arg);

return 0;
}
Expand All @@ -516,7 +516,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
int require_work_tree = 0, show_tag = 0, i;
const char *max_prefix;
struct dir_struct dir;
struct exclude_list *el;
struct pattern_list *pl;
struct string_list exclude_list = STRING_LIST_INIT_NODUP;
struct option builtin_ls_files_options[] = {
/* Think twice before adding "--nul" synonym to this */
Expand Down Expand Up @@ -594,9 +594,9 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)

argc = parse_options(argc, argv, prefix, builtin_ls_files_options,
ls_files_usage, 0);
el = add_exclude_list(&dir, EXC_CMDL, "--exclude option");
pl = add_pattern_list(&dir, EXC_CMDL, "--exclude option");
for (i = 0; i < exclude_list.nr; i++) {
add_exclude(exclude_list.items[i].string, "", 0, el, --exclude_args);
add_pattern(exclude_list.items[i].string, "", 0, pl, --exclude_args);
}
if (show_tag || show_valid_bit || show_fsmonitor_bit) {
tag_cached = "H ";
Expand Down
Loading