@matheusbsbs / matheus-bagliano / commits / 95c11ecc73

Fork 0

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>

Elijah Newren committed Apr 1, 2020 at 04:17 UTC 95c11ecc73f286e0a95d9591ae98f1221efe4633

6 files changed +18 -27

builtin/clean.c

-6

@@ -989,12 +989,6 @@ int cmd_clean(int argc, const char **argv, const char *prefix)

989 if (!cache_name_is_other(ent->name, ent->len))

990 continue;

991

992 - if (pathspec.nr)

993 - matches = dir_path_match(&the_index, ent, &pathspec, 0, NULL);

994 -

995 - if (pathspec.nr && !matches)

996 - continue;

997 -

992 if (lstat(ent->name, &st))

993 die_errno("Cannot lstat '%s'", ent->name);

994

builtin/grep.c

-2

@@ -691,8 +691,6 @@ static int grep_directory(struct grep_opt *opt, const struct pathspec *pathspec,

691

692 fill_directory(&dir, opt->repo->index, pathspec);

693 for (i = 0; i < dir.nr; i++) {

694 - if (!dir_path_match(opt->repo->index, dir.entries[i], pathspec, 0, NULL))

695 - continue;

694 hit |= grep_file(opt, dir.entries[i]->name);

695 if (hit && opt->status_only)

696 break;

builtin/ls-files.c

+3 -2

@@ -128,8 +128,9 @@ static void show_dir_entry(const struct index_state *istate,

128 if (len > ent->len)

129 die("git ls-files: internal error - directory entry not superset of prefix");

130

131 - if (!dir_path_match(istate, ent, &pathspec, len, ps_matched))

132 - return;

131 + /* If ps_matches is non-NULL, figure out which pathspec(s) match. */

132 + if (ps_matched)

133 + dir_path_match(istate, ent, &pathspec, len, ps_matched);

134

135 fputs(tag, stdout);

136 write_eolinfo(istate, NULL, ent->name);

builtin/stash.c

+5 -12

@@ -856,30 +856,23 @@ static int get_untracked_files(const struct pathspec *ps, int include_untracked,

856 struct strbuf *untracked_files)

857 {

858 int i;

859 - int max_len;

859 int found = 0;

861 - char *seen;

860 struct dir_struct dir;

861

862 memset(&dir, 0, sizeof(dir));

863 if (include_untracked != INCLUDE_ALL_FILES)

864 setup_standard_excludes(&dir);

865

868 - seen = xcalloc(ps->nr, 1);

869 -

870 - max_len = fill_directory(&dir, the_repository->index, ps);

866 + fill_directory(&dir, the_repository->index, ps);

867 for (i = 0; i < dir.nr; i++) {

868 struct dir_entry *ent = dir.entries[i];

873 - if (dir_path_match(&the_index, ent, ps, max_len, seen)) {

874 - found++;

875 - strbuf_addstr(untracked_files, ent->name);

876 - /* NUL-terminate: will be fed to update-index -z */

877 - strbuf_addch(untracked_files, '\0');

878 - }

869 + found++;

870 + strbuf_addstr(untracked_files, ent->name);

871 + /* NUL-terminate: will be fed to update-index -z */

872 + strbuf_addch(untracked_files, '\0');

873 free(ent);

874 }

875

882 - free(seen);

876 free(dir.entries);

877 free(dir.ignored);

878 clear_directory(&dir);

dir.c

+8 -1

@@ -2117,7 +2117,14 @@ static enum path_treatment treat_path(struct dir_struct *dir,

2117 baselen, excluded, pathspec);

2118 case DT_REG:

2119 case DT_LNK:

2120 - return excluded ? path_excluded : path_untracked;

2120 + if (excluded)

2121 + return path_excluded;

2122 + if (pathspec &&

2123 + !do_match_pathspec(istate, pathspec, path->buf, path->len,

2124 + 0 /* prefix */, NULL /* seen */,

2125 + 0 /* flags */))

2126 + return path_none;

2127 + return path_untracked;

2128 }

2129 }

2130

wt-status.c

+2 -4

@@ -722,16 +722,14 @@ static void wt_status_collect_untracked(struct wt_status *s)

722

723 for (i = 0; i < dir.nr; i++) {

724 struct dir_entry *ent = dir.entries[i];

725 - if (index_name_is_other(istate, ent->name, ent->len) &&

726 - dir_path_match(istate, ent, &s->pathspec, 0, NULL))

725 + if (index_name_is_other(istate, ent->name, ent->len))

726 string_list_insert(&s->untracked, ent->name);

727 free(ent);

728 }

729

730 for (i = 0; i < dir.ignored_nr; i++) {

731 struct dir_entry *ent = dir.ignored[i];

733 - if (index_name_is_other(istate, ent->name, ent->len) &&

734 - dir_path_match(istate, ent, &s->pathspec, 0, NULL))

732 + if (index_name_is_other(istate, ent->name, ent->len))

733 string_list_insert(&s->ignored, ent->name);

734 free(ent);

735 }