Refactor excludes library
cover Refactor excludes library
The exclude library defined in dir.h was originally written for the .gitignore feature, but has since been used for .gitattributes and sparse-checkout. In the later applications, these patterns are used for inclusion rather than exclusion, so the name is confusing. This gets particularly bad when looking at how the sparse-checkout feature uses is_excluded_from_list() to really mean "should be included in the working directory".
This series performs several renames of structs and methods to generalize the exclude library to be a "pattern matching" library. Instead of a list of excludes, we have a list of path patterns. It is up to the consumer to decide what to do with a match. The .gitignore logic will still treat the patterns as a list of exclusions, the sparse-checkout logic treats the patterns as a list of inclusions.
For this reason, some methods and structs in dir.h retain "exclude" in their name. These are limited to things consumed only by the .gitignore feature (as far as I can tell).
Most of these changes are mechanical find-and-replaces, with the exception of some variable names and the last patch.
The last patch, "unpack-trees: rename 'is_excluded_from_list()'", performs a more meaningful refactor. The method is_excluded_from_list() was only used by sparse-checkout (inside the clear_ce_flags() methods) to see if a path should be included in the working directory. The return value of "1 for excluded" was confusing. Instead, use a new enum value. This required changing several method prototypes inside unpack-trees.c.
This refactor was inspired by Elijah Newren's feedback [1] on my sparse-checkout builtin RFC. I am working on a few other orthogonal changes to make to the existing sparse-checkout behavior before I resubmit that RFC.
I had started working on v2.23.0, but found adjacent-diff conflicts with md/list-objects-filter-combo [2] and js/partial-clone-sparse-blob [3]. Those branches are independent, but the conflicts with md/list-objects-filter-combo were more severe (and that branch seems closer to merging) so this is based on md/list-object-filter-combo. Hopefully the conflicts with js/partial-clone-sparse-blob are clear enough to resolve easily.
Thanks, -Stolee
[1]
https://public-inbox.org/git/CABPp-BFMtO=7UGVZPbqh3tthSetvz5F=W3S=RsryPSuchmZeZw@mail.gmail.com/
Re: [PATCH 8/9] sparse-checkout: use hashmaps for cone patterns
[2] https://public-inbox.org/git/[email protected]/
[RFC PATCH 0/3] implement composite filters
[3] https://public-inbox.org/git/[email protected]/
[PATCH 0/2] partial-clone: fix two issues with sparse filter handling
Derrick Stolee (5):
treewide: rename 'struct exclude' to 'struct path_pattern'
treewide: rename 'struct exclude_list' to 'struct pattern_list'
treewide: rename 'EXCL_FLAG_' to 'PATTERN_FLAG_'
treewide: rename 'exclude' methods to 'pattern'
unpack-trees: rename 'is_excluded_from_list()'
Documentation/RelNotes/2.7.1.txt | 2 +- Documentation/RelNotes/2.8.0.txt | 2 +- .../technical/api-directory-listing.txt | 6 +- attr.c | 10 +- builtin/check-ignore.c | 34 +-- builtin/clean.c | 12 +- builtin/ls-files.c | 8 +- dir.c | 284 +++++++++--------- dir.h | 71 +++-- list-objects-filter.c | 37 +-- unpack-trees.c | 67 +++-- unpack-trees.h | 4 +- 12 files changed, 285 insertions(+), 252 deletions(-)
base-commit: 90d21f9ebf6906f0ebb4fb1b20ec9536072e2916
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-329%2Fderrickstolee%2Fexclude-refactor-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-329/derrickstolee/exclude-refactor-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/329
Signature · 1 line
gitgitgadget
PATCH 1/5 treewide: rename 'struct exclude' to 'struct path_pattern'
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.
--- builtin/check-ignore.c | 34 ++++++------ dir.c | 115 +++++++++++++++++++++-------------------- dir.h | 8 +-- 3 files changed, 80 insertions(+), 77 deletions(-)
builtin/check-ignore.c +17 -17
@@ -32,19 +32,19 @@ static const struct option check_ignore_options[] = { OPT_END() }; -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 &EXC_FLAG_NEGATIVE ) ?"!" :"" ; +char *slash = (pattern && pattern->flags &EXC_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->el->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" );
@@ -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->el->src,'\0' , + pattern->srcpos,'\0' , + bang, pattern->pattern, slash,'\0' , path,'\0' );else printf ("%c%c%c%s%c" ,'\0' ,'\0' ,'\0' , path,'\0' );
@@ -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) {
@@ -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_exclude_matching (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);
dir.c +59 -56
236 lines not on this page · raw message
dir.h +4 -4
@@ -16,7 +16,7 @@ struct dir_entry {#define EXC_FLAG_MUSTBEDIR 8#define EXC_FLAG_NEGATIVE 16 -struct exclude { +struct path_pattern {/* * This allows callers of last_exclude_matching() etc. * to determine the origin of the matching pattern.
@@ -54,7 +54,7 @@ struct exclude_list {/* origin of list, e.g. path to filename, or descriptive string */ const char *src; -struct exclude **excludes; +struct path_pattern **patterns; };/*
@@ -191,7 +191,7 @@ struct dir_struct {* matching exclude struct if the directory is excluded. */ struct exclude_stack *exclude_stack; -struct exclude *exclude; +struct path_pattern *pattern;struct strbuf basebuf;/* Enable untracked file cache if set */
@@ -248,7 +248,7 @@ int match_pathname(const char *, int, const char *, int, const char *, int, int, unsigned); -struct exclude *last_exclude_matching (struct dir_struct *dir, +struct path_pattern *last_exclude_matching (struct dir_struct *dir,struct index_state *istate,const char *name,int *dtype);
Signature · 1 line
gitgitgadget
Re: [PATCH 1/5] treewide: rename 'struct exclude' to 'struct path_pattern'
Quoted 9 lines · Derrick Stolee via GitGitGadget
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!
Quoted 8 lines
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 byattr.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
Re: [PATCH 1/5] treewide: rename 'struct exclude' to 'struct path_pattern'
Quoted 5 lines · Jeff King
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).
Quoted 7 lines
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.
PATCH 2/5 treewide: rename 'struct exclude_list' to 'struct pattern_list'
In reply to Derrick Stolee via GitGitGadgetFrom: 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 'struct exclude_list' 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_list' to 'struct pattern_list' and renames several variables called 'el' to 'pl'.
--- builtin/check-ignore.c | 4 +- builtin/clean.c | 12 ++--- builtin/ls-files.c | 6 +-- dir.c | 106 ++++++++++++++++++++--------------------- dir.h | 18 +++---- list-objects-filter.c | 8 ++-- unpack-trees.c | 42 ++++++++-------- unpack-trees.h | 4 +- 8 files changed, 100 insertions(+), 100 deletions(-)
builtin/check-ignore.c +2 -2
@@ -41,7 +41,7 @@ static void output_pattern(const char *path, struct path_pattern *pattern)write_name_quoted (path, stdout,'\n' ); }else {if (pattern) { -quote_c_style (pattern->el->src,NULL , stdout,0 ); +quote_c_style (pattern->pl->src,NULL , stdout,0 );printf (":%d:%s%s%s\t" , pattern->srcpos, bang, pattern->pattern, slash);
@@ -58,7 +58,7 @@ static void output_pattern(const char *path, struct path_pattern *pattern) }else {if (pattern)printf ("%s%c%d%c%s%s%s%c%s%c" , - pattern->el->src,'\0' , + pattern->pl->src,'\0' , pattern->srcpos,'\0' , bang, pattern->pattern, slash,'\0' , path,'\0' );
builtin/clean.c +6 -6
@@ -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 (;;) {
@@ -670,7 +670,7 @@ static int filter_by_patterns_cmd(void)break ;memset (&dir,0 ,sizeof (dir)); - el =add_exclude_list (&dir,EXC_CMDL ,"manual exclude" ); + pl =add_exclude_list (&dir,EXC_CMDL ,"manual exclude" ); ignore_list =strbuf_split_max (&confirm,' ' ,0 );for (i =0 ; ignore_list[i]; i++) {
@@ -678,7 +678,7 @@ static int filter_by_patterns_cmd(void)if (!ignore_list[i]->len)continue ; -add_exclude (ignore_list[i]->buf,"" ,0 , el, -(i+1 )); +add_exclude (ignore_list[i]->buf,"" ,0 , pl, -(i+1 )); } changed =0 ;
@@ -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[] = {
@@ -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_exclude_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_exclude (exclude_list.items[i].string,"" ,0 , pl, -(i+1 )); parse_pathspec(&pathspec, 0, PATHSPEC_PREFER_CWD,
builtin/ls-files.c +3 -3
@@ -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 */
@@ -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_exclude_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_exclude (exclude_list.items[i].string,"" ,0 , pl, --exclude_args); }if (show_tag || show_valid_bit || show_fsmonitor_bit) { tag_cached ="H " ;
dir.c +53 -53
274 lines not on this page · raw message
dir.h +9 -9
@@ -21,7 +21,7 @@ struct path_pattern {* This allows callers of last_exclude_matching() etc. * to determine the origin of the matching pattern. */ -struct exclude_list *el; +struct pattern_list *pl;const char *pattern;int patternlen;
@@ -44,7 +44,7 @@ struct path_pattern {* can also be used to represent the list of --exclude values passed * via CLI args. */ -struct exclude_list { +struct pattern_list {int nr;int alloc;
@@ -72,7 +72,7 @@ struct exclude_stack {struct exclude_list_group {int nr, alloc; -struct exclude_list *el; +struct pattern_list *pl; };struct oid_stat {
@@ -232,7 +232,7 @@ int read_directory(struct dir_struct *, struct index_state *istate,int is_excluded_from_list (const char *pathname,int pathlen,const char *basename,int *dtype, -struct exclude_list *el, +struct pattern_list *pl,struct index_state *istate); struct dir_entry *dir_add_ignored(struct dir_struct *dir, struct index_state *istate,
@@ -256,18 +256,18 @@ int is_excluded(struct dir_struct *dir,struct index_state *istate, const char *name, int *dtype); -struct exclude_list *add_exclude_list (struct dir_struct *dir, +struct pattern_list *add_exclude_list (struct dir_struct *dir,int group_type,const char *src);int add_excludes_from_file_to_list (const char *fname,const char *base,int baselen, -struct exclude_list *el,struct index_state *istate); +struct pattern_list *pl,struct index_state *istate);void add_excludes_from_file (struct dir_struct *,const char *fname);int add_excludes_from_blob_to_list (struct object_id *oid,const char *base,int baselen, -struct exclude_list *el); +struct pattern_list *pl);void parse_exclude_pattern (const char **string,int *patternlen,unsigned *flags,int *nowildcardlen);void add_exclude (const char *string,const char *base, -int baselen,struct exclude_list *el,int srcpos); -void clear_exclude_list (struct exclude_list *el); +int baselen,struct pattern_list *pl,int srcpos); +void clear_exclude_list (struct pattern_list *pl);void clear_directory (struct dir_struct *dir);int repo_file_exists (struct repository *repo,const char *path);
list-objects-filter.c +4 -4
@@ -347,7 +347,7 @@ struct frame { };struct filter_sparse_data { -struct exclude_list el; +struct pattern_list pl;size_t nr, alloc;struct frame *array_frame;
@@ -374,7 +374,7 @@ static enum list_objects_filter_result filter_sparse(assert (obj->type ==OBJ_TREE ); dtype =DT_DIR ; val =is_excluded_from_list (pathname,strlen (pathname), - filename, &dtype, &filter_data->el, + filename, &dtype, &filter_data->pl, r->index);if (val <0 ) val = filter_data->array_frame[filter_data->nr -1 ].defval;
@@ -436,7 +436,7 @@ static enum list_objects_filter_result filter_sparse( dtype =DT_REG ; val =is_excluded_from_list (pathname,strlen (pathname), - filename, &dtype, &filter_data->el, + filename, &dtype, &filter_data->pl, r->index);if (val <0 ) val = frame->defval;
@@ -483,7 +483,7 @@ static void filter_sparse_oid__init( {struct filter_sparse_data *d =xcalloc (1 ,sizeof (*d));if (add_excludes_from_blob_to_list (filter_options->sparse_oid_value, -NULL ,0 , &d->el) <0 ) +NULL ,0 , &d->pl) <0 )die ("could not load filter specification" );ALLOC_GROW (d->array_frame, d->nr +1 , d->alloc);
unpack-trees.c +21 -21
158 lines not on this page · raw message
unpack-trees.h +2 -2
@@ -10,7 +10,7 @@struct cache_entry ;struct unpack_trees_options ; -struct exclude_list ; +struct pattern_list ;typedef int (*merge_fn_t )(const struct cache_entry *const *src,struct unpack_trees_options *options);
@@ -83,7 +83,7 @@ struct unpack_trees_options {struct index_state *src_index;struct index_state result; -struct exclude_list *el;/* for internal use */ +struct pattern_list *pl;/* for internal use */ }; int unpack_trees(unsigned n, struct tree_desc *t,
Signature · 1 line
gitgitgadget
PATCH 3/5 treewide: rename 'EXCL_FLAG_' to 'PATTERN_FLAG_'
In reply to Derrick Stolee via GitGitGadgetFrom: 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 'struct exclude_list' 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 replaces 'EXCL_FLAG_' to 'PATTERN_FLAG_' in the names of the flags used on 'struct path_pattern'.
--- attr.c | 8 ++++---- builtin/check-ignore.c | 4 ++-- dir.c | 22 +++++++++++----------- dir.h | 10 +++++----- 4 files changed, 22 insertions(+), 22 deletions(-)
attr.c +4 -4
@@ -259,7 +259,7 @@ struct pattern {const char *pattern;int patternlen;int nowildcardlen; -unsigned flags;/* EXC_FLAG_* */ +unsigned flags;/* PATTERN_FLAG_* */ };/*
@@ -404,7 +404,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src, &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;
@@ -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,
builtin/check-ignore.c +2 -2
@@ -34,8 +34,8 @@ static const struct option check_ignore_options[] = {static void output_pattern (const char *path,struct path_pattern *pattern) { -char *bang = (pattern && pattern->flags &EXC_FLAG_NEGATIVE ) ?"!" :"" ; -char *slash = (pattern && pattern->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' );
dir.c +11 -11
@@ -571,20 +571,20 @@ void parse_exclude_pattern(const char **pattern, *flags =0 ;if (*p =='!' ) { - *flags |=EXC_FLAG_NEGATIVE ; + *flags |=PATTERN_FLAG_NEGATIVE ; p++; } len =strlen (p);if (len && p[len -1 ] =='/' ) { len--; - *flags |=EXC_FLAG_MUSTBEDIR ; + *flags |=PATTERN_FLAG_MUSTBEDIR ; }for (i =0 ; i < len; i++) {if (p[i] =='/' )break ; }if (i == len) - *flags |=EXC_FLAG_NODIR ; + *flags |=PATTERN_FLAG_NODIR ; *nowildcardlen =simple_length (p);/* * we should have excluded the trailing slash from 'p' too,
@@ -594,7 +594,7 @@ void parse_exclude_pattern(const char **pattern,if (*nowildcardlen > len) *nowildcardlen = len;if (*p =='*' &&no_wildcard (p +1 )) - *flags |=EXC_FLAG_ENDSWITH ; + *flags |=PATTERN_FLAG_ENDSWITH ; *pattern = p; *patternlen = len; }
@@ -608,7 +608,7 @@ void add_exclude(const char *string, const char *base,int nowildcardlen;parse_exclude_pattern (&string, &patternlen, &flags, &nowildcardlen); -if (flags &EXC_FLAG_MUSTBEDIR ) { +if (flags &PATTERN_FLAG_MUSTBEDIR ) {FLEXPTR_ALLOC_MEM (pattern, pattern, string, patternlen); }else { pattern =xmalloc (sizeof (*pattern));
@@ -940,7 +940,7 @@ int match_basename(const char *basename, int basenamelen,if (patternlen == basenamelen && !fspathncmp (pattern, basename, basenamelen))return 1 ; - } else if (flags & EXC_FLAG_ENDSWITH) { + } else if (flags & PATTERN_FLAG_ENDSWITH) { /* "*literal" matching against "fooliteral" */ if (patternlen - 1 <= basenamelen && !fspathncmp(pattern + 1,
@@ -1039,14 +1039,14 @@ static struct path_pattern *last_exclude_matching_from_list(const char *pathnameconst char *exclude = pattern->pattern;int prefix = pattern->nowildcardlen; -if (pattern->flags &EXC_FLAG_MUSTBEDIR ) { +if (pattern->flags &PATTERN_FLAG_MUSTBEDIR ) {if (*dtype ==DT_UNKNOWN ) *dtype =get_dtype (NULL , istate, pathname, pathlen);if (*dtype !=DT_DIR )continue ; } -if (pattern->flags &EXC_FLAG_NODIR ) { +if (pattern->flags &PATTERN_FLAG_NODIR ) { if (match_basename(basename, pathlen - (basename - pathname), exclude, prefix, pattern->patternlen,
@@ -1083,7 +1083,7 @@ int is_excluded_from_list(const char *pathname, pattern =last_exclude_matching_from_list (pathname, pathlen, basename, dtype, pl, istate);if (pattern) -return pattern->flags &EXC_FLAG_NEGATIVE ?0 :1 ; +return pattern->flags &PATTERN_FLAG_NEGATIVE ?0 :1 ;return -1 ;/* undecided */ }
@@ -1198,7 +1198,7 @@ static void prep_exclude(struct dir_struct *dir, dir->basebuf.buf + current, &dt); dir->basebuf.buf[stk->baselen -1 ] ='/' ;if (dir->pattern && - dir->pattern->flags &EXC_FLAG_NEGATIVE ) + dir->pattern->flags &PATTERN_FLAG_NEGATIVE ) dir->pattern =NULL ;if (dir->pattern) { dir->exclude_stack = stk;
@@ -1298,7 +1298,7 @@ int is_excluded(struct dir_struct *dir, struct index_state *istate,struct path_pattern *pattern =last_exclude_matching (dir, istate, pathname, dtype_p);if (pattern) -return pattern->flags &EXC_FLAG_NEGATIVE ?0 :1 ; +return pattern->flags &PATTERN_FLAG_NEGATIVE ?0 :1 ;return 0 ; }
dir.h +5 -5
@@ -11,10 +11,10 @@ struct dir_entry {char name[FLEX_ARRAY ];/* more */ }; -#define EXC_FLAG_NODIR 1 -#define EXC_FLAG_ENDSWITH 4 -#define EXC_FLAG_MUSTBEDIR 8 -#define EXC_FLAG_NEGATIVE 16 +#define PATTERN_FLAG_NODIR 1 +#define PATTERN_FLAG_ENDSWITH 4 +#define PATTERN_FLAG_MUSTBEDIR 8 +#define PATTERN_FLAG_NEGATIVE 16struct path_pattern {/*
@@ -28,7 +28,7 @@ struct path_pattern {int nowildcardlen;const char *base;int baselen; -unsigned flags;/* EXC_FLAG_* */ +unsigned flags;/* PATTERN_FLAG_* */ /* * Counting starts from 1 for line numbers in ignore files,
Signature · 1 line
gitgitgadget
PATCH 4/5 treewide: rename 'exclude' methods to 'pattern'
In reply to Derrick Stolee via GitGitGadgetFrom: 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 'struct exclude_list' 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 several methods defined in dir.h to make more sense with the renamed 'struct exclude_list' to 'struct pattern_list' and 'struct exclude' to 'struct path_pattern':
* last_exclude_matching() -> last_matching_pattern()
* parse_exclude() -> parse_path_pattern()
In addition, the word 'exclude' was replaced with 'pattern' in the methods below:
* add_exclude_list()
* add_excludes_from_file_to_list()
* add_excludes_from_file()
* add_excludes_from_blob_to_list()
* add_exclude()
* clear_exclude_list()
A few methods with the word "exclude" remain. These will be handled seperately. In particular, the method "is_excluded()" is concretely about the .gitignore file relative to a specific directory. This is the important boundary between library and consumer: is_excluded() cares about .gitignore, but is_excluded() calls last_matching_pattern() to make that decision.
--- Documentation/RelNotes/2.7.1.txt | 2 +- Documentation/RelNotes/2.8.0.txt | 2 +- .../technical/api-directory-listing.txt | 6 +- attr.c | 2 +- builtin/check-ignore.c | 2 +- builtin/clean.c | 8 +- builtin/ls-files.c | 6 +- dir.c | 78 +++++++++---------- dir.h | 22 +++--- list-objects-filter.c | 2 +- unpack-trees.c | 4 +- 11 files changed, 67 insertions(+), 67 deletions(-)
Documentation/RelNotes/2.7.1.txt +1 -1
@@ -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.
Documentation/RelNotes/2.8.0.txt +1 -1
@@ -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.
Documentation/technical/api-directory-listing.txt +3 -3
@@ -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.
attr.c +1 -1
@@ -400,7 +400,7 @@ 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);
builtin/check-ignore.c +1 -1
@@ -106,7 +106,7 @@ static int check_ignore(struct dir_struct *dir, pattern =NULL ;if (!seen[i]) {int dtype =DT_UNKNOWN ; - pattern =last_exclude_matching (dir, &the_index, + pattern =last_matching_pattern (dir, &the_index, full_path, &dtype); }if (!quiet && (pattern || show_non_matching))
builtin/clean.c +4 -4
@@ -670,7 +670,7 @@ static int filter_by_patterns_cmd(void)break ;memset (&dir,0 ,sizeof (dir)); - pl =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++) {
@@ -678,7 +678,7 @@ static int filter_by_patterns_cmd(void)if (!ignore_list[i]->len)continue ; -add_exclude (ignore_list[i]->buf,"" ,0 , pl, -(i+1 )); +add_pattern (ignore_list[i]->buf,"" ,0 , pl, -(i+1 )); } changed =0 ;
@@ -957,9 +957,9 @@ int cmd_clean(int argc, const char **argv, const char *prefix)if (!ignored)setup_standard_excludes (&dir); - pl =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 , pl, -(i+1 )); +add_pattern (exclude_list.items[i].string,"" ,0 , pl, -(i+1 )); parse_pathspec(&pathspec, 0, PATHSPEC_PREFER_CWD,
builtin/ls-files.c +3 -3
@@ -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 ; }
@@ -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 ); - pl =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 , pl, --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 " ;
dir.c +39 -39
275 lines not on this page · raw message
dir.h +11 -11
@@ -18,7 +18,7 @@ struct dir_entry {struct path_pattern {/* -* This allows callers of last_exclude_matching() etc. +* This allows callers of last_matching_pattern() etc. * to determine the origin of the matching pattern. */ struct pattern_list *pl;
@@ -248,26 +248,26 @@ int match_pathname(const char *, int, const char *, int, const char *, int, int, unsigned); -struct path_pattern *last_exclude_matching (struct dir_struct *dir, -struct index_state *istate, -const char *name,int *dtype); +struct path_pattern *last_matching_pattern (struct dir_struct *dir, +struct index_state *istate, +const char *name,int *dtype);int is_excluded (struct dir_struct *dir,struct index_state *istate,const char *name,int *dtype); -struct pattern_list *add_exclude_list (struct dir_struct *dir, +struct pattern_list *add_pattern_list (struct dir_struct *dir,int group_type,const char *src); -int add_excludes_from_file_to_list (const char *fname,const char *base,int baselen, +int add_patterns_from_file_to_list (const char *fname,const char *base,int baselen,struct pattern_list *pl,struct index_state *istate); -void add_excludes_from_file (struct dir_struct *,const char *fname); -int add_excludes_from_blob_to_list (struct object_id *oid, +void add_patterns_from_file (struct dir_struct *,const char *fname); +int add_patterns_from_blob_to_list (struct object_id *oid,const char *base,int baselen,struct pattern_list *pl); -void parse_exclude_pattern (const char **string,int *patternlen,unsigned *flags,int *nowildcardlen); -void add_exclude (const char *string,const char *base, +void parse_path_pattern (const char **string,int *patternlen,unsigned *flags,int *nowildcardlen); +void add_pattern (const char *string,const char *base,int baselen,struct pattern_list *pl,int srcpos); -void clear_exclude_list (struct pattern_list *pl); +void clear_pattern_list (struct pattern_list *pl);void clear_directory (struct dir_struct *dir);int repo_file_exists (struct repository *repo,const char *path);
list-objects-filter.c +1 -1
@@ -482,7 +482,7 @@ static void filter_sparse_oid__init( struct filter *filter) {struct filter_sparse_data *d =xcalloc (1 ,sizeof (*d)); -if (add_excludes_from_blob_to_list (filter_options->sparse_oid_value, +if (add_patterns_from_blob_to_list (filter_options->sparse_oid_value,NULL ,0 , &d->pl) <0 )die ("could not load filter specification" );
unpack-trees.c +2 -2
@@ -1464,7 +1464,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options o->skip_sparse_checkout =1 ;if (!o->skip_sparse_checkout) {char *sparse =git_pathdup ("info/sparse-checkout" ); -if (add_excludes_from_file_to_list (sparse,"" ,0 , &pl,NULL ) <0 ) +if (add_patterns_from_file_to_list (sparse,"" ,0 , &pl,NULL ) <0 ) o->skip_sparse_checkout =1 ;else o->pl = &pl;
@@ -1631,7 +1631,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options done:trace_performance_leave ("unpack_trees" ); -clear_exclude_list (&pl); +clear_pattern_list (&pl);return ret; return_failed:
Signature · 1 line
gitgitgadget
PATCH 5/5 unpack-trees: rename 'is_excluded_from_list()'
In reply to Derrick Stolee via GitGitGadgetFrom: 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 'struct exclude_list' 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!
Now that this library is renamed to use 'struct pattern_list' and 'struct pattern', we can now rename the method used by the sparse-checkout feature to determine which paths should appear in the working directory.
The method is_excluded_from_list() is only used by the sparse-checkout logic in unpack-trees and list-objects-filter. The confusing part is that it returned 1 for "excluded" (i.e. it matches the list of exclusions) but that really manes that the path matched the list of patterns for _inclusion_ in the working directory.
Rename the method to be path_matches_pattern_list() and have it return an explicit 'enum pattern_match_result'. Here, the values MATCHED = 1, UNMATCHED = 0, and UNDECIDED = -1 agree with the previous integer values. This shift allows future consumers to better understand what the retur values mean, and provides more type checking for handling those values.
--- dir.c | 25 +++++++++++++++++-------- dir.h | 21 +++++++++++++++++---- list-objects-filter.c | 29 +++++++++++++++-------------- unpack-trees.c | 39 +++++++++++++++++++++++---------------- 4 files changed, 72 insertions(+), 42 deletions(-)
dir.c +17 -8
@@ -1072,19 +1072,28 @@ static struct path_pattern *last_matching_pattern_from_list(const char *pathname }/* -* Scan the list and let the last match determine the fate. -* Return 1 for exclude, 0 for include and -1 for undecided. +* Scan the list of patterns to determine if the ordered list +* of patterns matches on 'pathname'. +* +* Return 1 for a match, 0 for not matched and -1 for undecided. */ -int is_excluded_from_list (const char *pathname, -int pathlen,const char *basename,int *dtype, -struct pattern_list *pl,struct index_state *istate) +enum pattern_match_result path_matches_pattern_list ( +const char *pathname,int pathlen, +const char *basename,int *dtype, +struct pattern_list *pl, +struct index_state *istate) {struct path_pattern *pattern; pattern =last_matching_pattern_from_list (pathname, pathlen, basename, dtype, pl, istate); -if (pattern) -return pattern->flags &PATTERN_FLAG_NEGATIVE ?0 :1 ; -return -1 ;/* undecided */ +if (pattern) { +if (pattern->flags &PATTERN_FLAG_NEGATIVE ) +return NOT_MATCHED ; +else +return MATCHED ; + } + +return UNDECIDED ; } static struct path_pattern *last_matching_pattern_from_lists(
dir.h +17 -4
@@ -230,10 +230,23 @@ int read_directory(struct dir_struct *, struct index_state *istate, const char *path, int len, const struct pathspec *pathspec); -int is_excluded_from_list (const char *pathname,int pathlen, -const char *basename,int *dtype, -struct pattern_list *pl, -struct index_state *istate); +enum pattern_match_result { +UNDECIDED =-1 , +NOT_MATCHED =0 , +MATCHED =1 , +}; + +/* +* Scan the list of patterns to determine if the ordered list +* of patterns matches on 'pathname'. +* +* Return 1 for a match, 0 for not matched and -1 for undecided. +*/ +enum pattern_match_result path_matches_pattern_list (const char *pathname, +int pathlen, +const char *basename,int *dtype, +struct pattern_list *pl, +struct index_state *istate);struct dir_entry *dir_add_ignored (struct dir_struct *dir,struct index_state *istate,const char *pathname,int len);
list-objects-filter.c +15 -14
@@ -328,12 +328,12 @@ static void filter_blobs_limit__init(*/ struct frame {/* -* defval is the usual default include/exclude value that +* default_match is the usual default include/exclude value that * should be inherited as we recurse into directories based * upon pattern matching of the directory itself or of a * containing directory. */ -int defval; +enum pattern_match_result default_match;/* * 1 if the directory (recursively) contains any provisionally
@@ -363,8 +363,9 @@ static enum list_objects_filter_result filter_sparse( void *filter_data_) {struct filter_sparse_data *filter_data = filter_data_; -int val, dtype; +int dtype;struct frame *frame; +enum pattern_match_result match;switch (filter_situation) {default :
@@ -373,15 +374,15 @@ static enum list_objects_filter_result filter_sparse(case LOFS_BEGIN_TREE :assert (obj->type ==OBJ_TREE ); dtype =DT_DIR ; - val =is_excluded_from_list (pathname,strlen (pathname), - filename, &dtype, &filter_data->pl, - r->index); -if (val <0 ) - val = filter_data->array_frame[filter_data->nr -1 ].defval; + match =path_matches_pattern_list (pathname,strlen (pathname), + filename, &dtype, &filter_data->pl, + r->index); +if (match ==UNDECIDED ) + match = filter_data->array_frame[filter_data->nr -1 ].default_match;ALLOC_GROW (filter_data->array_frame, filter_data->nr +1 , filter_data->alloc); - filter_data->array_frame[filter_data->nr].defval = val; + filter_data->array_frame[filter_data->nr].default_match = match; filter_data->array_frame[filter_data->nr].child_prov_omit =0 ; filter_data->nr++;
@@ -435,12 +436,12 @@ static enum list_objects_filter_result filter_sparse( frame = &filter_data->array_frame[filter_data->nr -1 ]; dtype =DT_REG ; - val =is_excluded_from_list (pathname,strlen (pathname), + match =path_matches_pattern_list (pathname,strlen (pathname), filename, &dtype, &filter_data->pl, r->index); -if (val <0 ) - val = frame->defval; -if (val >0 ) { +if (match ==UNDECIDED ) + match = frame->default_match; +if (match ==MATCHED ) {if (omits)oidset_remove (omits, &obj->oid);return LOFR_MARK_SEEN |LOFR_DO_SHOW ;
@@ -487,7 +488,7 @@ static void filter_sparse_oid__init(die ("could not load filter specification" );ALLOC_GROW (d->array_frame, d->nr +1 , d->alloc); - d->array_frame[d->nr].defval =0 ;/* default to include */ + d->array_frame[d->nr].default_match =0 ;/* default to include */ d->array_frame[d->nr].child_prov_omit =0 ; d->nr++;
unpack-trees.c +23 -16
@@ -1265,7 +1265,8 @@ static int clear_ce_flags_1(struct index_state *istate, struct cache_entry **cache, int nr, struct strbuf *prefix, int select_mask, int clear_mask, - struct pattern_list *pl, int defval); + struct pattern_list *pl, + enum pattern_match_result default_match);/* Whole directory matching */ static int clear_ce_flags_dir(struct index_state *istate,
@@ -1273,19 +1274,21 @@ static int clear_ce_flags_dir(struct index_state *istate,struct strbuf *prefix, char *basename, int select_mask, int clear_mask, - struct pattern_list *pl, int defval) + struct pattern_list *pl, + enum pattern_match_result default_match) { struct cache_entry **cache_end;int dtype =DT_DIR ; -int ret =is_excluded_from_list (prefix->buf, prefix->len, - basename, &dtype, pl, istate);int rc; +enum pattern_match_result ret; + ret =path_matches_pattern_list (prefix->buf, prefix->len, + basename, &dtype, pl, istate);strbuf_addch (prefix,'/' );/* If undecided, use matching result of parent dir in defval */ -if (ret <0 ) - ret = defval; +if (ret ==UNDECIDED ) + ret = default_match;for (cache_end = cache; cache_end != cache + nr; cache_end++) {struct cache_entry *ce = *cache_end;
@@ -1298,7 +1301,7 @@ static int clear_ce_flags_dir(struct index_state *istate,* with ret (iow, we know in advance the incl/excl * decision for the entire directory), clear flag here without * calling clear_ce_flags_1(). That function will call -* the expensive is_excluded_from_list() on every entry. +* the expensive path_matches_pattern_list() on every entry. */ rc = clear_ce_flags_1(istate, cache, cache_end - cache, prefix,
@@ -1327,7 +1330,8 @@ static int clear_ce_flags_1(struct index_state *istate, struct cache_entry **cache, int nr, struct strbuf *prefix, int select_mask, int clear_mask, - struct pattern_list *pl, int defval) + struct pattern_list *pl, + enum pattern_match_result default_match) { struct cache_entry **cache_end = cache + nr;
@@ -1338,7 +1342,8 @@ static int clear_ce_flags_1(struct index_state *istate,while (cache != cache_end) {struct cache_entry *ce = *cache;const char *name, *slash; -int len, dtype, ret; +int len, dtype; +enum pattern_match_result ret;if (select_mask && !(ce->ce_flags & select_mask)) { cache++;
@@ -1362,7 +1367,7 @@ static int clear_ce_flags_1(struct index_state *istate, prefix, prefix->buf + prefix->len - len, select_mask, clear_mask, - pl, defval); + pl, default_match);/* clear_c_f_dir eats a whole dir already? */ if (processed) {
@@ -1374,18 +1379,20 @@ static int clear_ce_flags_1(struct index_state *istate,strbuf_addch (prefix,'/' ); cache +=clear_ce_flags_1 (istate, cache, cache_end - cache, prefix, - select_mask, clear_mask, pl, defval); + select_mask, clear_mask, pl, + default_match);strbuf_setlen (prefix, prefix->len - len -1 );continue ; }/* Non-directory */ dtype =ce_to_dtype (ce); - ret =is_excluded_from_list (ce->name,ce_namelen (ce), - name, &dtype, pl, istate); -if (ret <0 ) - ret = defval; -if (ret >0 ) + ret =path_matches_pattern_list (ce->name, +ce_namelen (ce), + name, &dtype, pl, istate); +if (ret ==UNDECIDED ) + ret = default_match; +if (ret ==MATCHED ) ce->ce_flags &= ~clear_mask; cache++; }
Signature · 1 line
gitgitgadget
Re: [PATCH 5/5] unpack-trees: rename 'is_excluded_from_list()'
Quoted 15 lines · Derrick Stolee via GitGitGadget
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 'struct
exclude_list' 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!Now that this library is renamed to use 'struct
pattern_list' and 'struct pattern', we can now rename the method used by the sparse-checkout feature to determine which paths should appear in the working directory.
Quoted 4 lines
The method
is_excluded_from_list()is only used by the sparse-checkout logic in unpack-trees and list-objects-filter. The confusing part is that it returned 1 for "excluded" (i.e. it matches the list of exclusions) but that really manes that
s/manes/means/
Quoted 8 lines
the path matched the list of patterns for _inclusion_ in the working directory.
Rename the method to be
path_matches_pattern_list()and have it return an explicit 'enumpattern_match_result'. Here, the values MATCHED = 1, UNMATCHED = 0, and UNDECIDED = -1 agree with the previous integer values. This shift allows future consumers to better understand what the retur values mean,
s/retur/return/
Quoted 15 lines
and provides more type checking for handling those values.
Signed-off-by Derrick Stolee <[email protected]>--- dir.c | 25 +++++++++++++++++-------- dir.h | 21 +++++++++++++++++---- list-objects-filter.c | 29 +++++++++++++++-------------- unpack-trees.c | 39 +++++++++++++++++++++++---------------- 4 files changed, 72 insertions(+), 42 deletions(-)dir.c +0
-0
dir.c static struct path_pattern *last_matching_pattern_from_list(const char *pathname · line 1078
Quoted 9 lines
dir.c +4
-2@@ -1072,19 +1072,28 @@ static struct path_pattern *last_matching_pattern_from_list(const char *pathname }/* -* Scan the list and let the last match determine the fate. -* Return 1 for exclude, 0 for include and -1 for undecided. +* Scan the list of patterns to determine if the ordered list +* of patterns matches on 'pathname'. +* +* Return 1 for a match, 0 for not matched and -1 for undecided.
Maybe drop this last sentence since it's misleading, and since the function signature already specifies that it returns an enum and the enums have obvious meanings already?
Quoted 33 lines
dir.c +13
-6*/ -int is_excluded_from_list (const char *pathname, -int pathlen,const char *basename,int *dtype, -struct pattern_list *pl,struct index_state *istate) +enum pattern_match_result path_matches_pattern_list ( +const char *pathname,int pathlen, +const char *basename,int *dtype, +struct pattern_list *pl, +struct index_state *istate) {struct path_pattern *pattern; pattern =last_matching_pattern_from_list (pathname, pathlen, basename, dtype, pl, istate); -if (pattern) -return pattern->flags &PATTERN_FLAG_NEGATIVE ?0 :1 ; -return -1 ;/* undecided */ +if (pattern) { +if (pattern->flags &PATTERN_FLAG_NEGATIVE ) +return NOT_MATCHED ; +else +return MATCHED ; + } + +return UNDECIDED ; } static struct path_pattern *last_matching_pattern_from_lists(dir.h +0
-0@@ -230,10 +230,23 @@ int read_directory(struct dir_struct *, struct index_state *istate, const char *path, int len,
dir.h int read_directory(struct dir_struct *, struct index_state *istate, · line 236
Quoted 10 lines
dir.h +4
-4const struct pathspec *pathspec); -int is_excluded_from_list (const char *pathname,int pathlen, -const char *basename,int *dtype, -struct pattern_list *pl, -struct index_state *istate); +enum pattern_match_result { +UNDECIDED =-1 , +NOT_MATCHED =0 , +MATCHED =1 ,
Are we worried about preventing e.g. grep from using these names or clashing with another lib (block-sha1?) that might choose to define these?
dir.h int read_directory(struct dir_struct *, struct index_state *istate, · line 243
Quoted 7 lines
dir.h +7
-0+}; + +/* +* Scan the list of patterns to determine if the ordered list +* of patterns matches on 'pathname'. +* +* Return 1 for a match, 0 for not matched and -1 for undecided.
Again, I'd drop the last sentence.
Quoted 79 lines
dir.h +6
-0+*/ +enum pattern_match_result path_matches_pattern_list (const char *pathname, +int pathlen, +const char *basename,int *dtype, +struct pattern_list *pl, +struct index_state *istate);struct dir_entry *dir_add_ignored (struct dir_struct *dir,struct index_state *istate,const char *pathname,int len);list-objects-filter.c +14
-13@@ -328,12 +328,12 @@ static void filter_blobs_limit__init(*/ struct frame {/* -* defval is the usual default include/exclude value that +* default_match is the usual default include/exclude value that * should be inherited as we recurse into directories based * upon pattern matching of the directory itself or of a * containing directory. */ -int defval; +enum pattern_match_result default_match;/* * 1 if the directory (recursively) contains any provisionally @@ -363,8 +363,9 @@ static enum list_objects_filter_result filter_sparse( void *filter_data_) {struct filter_sparse_data *filter_data = filter_data_; -int val, dtype; +int dtype;struct frame *frame; +enum pattern_match_result match;switch (filter_situation) {default :@@ -373,15 +374,15 @@ static enum list_objects_filter_result filter_sparse(case LOFS_BEGIN_TREE :assert (obj->type ==OBJ_TREE ); dtype =DT_DIR ; - val =is_excluded_from_list (pathname,strlen (pathname), - filename, &dtype, &filter_data->pl, - r->index); -if (val <0 ) - val = filter_data->array_frame[filter_data->nr -1 ].defval; + match =path_matches_pattern_list (pathname,strlen (pathname), + filename, &dtype, &filter_data->pl, + r->index); +if (match ==UNDECIDED ) + match = filter_data->array_frame[filter_data->nr -1 ].default_match;ALLOC_GROW (filter_data->array_frame, filter_data->nr +1 , filter_data->alloc); - filter_data->array_frame[filter_data->nr].defval = val; + filter_data->array_frame[filter_data->nr].default_match = match; filter_data->array_frame[filter_data->nr].child_prov_omit =0 ; filter_data->nr++;@@ -435,12 +436,12 @@ static enum list_objects_filter_result filter_sparse( frame = &filter_data->array_frame[filter_data->nr -1 ]; dtype =DT_REG ; - val =is_excluded_from_list (pathname,strlen (pathname), + match =path_matches_pattern_list (pathname,strlen (pathname), filename, &dtype, &filter_data->pl, r->index); -if (val <0 ) - val = frame->defval; -if (val >0 ) { +if (match ==UNDECIDED ) + match = frame->default_match; +if (match ==MATCHED ) {if (omits)oidset_remove (omits, &obj->oid);return LOFR_MARK_SEEN |LOFR_DO_SHOW ;
list-objects-filter.c static void filter_sparse_oid__init( · line 491
Quoted 5 lines
list-objects-filter.c +1
-1@@ -487,7 +488,7 @@ static void filter_sparse_oid__init(die ("could not load filter specification" );ALLOC_GROW (d->array_frame, d->nr +1 , d->alloc); - d->array_frame[d->nr].defval =0 ;/* default to include */ + d->array_frame[d->nr].default_match =0 ;/* default to include */
s/0/MATCHED/ ?
Quoted 110 lines
list-objects-filter.c +0
-0d->array_frame[d->nr].child_prov_omit =0 ; d->nr++;unpack-trees.c +23
-17@@ -1265,7 +1265,8 @@ static int clear_ce_flags_1(struct index_state *istate, struct cache_entry **cache, int nr, struct strbuf *prefix, int select_mask, int clear_mask, - struct pattern_list *pl, int defval); + struct pattern_list *pl, + enum pattern_match_result default_match);/* Whole directory matching */ static int clear_ce_flags_dir(struct index_state *istate,@@ -1273,19 +1274,21 @@ static int clear_ce_flags_dir(struct index_state *istate,struct strbuf *prefix, char *basename, int select_mask, int clear_mask, - struct pattern_list *pl, int defval) + struct pattern_list *pl, + enum pattern_match_result default_match) { struct cache_entry **cache_end;int dtype =DT_DIR ; -int ret =is_excluded_from_list (prefix->buf, prefix->len, - basename, &dtype, pl, istate);int rc; +enum pattern_match_result ret; + ret =path_matches_pattern_list (prefix->buf, prefix->len, + basename, &dtype, pl, istate);strbuf_addch (prefix,'/' );/* If undecided, use matching result of parent dir in defval */ -if (ret <0 ) - ret = defval; +if (ret ==UNDECIDED ) + ret = default_match;for (cache_end = cache; cache_end != cache + nr; cache_end++) {struct cache_entry *ce = *cache_end;@@ -1298,7 +1301,7 @@ static int clear_ce_flags_dir(struct index_state *istate,* with ret (iow, we know in advance the incl/excl * decision for the entire directory), clear flag here without * calling clear_ce_flags_1(). That function will call -* the expensive is_excluded_from_list() on every entry. +* the expensive path_matches_pattern_list() on every entry. */ rc = clear_ce_flags_1(istate, cache, cache_end - cache, prefix,@@ -1327,7 +1330,8 @@ static int clear_ce_flags_1(struct index_state *istate, struct cache_entry **cache, int nr, struct strbuf *prefix, int select_mask, int clear_mask, - struct pattern_list *pl, int defval) + struct pattern_list *pl, + enum pattern_match_result default_match) { struct cache_entry **cache_end = cache + nr;@@ -1338,7 +1342,8 @@ static int clear_ce_flags_1(struct index_state *istate,while (cache != cache_end) {struct cache_entry *ce = *cache;const char *name, *slash; -int len, dtype, ret; +int len, dtype; +enum pattern_match_result ret;if (select_mask && !(ce->ce_flags & select_mask)) { cache++;@@ -1362,7 +1367,7 @@ static int clear_ce_flags_1(struct index_state *istate, prefix, prefix->buf + prefix->len - len, select_mask, clear_mask, - pl, defval); + pl, default_match);/* clear_c_f_dir eats a whole dir already? */ if (processed) {@@ -1374,18 +1379,20 @@ static int clear_ce_flags_1(struct index_state *istate,strbuf_addch (prefix,'/' ); cache +=clear_ce_flags_1 (istate, cache, cache_end - cache, prefix, - select_mask, clear_mask, pl, defval); + select_mask, clear_mask, pl, + default_match);strbuf_setlen (prefix, prefix->len - len -1 );continue ; }/* Non-directory */ dtype =ce_to_dtype (ce); - ret =is_excluded_from_list (ce->name,ce_namelen (ce), - name, &dtype, pl, istate); -if (ret <0 ) - ret = defval; -if (ret >0 ) + ret =path_matches_pattern_list (ce->name, +ce_namelen (ce), + name, &dtype, pl, istate); +if (ret ==UNDECIDED ) + ret = default_match; +if (ret ==MATCHED ) ce->ce_flags &= ~clear_mask; cache++; } --
Quoted 1 line
gitgitgadget
The rest looks good.
Quoted 9 lines · Derrick Stolee via GitGitGadget
The exclude library defined in
dir.hwas originally written for the .gitignore feature, but has since been used for .gitattributes and sparse-checkout. In the later applications, these patterns are used for inclusion rather than exclusion, so the name is confusing. This gets particularly bad when looking at how the sparse-checkout feature usesis_excluded_from_list()to really mean "should be included in the working directory".
Quoted 6 lines
This series performs several renames of structs and methods to generalize the exclude library to be a "pattern matching" library. Instead of a list of excludes, we have a list of path patterns. It is up to the consumer to decide what to do with a match. The .gitignore logic will still treat the patterns as a list of exclusions, the sparse-checkout logic treats the patterns as a list of inclusions.
Wahoo! Thanks for going through and cleaning these up. The possibility of negated patterns (or negated logic or both) on top of using "exclude" when "include" was _sometimes_ meant was just too much for me to keep track of. This series will make it much easier.
Quoted 6 lines
For this reason, some methods and structs in
dir.hretain "exclude" in their name. These are limited to things consumed only by the .gitignore feature (as far as I can tell).Most of these changes are mechanical find-and-replaces, with the exception of some variable names and the last patch.
I looked through the first four patches with --color-words=. to spot check them; as you say, looks like straightforward mechanical changes.
I did notice that the first two paragraphs were shared between the commit messages; it does make sense to help provide the context, but it seemed a little duplicative. I'm not sure what to suggest; it may be fine as it is. Just thought I'd flag it.
Quoted 11 lines
The last patch, "unpack-trees: rename '
is_excluded_from_list()'", performs a more meaningful refactor. The methodis_excluded_from_list()was only used by sparse-checkout (inside theclear_ce_flags()methods) to see if a path should be included in the working directory. The return value of "1 for excluded" was confusing. Instead, use a new enum value. This required changing several method prototypes insideunpack-trees.c.This refactor was inspired by Elijah Newren's feedback [1] on my sparse-checkout builtin RFC. I am working on a few other orthogonal changes to make to the existing sparse-checkout behavior before I resubmit that RFC.
Quoted 7 lines
I had started working on v2.23.0, but found adjacent-diff conflicts with md/list-objects-filter-combo [2] and js/partial-clone-sparse-blob [3]. Those branches are independent, but the conflicts with md/list-objects-filter-combo were more severe (and that branch seems closer to merging) so this is based on md/list-object-filter-combo. Hopefully the conflicts with js/partial-clone-sparse-blob are clear enough to resolve easily.
I posted some comments on patch 5; otherwise, the series looks good to me.
Quoted 7 lines · Derrick Stolee via GitGitGadget
I had started working on v2.23.0, but found adjacent-diff conflicts with md/list-objects-filter-combo [2] and js/partial-clone-sparse-blob [3]. Those branches are independent, but the conflicts with md/list-objects-filter-combo were more severe (and that branch seems closer to merging) so this is based on md/list-object-filter-combo. Hopefully the conflicts with js/partial-clone-sparse-blob are clear enough to resolve easily.
Thanks.
I did screw the resolution up in previous integration, but I think what we have today is in a good shape.