Fix error-prone fill_directory() API; make it only return matches

Traditionally, the expected calling convention for the dir.c API was:

    fill_directory(&dir, ..., pathspec)
    foreach entry in dir->entries:
        if (dir_path_match(entry, pathspec))
            process_or_display(entry)

This may have made sense once upon a time, because the fill_directory() call
could use cheap checks to avoid doing full pathspec matching, and an external
caller may have wanted to do other post-processing of the results anyway.
However:

    * this structure makes it easy for users of the API to get it wrong

    * this structure actually makes it harder to understand
      fill_directory() and the functions it uses internally.  It has
      tripped me up several times while trying to fix bugs and
      restructure things.

    * relying on post-filtering was already found to produce wrong
      results; pathspec matching had to be added internally for multiple
      cases in order to get the right results (see commits 404ebceda01c
      (dir: also check directories for matching pathspecs, 2019-09-17)
      and 89a1f4aaf765 (dir: if our pathspec might match files under a
      dir, recurse into it, 2019-09-17))

    * it's bad for performance: fill_directory() already has to do lots
      of checks and knows the subset of cases where it still needs to do
      more checks.  Forcing external callers to do full pathspec
      matching means they must re-check _every_ path.

So, add the pathspec matching within the fill_directory() internals, and
remove it from external callers.

Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/clean.c b/builtin/clean.c
index 5abf087e7c495153b36b927b5b615f80a68d414c..b189b7b4ea08de2c2c7450991800082516ec4a21 100644 (file)
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -989,12 +989,6 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
                if (!cache_name_is_other(ent->name, ent->len))
                        continue;
 
-               if (pathspec.nr)
-                       matches = dir_path_match(&the_index, ent, &pathspec, 0, NULL);
-
-               if (pathspec.nr && !matches)
-                       continue;
-
                if (lstat(ent->name, &st))
                        die_errno("Cannot lstat '%s'", ent->name);
 

diff --git a/builtin/grep.c b/builtin/grep.c
index ae2d5bbafcae263e91cd7f9ea7a97dc7a874d4aa..20bc8481317036b6dd57d11ebd28fd3bd9f33e62 100644 (file)
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -691,8 +691,6 @@ static int grep_directory(struct grep_opt *opt, const struct pathspec *pathspec,
 
        fill_directory(&dir, opt->repo->index, pathspec);
        for (i = 0; i < dir.nr; i++) {
-               if (!dir_path_match(opt->repo->index, dir.entries[i], pathspec, 0, NULL))
-                       continue;
                hit |= grep_file(opt, dir.entries[i]->name);
                if (hit && opt->status_only)
                        break;

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index f069a028cea1afa09edda8d039437fe39d9585ac..b87c22ac240834426e50c430ae1222f6866dc4b9 100644 (file)
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -128,8 +128,9 @@ static void show_dir_entry(const struct index_state *istate,
        if (len > ent->len)
                die("git ls-files: internal error - directory entry not superset of prefix");
 
-       if (!dir_path_match(istate, ent, &pathspec, len, ps_matched))
-               return;
+       /* If ps_matches is non-NULL, figure out which pathspec(s) match. */
+       if (ps_matched)
+               dir_path_match(istate, ent, &pathspec, len, ps_matched);
 
        fputs(tag, stdout);
        write_eolinfo(istate, NULL, ent->name);

diff --git a/builtin/stash.c b/builtin/stash.c
index 4ad3adf4ba5a01d78f75dfc59b7ee99b25880e0c..704740b245cec1166f3761c2aefc3466931951b5 100644 (file)
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -856,30 +856,23 @@ static int get_untracked_files(const struct pathspec *ps, int include_untracked,
                               struct strbuf *untracked_files)
 {
        int i;
-       int max_len;
        int found = 0;
-       char *seen;
        struct dir_struct dir;
 
        memset(&dir, 0, sizeof(dir));
        if (include_untracked != INCLUDE_ALL_FILES)
                setup_standard_excludes(&dir);
 
-       seen = xcalloc(ps->nr, 1);
-
-       max_len = fill_directory(&dir, the_repository->index, ps);
+       fill_directory(&dir, the_repository->index, ps);
        for (i = 0; i < dir.nr; i++) {
                struct dir_entry *ent = dir.entries[i];
-               if (dir_path_match(&the_index, ent, ps, max_len, seen)) {
-                       found++;
-                       strbuf_addstr(untracked_files, ent->name);
-                       /* NUL-terminate: will be fed to update-index -z */
-                       strbuf_addch(untracked_files, '\0');
-               }
+               found++;
+               strbuf_addstr(untracked_files, ent->name);
+               /* NUL-terminate: will be fed to update-index -z */
+               strbuf_addch(untracked_files, '\0');
                free(ent);
        }
 
-       free(seen);
        free(dir.entries);
        free(dir.ignored);
        clear_directory(&dir);

diff --git a/dir.c b/dir.c
index a67930dcff6414bebe99e161f8b30c05c8e87dd6..2de64910401540455217b1e5df6ffbdc9b31a295 100644 (file)
--- a/dir.c
+++ b/dir.c
@@ -2117,7 +2117,14 @@ static enum path_treatment treat_path(struct dir_struct *dir,
                                       baselen, excluded, pathspec);
        case DT_REG:
        case DT_LNK:
-               return excluded ? path_excluded : path_untracked;
+               if (excluded)
+                       return path_excluded;
+               if (pathspec &&
+                   !do_match_pathspec(istate, pathspec, path->buf, path->len,
+                                      0 /* prefix */, NULL /* seen */,
+                                      0 /* flags */))
+                       return path_none;
+               return path_untracked;
        }
 }
 

diff --git a/wt-status.c b/wt-status.c
index cc6f94504d9fa9f7ec80f5131996a10c878c26cb..98dfa6f73f9d7cd41867f97fdae41bd4dc5ec2a1 100644 (file)
--- a/wt-status.c
+++ b/wt-status.c
@@ -722,16 +722,14 @@ static void wt_status_collect_untracked(struct wt_status *s)
 
        for (i = 0; i < dir.nr; i++) {
                struct dir_entry *ent = dir.entries[i];
-               if (index_name_is_other(istate, ent->name, ent->len) &&
-                   dir_path_match(istate, ent, &s->pathspec, 0, NULL))
+               if (index_name_is_other(istate, ent->name, ent->len))
                        string_list_insert(&s->untracked, ent->name);
                free(ent);
        }
 
        for (i = 0; i < dir.ignored_nr; i++) {
                struct dir_entry *ent = dir.ignored[i];
-               if (index_name_is_other(istate, ent->name, ent->len) &&
-                   dir_path_match(istate, ent, &s->pathspec, 0, NULL))
+               if (index_name_is_other(istate, ent->name, ent->len))
                        string_list_insert(&s->ignored, ent->name);
                free(ent);
        }