summaryrefslogtreecommitdiff
path: root/dir.c
AgeCommit message (Collapse)AuthorFilesLines
2021-07-01dir.[ch]: replace dir_init() with DIR_INITLibravatar Ævar Arnfjörð Bjarmason1-7/+2
Remove the dir_init() function and replace it with a DIR_INIT macro. In many cases in the codebase we need to initialize things with a function for good reasons, e.g. needing to call another function on initialization. The "dir_init()" function was not one such case, and could trivially be replaced with a more idiomatic macro initialization pattern. The only place where we made use of its use of memset() was in dir_clear() itself, which resets the contents of an an existing struct pointer. Let's use the new "memcpy() a 'blank' struct on the stack" idiom to do that reset. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2021-05-28Merge branch 'en/dir-traversal'Libravatar Junio C Hamano1-2/+1
Fix-up to a topic that is already in 'master'. * en/dir-traversal: dir: introduce readdir_skip_dot_and_dotdot() helper dir: update stale description of treat_directory() Revert "dir: update stale description of treat_directory()" Revert "dir: introduce readdir_skip_dot_and_dotdot() helper"
2021-05-27dir: introduce readdir_skip_dot_and_dotdot() helperLibravatar Elijah Newren1-9/+16
Many places in the code were doing while ((d = readdir(dir)) != NULL) { if (is_dot_or_dotdot(d->d_name)) continue; ...process d... } Introduce a readdir_skip_dot_and_dotdot() helper to make that a one-liner: while ((d = readdir_skip_dot_and_dotdot(dir)) != NULL) { ...process d... } This helper particularly simplifies checks for empty directories. Also use this helper in read_cached_dir() so that our statistics are consistent across platforms. (In other words, read_cached_dir() should have been using is_dot_or_dotdot() and skipping such entries, but did not and left it to treat_path() to detect and mark such entries as path_none.) Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2021-05-27dir: update stale description of treat_directory()Libravatar Derrick Stolee1-7/+6
The documentation comment for treat_directory() was originally written in 095952 (Teach directory traversal about subprojects, 2007-04-11) which was before the 'struct dir_struct' split its bitfield of named options into a 'flags' enum in 7c4c97c0 (Turn the flags in struct dir_struct into a single variable, 2009-02-16). When those flags changed, the comment became stale, since members like 'show_other_directories' transitioned into flags like DIR_SHOW_OTHER_DIRECTORIES. Update the comments for treat_directory() to use these flag names rather than the old member names. Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2021-05-27Revert "dir: update stale description of treat_directory()"Libravatar Junio C Hamano1-6/+7
This reverts commit 4e689d81718eb6e939cace317ea3e33cb994dcbb, to be replaced with a reworked version.
2021-05-27Revert "dir: introduce readdir_skip_dot_and_dotdot() helper"Libravatar Junio C Hamano1-17/+9
This reverts commit b548f0f1568f6b01e55ca69c24d3cb19489f92aa, to be replaced with a reworked version.
2021-05-20Merge branch 'en/dir-traversal'Libravatar Junio C Hamano1-45/+101
"git clean" and "git ls-files -i" had confusion around working on or showing ignored paths inside an ignored directory, which has been corrected. * en/dir-traversal: dir: introduce readdir_skip_dot_and_dotdot() helper dir: update stale description of treat_directory() dir: traverse into untracked directories if they may have ignored subfiles dir: avoid unnecessary traversal into ignored directory t3001, t7300: add testcase showcasing missed directory traversal t7300: add testcase showing unnecessary traversal into ignored directory ls-files: error out on -i unless -o or -c are specified dir: report number of visited directories and paths with trace2 dir: convert trace calls to trace2 equivalents
2021-05-13dir: introduce readdir_skip_dot_and_dotdot() helperLibravatar Elijah Newren1-9/+17
Many places in the code were doing while ((d = readdir(dir)) != NULL) { if (is_dot_or_dotdot(d->d_name)) continue; ...process d... } Introduce a readdir_skip_dot_and_dotdot() helper to make that a one-liner: while ((d = readdir_skip_dot_and_dotdot(dir)) != NULL) { ...process d... } This helper particularly simplifies checks for empty directories. Also use this helper in read_cached_dir() so that our statistics are consistent across platforms. (In other words, read_cached_dir() should have been using is_dot_or_dotdot() and skipping such entries, but did not and left it to treat_path() to detect and mark such entries as path_none.) Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2021-05-13dir: update stale description of treat_directory()Libravatar Derrick Stolee1-7/+6
The documentation comment for treat_directory() was originally written in 095952 (Teach directory traversal about subprojects, 2007-04-11) which was before the 'struct dir_struct' split its bitfield of named options into a 'flags' enum in 7c4c97c0 (Turn the flags in struct dir_struct into a single variable, 2009-02-16). When those flags changed, the comment became stale, since members like 'show_other_directories' transitioned into flags like DIR_SHOW_OTHER_DIRECTORIES. Update the comments for treat_directory() to use these flag names rather than the old member names. Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2021-05-13dir: traverse into untracked directories if they may have ignored subfilesLibravatar Elijah Newren1-4/+6
A directory that is untracked does not imply that all files under it should be categorized as untracked; in particular, if the caller is interested in ignored files, many files or directories underneath the untracked directory may be ignored. We previously partially handled this right with DIR_SHOW_IGNORED_TOO, but missed DIR_SHOW_IGNORED. It was not obvious, though, because the logic for untracked and excluded files had been fused together making it harder to reason about. The previous commit split that logic out, making it easier to notice that DIR_SHOW_IGNORED was missing. Add it. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2021-05-13dir: avoid unnecessary traversal into ignored directoryLibravatar Elijah Newren1-15/+29
The show_other_directories case in treat_directory() tried to handle both excludes and untracked files with the same logic, and mishandled both the excludes and the untracked files in the process, in different ways. Split that logic apart, and then focus on the logic for the excludes; a subsequent commit will address the logic for untracked files. For show_other_directories, an excluded directory means that every path underneath that directory will also be excluded. Given that the calling code requested to just show directories when everything under a directory had the same state (that's what the "DIR_SHOW_OTHER_DIRECTORIES" flag means), we generally do not need to traverse into such directories and can just immediately mark them as ignored (i.e. as path_excluded). The only reason we cannot just immediately return path_excluded is the DIR_HIDE_EMPTY_DIRECTORIES flag and the possibility that the ignored directory is an empty directory. The code previously treated DIR_SHOW_IGNORED_TOO in most cases as an exception as well, which was wrong. It can sometimes reduce the number of cases where we need to recurse (namely if DIR_SHOW_IGNORED_TOO_MODE_MATCHING is also set), but should not be able to increase the number of cases where we need to recurse. Fix the logic accordingly. Some sidenotes about possible confusion with dir.c: * "ignored" often refers to an untracked ignore", i.e. a file which is not tracked which matches one of the ignore/exclusion rules. But you can also have a "tracked ignore", a tracked file that happens to match one of the ignore/exclusion rules and which dir.c has to worry about since "git ls-files -c -i" is supposed to list them. * The dir code often uses "ignored" and "excluded" interchangeably, which you need to keep in mind while reading the code. * "exclude" is used multiple ways in the code: * As noted above, "exclude" is often a synonym for "ignored". * The logic for parsing .gitignore files was re-used in .git/info/sparse-checkout, except there it is used to mark paths that the user wants to *keep*. This was mostly addressed by commit 65edd96aec ("treewide: rename 'exclude' methods to 'pattern'", 2019-09-03), but every once in a while you'll find a comment about "exclude" referring to these patterns that might in fact be in use by the sparse-checkout machinery for inclusion rules. * The word "EXCLUDE" is also used for pathspec negation, as in (pathspec->items[3].magic & PATHSPEC_EXCLUDE) Thus if a user had a .gitignore file containing *~ *.log !settings.log And then ran git add -- 'settings.*' ':^settings.log' Then :^settings.log is a pathspec negation making settings.log not be requested to be added even though all other settings.* files are being added. Also, !settings.log in the gitignore file is a negative exclude pattern meaning that settings.log is normally a file we want to track even though all other *.log files are ignored. Sometimes it feels like dir.c needs its own glossary with its many definitions, including the multiply-defined terms. Reported-by: Jason Gore <Jason.Gore@microsoft.com> Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2021-05-13dir: report number of visited directories and paths with trace2Libravatar Elijah Newren1-0/+9
Provide more statistics in trace2 output that include the number of directories and total paths visited by the directory traversal logic. Subsequent patches will take advantage of this to ensure we do not unnecessarily traverse into ignored directories. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2021-05-13dir: convert trace calls to trace2 equivalentsLibravatar Elijah Newren1-13/+37
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2021-05-10Merge branch 'bc/hash-transition-interop-part-1'Libravatar Junio C Hamano1-3/+3
SHA-256 transition. * bc/hash-transition-interop-part-1: hex: print objects using the hash algorithm member hex: default to the_hash_algo on zero algorithm value builtin/pack-objects: avoid using struct object_id for pack hash commit-graph: don't store file hashes as struct object_id builtin/show-index: set the algorithm for object IDs hash: provide per-algorithm null OIDs hash: set, copy, and use algo field in struct object_id builtin/pack-redundant: avoid casting buffers to struct object_id Use the final_oid_fn to finalize hashing of object IDs hash: add a function to finalize object IDs http-push: set algorithm when reading object ID Always use oidread to read into struct object_id hash: add an algo member to struct object_id
2021-04-30Merge branch 'ds/sparse-index-protections'Libravatar Junio C Hamano1-6/+8
Builds on top of the sparse-index infrastructure to mark operations that are not ready to mark with the sparse index, causing them to fall back on fully-populated index that they always have worked with. * ds/sparse-index-protections: (47 commits) name-hash: use expand_to_path() sparse-index: expand_to_path() name-hash: don't add directories to name_hash revision: ensure full index resolve-undo: ensure full index read-cache: ensure full index pathspec: ensure full index merge-recursive: ensure full index entry: ensure full index dir: ensure full index update-index: ensure full index stash: ensure full index rm: ensure full index merge-index: ensure full index ls-files: ensure full index grep: ensure full index fsck: ensure full index difftool: ensure full index commit: ensure full index checkout: ensure full index ...
2021-04-27hash: provide per-algorithm null OIDsLibravatar brian m. carlson1-1/+1
Up until recently, object IDs did not have an algorithm member, only a hash. Consequently, it was possible to share one null (all-zeros) object ID among all hash algorithms. Now that we're going to be handling objects from multiple hash algorithms, it's important to make sure that all object IDs have a correct algorithm field. Introduce a per-algorithm null OID, and add it to struct hash_algo. Introduce a wrapper function as well, and use it everywhere we used to use the null_oid constant. Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2021-04-27Always use oidread to read into struct object_idLibravatar brian m. carlson1-2/+2
In the future, we'll want oidread to automatically set the hash algorithm member for an object ID we read into it, so ensure we use oidread instead of hashcpy everywhere we're copying a hash value into a struct object_id. Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2021-04-14dir: ensure full indexLibravatar Derrick Stolee1-0/+2
Before iterating over all cache entries, ensure that a sparse index is expanded to a full index to avoid unexpected behavior. Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2021-04-14*: remove 'const' qualifier for struct index_stateLibravatar Derrick Stolee1-6/+6
Several methods specify that they take a 'struct index_state' pointer with the 'const' qualifier because they intend to only query the data, not change it. However, we will be introducing a step very low in the method stack that might modify a sparse-index to become a full index in the case that our queries venture inside a sparse-directory entry. This change only removes the 'const' qualifiers that are necessary for the following change which will actually modify the implementation of index_name_stage_pos(). Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2021-03-22Merge branch 'jk/open-dotgitx-with-nofollow'Libravatar Junio C Hamano1-6/+15
It does not make sense to make ".gitattributes", ".gitignore" and ".mailmap" symlinks, as they are supposed to be usable from the object store (think: bare repositories where HEAD:.mailmap etc. are used). When these files are symbolic links, we used to read the contents of the files pointed by them by mistake, which has been corrected. * jk/open-dotgitx-with-nofollow: mailmap: do not respect symlinks for in-tree .mailmap exclude: do not respect symlinks for in-tree .gitignore attr: do not respect symlinks for in-tree .gitattributes exclude: add flags parameter to add_patterns() attr: convert "macro_ok" into a flags field add open_nofollow() helper
2021-03-13use CALLOC_ARRAYLibravatar René Scharfe1-3/+3
Add and apply a semantic patch for converting code that open-codes CALLOC_ARRAY to use it instead. It shortens the code and infers the element size automatically. Signed-off-by: René Scharfe <l.s.r@web.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2021-03-01Merge branch 'jh/untracked-cache-fix'Libravatar Junio C Hamano1-5/+2
An under-allocation for the untracked cache data has been corrected. * jh/untracked-cache-fix: dir: fix malloc of root untracked_cache_dir
2021-02-24dir: fix malloc of root untracked_cache_dirLibravatar Jeff Hostetler1-5/+2
Use FLEX_ALLOC_STR() to allocate the `struct untracked_cache_dir` for the root directory. Get rid of unsafe code that might fail to initialize the `name` field (if FLEX_ARRAY is not 1). This will make it clear that we intend to have a structure with an empty string following it. A problem was observed on Windows where the length of the memset() was too short, so the first byte of the name field was not zeroed. This resulted in the name field having garbage from a previous use of that area of memory. The record for the root directory was then written to the untracked-cache extension in the index. This garbage would then be visible to future commands when they reloaded the untracked-cache extension. Since the directory record for the root directory had garbage in the `name` field, the `t/helper/test-tool dump-untracked-cache` tool printed this garbage as the path prefix (rather than '/') for each directory in the untracked cache as it recursed. Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com> Reviewed-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2021-02-16exclude: do not respect symlinks for in-tree .gitignoreLibravatar Jeff King1-2/+10
As with .gitattributes, we would like to make sure that .gitignore files are handled consistently whether read from the index or from the filesystem. Likewise, we would like to avoid reading out-of-tree files pointed to by the symlinks, which could have security implications in certain setups. We can cover both by using open_nofollow() when opening the in-tree files. We'll continue to follow links for core.excludesFile, as well as $GIT_DIR/info/exclude. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2021-02-16exclude: add flags parameter to add_patterns()Libravatar Jeff King1-6/+7
There are a number of callers of add_patterns() and its sibling functions. Let's give them a "flags" parameter for adding new options without having to touch each caller. We'll use this in a future patch to add O_NOFOLLOW support. But for now each caller just passes 0. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2021-01-23sparse-checkout: load sparse-checkout patternsLibravatar Derrick Stolee1-0/+17
A future feature will want to load the sparse-checkout patterns into a pattern_list, but the current mechanism to do so is a bit complicated. This is made difficult due to needing to find the sparse-checkout file in different ways throughout the codebase. The logic implemented in the new get_sparse_checkout_patterns() was duplicated in populate_from_existing_patterns() in unpack-trees.c. Use the new method instead, keeping the logic around handling the struct unpack_trees_options. The callers to get_sparse_checkout_filename() in builtin/sparse-checkout.c manipulate the sparse-checkout file directly, so it is not appropriate to replace logic in that file with get_sparse_checkout_patterns(). Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-11-21Merge branch 'en/strmap'Libravatar Junio C Hamano1-4/+4
A specialization of hashmap that uses a string as key has been introduced. Hopefully it will see wider use over time. * en/strmap: shortlog: use strset from strmap.h Use new HASHMAP_INIT macro to simplify hashmap initialization strmap: take advantage of FLEXPTR_ALLOC_STR when relevant strmap: enable allocations to come from a mem_pool strmap: add a strset sub-type strmap: split create_entry() out of strmap_put() strmap: add functions facilitating use as a string->int map strmap: enable faster clearing and reusing of strmaps strmap: add more utility functions strmap: new utility functions hashmap: provide deallocation function names hashmap: introduce a new hashmap_partial_clear() hashmap: allow re-use after hashmap_free() hashmap: adjust spacing to fix argument alignment hashmap: add usage documentation explaining hashmap_free[_entries]()
2020-11-02Merge branch 'nk/dir-c-comment-update'Libravatar Junio C Hamano1-3/+3
Update stale in-code comment. * nk/dir-c-comment-update: dir.c: fix comments to agree with argument name
2020-11-02hashmap: provide deallocation function namesLibravatar Elijah Newren1-4/+4
hashmap_free(), hashmap_free_entries(), and hashmap_free_() have existed for a while, but aren't necessarily the clearest names, especially with hashmap_partial_clear() being added to the mix and lazy-initialization now being supported. Peff suggested we adopt the following names[1]: - hashmap_clear() - remove all entries and de-allocate any hashmap-specific data, but be ready for reuse - hashmap_clear_and_free() - ditto, but free the entries themselves - hashmap_partial_clear() - remove all entries but don't deallocate table - hashmap_partial_clear_and_free() - ditto, but free the entries This patch provides the new names and converts all existing callers over to the new naming scheme. [1] https://lore.kernel.org/git/20201030125059.GA3277724@coredump.intra.peff.net/ Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-10-16dir.c: fix comments to agree with argument nameLibravatar Alex Vandiver1-3/+3
Signed-off-by: Alex Vandiver <alexmv@dropbox.com> Signed-off-by: Nipunn Koorapati <nipunn@dropbox.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-09-30dir.c: drop unused "untracked" from treat_path_fast()Libravatar Jeff King1-2/+1
We don't use the untracked_cache_dir parameter that is passed in, but instead look at the untracked_cache_dir inside the cached_dir struct we are passed. It's been this way since the introduction of treat_path_fast() in 91a2288b5f (untracked cache: record/validate dir mtime and reuse cached output, 2015-03-08). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-08-27Merge branch 'jk/leakfix'Libravatar Junio C Hamano1-0/+2
Code clean-up. * jk/leakfix: submodule--helper: fix leak of core.worktree value config: fix leak in git_config_get_expiry_in_days() config: drop git_config_get_string_const() config: fix leaks from git_config_get_string_const() checkout: fix leak of non-existent branch names submodule--helper: use strbuf_release() to free strbufs clear_pattern_list(): clear embedded hashmaps
2020-08-24Merge branch 'en/dir-clear'Libravatar Junio C Hamano1-3/+17
Leakfix with code clean-up. * en/dir-clear: dir: fix problematic API to avoid memory leaks dir: make clear_directory() free all relevant memory
2020-08-24Merge branch 'en/dir-nonbare-embedded'Libravatar Junio C Hamano1-3/+6
"ls-files -o" mishandled the top-level directory of another git working tree that hangs in the current git working tree. * en/dir-nonbare-embedded: dir: avoid prematurely marking nonbare repositories as matches t3000: fix some test description typos
2020-08-18dir: fix problematic API to avoid memory leaksLibravatar Elijah Newren1-2/+7
The dir structure seemed to have a number of leaks and problems around it. First I noticed that parent_hashmap and recursive_hashmap were being leaked (though Peff noticed and submitted fixes before me). Then I noticed in the previous commit that clear_directory() was only taking responsibility for a subset of fields within dir_struct, despite the fact that entries[] and ignored[] we allocated internally to dir.c. That, of course, resulted in many callers either leaking or haphazardly trying to free these arrays and their contents. Digging further, I found that despite the pretty clear documentation near the top of dir.h that folks were supposed to call clear_directory() when the user no longer needed the dir_struct, there were four callers that didn't bother doing that at all. However, two of them clearly thought about leaks since they had an UNLEAK(dir) directive, which to me suggests that the method to free the data was too unclear. I suspect the non-obviousness of the API and its holes led folks to avoid it, which then snowballed into further problems with the entries[], ignored[], parent_hashmap, and recursive_hashmap problems. Rename clear_directory() to dir_clear() to be more in line with other data structures in git, and introduce a dir_init() to handle the suggested memsetting of dir_struct to all zeroes. I hope that a name like "dir_clear()" is more clear, and that the presence of dir_init() will provide a hint to those looking at the code that they need to look for either a dir_clear() or a dir_free() and lead them to find dir_clear(). Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-08-18dir: make clear_directory() free all relevant memoryLibravatar Elijah Newren1-2/+11
The calling convention for the dir API is supposed to end with a call to clear_directory() to free up no longer needed memory. However, clear_directory() didn't free dir->entries or dir->ignored. I believe this was an oversight, but a number of callers noticed memory leaks and started free'ing these. Unfortunately, they did so somewhat haphazardly (sometimes freeing the entries in the arrays, and sometimes only free'ing the arrays themselves). This suggests the callers weren't trying to make sure any possible memory used might be free'd, but just the memory they noticed their usecase definitely had allocated. Fix this mess by moving all the duplicated free'ing logic into clear_directory(). End by resetting dir to a pristine state so it could be reused if desired. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-08-14clear_pattern_list(): clear embedded hashmapsLibravatar Jeff King1-0/+2
Commit 96cc8ab531 (sparse-checkout: use hashmaps for cone patterns, 2019-11-21) added some auxiliary hashmaps to the pattern_list struct, but they're leaked when clear_pattern_list() is called. Signed-off-by: Jeff King <peff@peff.net> Acked-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-08-12dir: avoid prematurely marking nonbare repositories as matchesLibravatar Elijah Newren1-3/+6
Nonbare repositories are special directories. Unlike normal directories that we might recurse into to list the files they contain, nonbare repositories must themselves match and then we always report only on the nonbare repository directory itself and not on any of its contents. Separately, when traversing directories to try to find untracked or excluded files, we often think in terms of paths either matching the specified pathspec, or not matching them. However, there is a special value that do_match_pathspec() uses named MATCHED_RECURSIVELY_LEADING_PATHSPEC which means "this directory does not match any pathspec BUT it is possible a file or directory underneath it does." That special value prevents us from prematurely thinking that some directory and everything under it is irrelevant, but also allows us to differentiate from "this is a match". The combination of these two special cases was previously uncovered. Add a test to the testsuite to cover it, and make sure that we return a nonbare repository as a non-match if the best match it got was MATCHED_RECURSIVELY_LEADING_PATHSPEC. Reported-by: christian w <usebees@gmail.com> Simplified-testcase-and-bisection-by: Kyle Meyer <kyle@kyleam.com> Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-07-30Merge branch 'en/fill-directory-exponential' into masterLibravatar Junio C Hamano1-2/+2
Fix to a regression introduced during 2.27 cycle. * en/fill-directory-exponential: dir: check pathspecs before returning `path_excluded`
2020-07-20dir: check pathspecs before returning `path_excluded`Libravatar Martin Ågren1-2/+2
In 95c11ecc73 ("Fix error-prone fill_directory() API; make it only return matches", 2020-04-01), we taught `fill_directory()`, or more specifically `treat_path()`, to check against any pathspecs so that we could simplify the callers. But in doing so, we added a slightly-too-early return for the "excluded" case. We end up not checking the pathspecs, meaning we return `path_excluded` when maybe we should return `path_none`. As a result, `git status --ignored -- pathspec` might show paths that don't actually match "pathspec". Move the "excluded" check down to after we've checked any pathspecs. Reported-by: Andreas Schwab <schwab@linux-m68k.org> Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Martin Ågren <martin.agren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-06-25Merge branch 'en/clean-cleanups'Libravatar Junio C Hamano1-5/+10
Code clean-up of "git clean" resulted in a fix of recent performance regression. * en/clean-cleanups: clean: optimize and document cases where we recurse into subdirectories clean: consolidate handling of ignored parameters dir, clean: avoid disallowed behavior dir: fix a few confusing comments
2020-06-17Merge branch 'en/do-match-pathspec-fix'Libravatar Junio C Hamano1-17/+33
Use of negative pathspec, while collecting paths including untracked ones in the working tree, was broken. * en/do-match-pathspec-fix: dir: fix treatment of negated pathspecs
2020-06-12dir, clean: avoid disallowed behaviorLibravatar Elijah Newren1-0/+4
dir.h documented quite clearly that DIR_SHOW_IGNORED and DIR_SHOW_IGNORED_TOO are mutually exclusive, with a big comment to this effect by the definition of both enum values. However, a command like git clean -fx $DIR would set both values for dir.flags. I _think_ it happened to work because: * As dir.h points out, DIR_KEEP_UNTRACKED_CONTENTS only takes effect if DIR_SHOW_IGNORED_TOO is set. * As coded, I believe DIR_SHOW_IGNORED would just happen to take precedence over DIR_SHOW_IGNORED_TOO in the code as currently constructed. Which is a long way of saying "we just got lucky". Fix clean.c to avoid setting these mutually exclusive values at the same time, and add a check to dir.c that will throw a BUG() to prevent anyone else from making this mistake. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-06-12dir: fix a few confusing commentsLibravatar Elijah Newren1-5/+6
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-06-05dir: fix treatment of negated pathspecsLibravatar Elijah Newren1-17/+33
do_match_pathspec() started life as match_pathspec_depth_1() and for correctness was only supposed to be called from match_pathspec_depth(). match_pathspec_depth() was later renamed to match_pathspec(), so the invariant we expect today is that do_match_pathspec() has no direct callers outside of match_pathspec(). Unfortunately, this intention was lost with the renames of the two functions, and additional calls to do_match_pathspec() were added in commits 75a6315f74 ("ls-files: add pathspec matching for submodules", 2016-10-07) and 89a1f4aaf7 ("dir: if our pathspec might match files under a dir, recurse into it", 2019-09-17). Of course, do_match_pathspec() had an important advantge over match_pathspec() -- match_pathspec() would hardcode flags to one of two values, and these new callers needed to pass some other value for flags. Also, although calling do_match_pathspec() directly was incorrect, there likely wasn't any difference in the observable end output, because the bug just meant that fill_diretory() would recurse into unneeded directories. Since subsequent does-this-path-match checks on individual paths under the directory would cause those extra paths to be filtered out, the only difference from using the wrong function was unnecessary computation. The second of those bad calls to do_match_pathspec() was involved -- via either direct movement or via copying+editing -- into a number of later refactors. See commits 777b420347 ("dir: synchronize treat_leading_path() and read_directory_recursive()", 2019-12-19), 8d92fb2927 ("dir: replace exponential algorithm with a linear one", 2020-04-01), and 95c11ecc73 ("Fix error-prone fill_directory() API; make it only return matches", 2020-04-01). The last of those introduced the usage of do_match_pathspec() on an individual file, and thus resulted in individual paths being returned that shouldn't be. The problem with calling do_match_pathspec() instead of match_pathspec() is that any negated patterns such as ':!unwanted_path` will be ignored. Add a new match_pathspec_with_flags() function to fulfill the needs of specifying special flags while still correctly checking negated patterns, add a big comment above do_match_pathspec() to prevent others from misusing it, and correct current callers of do_match_pathspec() to instead use either match_pathspec() or match_pathspec_with_flags(). One final note is that DO_MATCH_LEADING_PATHSPEC needs special consideration when working with DO_MATCH_EXCLUDE. The point of DO_MATCH_LEADING_PATHSPEC is that if we have a pathspec like */Makefile and we are checking a directory path like src/module/component that we want to consider it a match so that we recurse into the directory because it _might_ have a file named Makefile somewhere below. However, when we are using an exclusion pattern, i.e. we have a pathspec like :(exclude)*/Makefile we do NOT want to say that a directory path like src/module/component is a (negative) match. While there *might* be a file named 'Makefile' somewhere below that directory, there could also be other files and we cannot pre-emptively rule all the files under that directory out; we need to recurse and then check individual files. Adjust the DO_MATCH_LEADING_PATHSPEC logic to only get activated for positive pathspecs. Reported-by: John Millikin <jmillikin@stripe.com> Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-04-29Merge branch 'en/fill-directory-exponential'Libravatar Junio C Hamano1-174/+248
The directory traversal code had redundant recursive calls which made its performance characteristics exponential with respect to the depth of the tree, which was corrected. * en/fill-directory-exponential: completion: fix 'git add' on paths under an untracked directory Fix error-prone fill_directory() API; make it only return matches dir: replace double pathspec matching with single in treat_directory() dir: include DIR_KEEP_UNTRACKED_CONTENTS handling in treat_directory() dir: replace exponential algorithm with a linear one dir: refactor treat_directory to clarify control flow dir: fix confusion based on variable tense dir: fix broken comment dir: consolidate treat_path() and treat_one_path() dir: fix simple typo in comment t3000: add more testcases testing a variety of ls-files issues t7063: more thorough status checking
2020-04-01Fix error-prone fill_directory() API; make it only return matchesLibravatar Elijah Newren1-1/+8
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>
2020-04-01dir: replace double pathspec matching with single in treat_directory()Libravatar Elijah Newren1-19/+19
treat_directory() had a call to both do_match_pathspec() and match_pathspec(). These calls have migrated through the code somewhat since their introduction, but we don't actually need both. Replace the two calls with one, and while at it, move the check earlier in order to reduce the need for callers of fill_directory() to do post-filtering of results. The next patch will address post-filtering more forcefully and provide more relevant history and context. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-04-01dir: include DIR_KEEP_UNTRACKED_CONTENTS handling in treat_directory()Libravatar Elijah Newren1-24/+19
Handling DIR_KEEP_UNTRACKED_CONTENTS within treat_directory() instead of as a post-processing step in read_directory(): * allows us to directly access and remove the relevant entries instead of needing to calculate which ones need to be removed * keeps the logic for directory handling in one location (and puts it closer the the logic for stripping out extra ignored entries, which seems logical). Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-04-01dir: replace exponential algorithm with a linear oneLibravatar Elijah Newren1-63/+147
dir's read_directory_recursive() naturally operates recursively in order to walk the directory tree. Treating of directories is sometimes weird because there are so many different permutations about how to handle directories. Some examples: * 'git ls-files -o --directory' only needs to know that a directory itself is untracked; it doesn't need to recurse into it to see what is underneath. * 'git status' needs to recurse into an untracked directory, but only to determine whether or not it is empty. If there are no files underneath, the directory itself will be omitted from the output. If it is not empty, only the directory will be listed. * 'git status --ignored' needs to recurse into untracked directories and report all the ignored entries and then report the directory as untracked -- UNLESS all the entries under the directory are ignored, in which case we don't print any of the entries under the directory and just report the directory itself as ignored. (Note that although this forces us to walk all untracked files underneath the directory as well, we strip them from the output, except for users like 'git clean' who also set DIR_KEEP_TRACKED_CONTENTS.) * For 'git clean', we may need to recurse into a directory that doesn't match any specified pathspecs, if it's possible that there is an entry underneath the directory that can match one of the pathspecs. In such a case, we need to be careful to omit the directory itself from the list of paths (see commit 404ebceda01c ("dir: also check directories for matching pathspecs", 2019-09-17)) Part of the tension noted above is that the treatment of a directory can change based on the files within it, and based on the various settings in dir->flags. Trying to keep this in mind while reading over the code, it is easy to think in terms of "treat_directory() tells us what to do with a directory, and read_directory_recursive() is the thing that recurses". Since we need to look into a directory to know how to treat it, though, it is quite easy to decide to (also) recurse into the directory from treat_directory() by adding a read_directory_recursive() call. Adding such a call is actually fine, IF we make sure that read_directory_recursive() does not also recurse into that same directory. Unfortunately, commit df5bcdf83aeb ("dir: recurse into untracked dirs for ignored files", 2017-05-18), added exactly such a case to the code, meaning we'd have two calls to read_directory_recursive() for an untracked directory. So, if we had a file named one/two/three/four/five/somefile.txt and nothing in one/ was tracked, then 'git status --ignored' would call read_directory_recursive() twice on the directory 'one/', and each of those would call read_directory_recursive() twice on the directory 'one/two/', and so on until read_directory_recursive() was called 2^5 times for 'one/two/three/four/five/'. Avoid calling read_directory_recursive() twice per level by moving a lot of the special logic into treat_directory(). Since dir.c is somewhat complex, extra cruft built up around this over time. While trying to unravel it, I noticed several instances where the first call to read_directory_recursive() would return e.g. path_untracked for some directory and a later one would return e.g. path_none, despite the fact that the directory clearly should have been considered untracked. The code happened to work due to the side-effect from the first invocation of adding untracked entries to dir->entries; this allowed it to get the correct output despite the supposed override in return value by the later call. I am somewhat concerned that there are still bugs and maybe even testcases with the wrong expectation. I have tried to carefully document treat_directory() since it becomes more complex after this change (though much of this complexity came from elsewhere that probably deserved better comments to begin with). However, much of my work felt more like a game of whackamole while attempting to make the code match the existing regression tests than an attempt to create an implementation that matched some clear design. That seems wrong to me, but the rules of existing behavior had so many special cases that I had a hard time coming up with some overarching rules about what correct behavior is for all cases, forcing me to hope that the regression tests are correct and sufficient. Such a hope seems likely to be ill-founded, given my experience with dir.c-related testcases in the last few months: Examples where the documentation was hard to parse or even just wrong: * 3aca58045f4f (git-clean.txt: do not claim we will delete files with -n/--dry-run, 2019-09-17) * 09487f2cbad3 (clean: avoid removing untracked files in a nested git repository, 2019-09-17) * e86bbcf987fa (clean: disambiguate the definition of -d, 2019-09-17) Examples where testcases were declared wrong and changed: * 09487f2cbad3 (clean: avoid removing untracked files in a nested git repository, 2019-09-17) * e86bbcf987fa (clean: disambiguate the definition of -d, 2019-09-17) * a2b13367fe55 (Revert "dir.c: make 'git-status --ignored' work within leading directories", 2019-12-10) Examples where testcases were clearly inadequate: * 502c386ff944 (t7300-clean: demonstrate deleting nested repo with an ignored file breakage, 2019-08-25) * 7541cc530239 (t7300: add testcases showing failure to clean specified pathspecs, 2019-09-17) * a5e916c7453b (dir: fix off-by-one error in match_pathspec_item, 2019-09-17) * 404ebceda01c (dir: also check directories for matching pathspecs, 2019-09-17) * 09487f2cbad3 (clean: avoid removing untracked files in a nested git repository, 2019-09-17) * e86bbcf987fa (clean: disambiguate the definition of -d, 2019-09-17) * 452efd11fbf6 (t3011: demonstrate directory traversal failures, 2019-12-10) * b9670c1f5e6b (dir: fix checks on common prefix directory, 2019-12-19) Examples where "correct behavior" was unclear to everyone: https://lore.kernel.org/git/20190905154735.29784-1-newren@gmail.com/ Other commits of note: * 902b90cf42bc (clean: fix theoretical path corruption, 2019-09-17) However, on the positive side, it does make the code much faster. For the following simple shell loop in an empty repository: for depth in $(seq 10 25) do dirs=$(for i in $(seq 1 $depth) ; do printf 'dir/' ; done) rm -rf dir mkdir -p $dirs >$dirs/untracked-file /usr/bin/time --format="$depth: %e" git status --ignored >/dev/null done I saw the following timings, in seconds (note that the numbers are a little noisy from run-to-run, but the trend is very clear with every run): 10: 0.03 11: 0.05 12: 0.08 13: 0.19 14: 0.29 15: 0.50 16: 1.05 17: 2.11 18: 4.11 19: 8.60 20: 17.55 21: 33.87 22: 68.71 23: 140.05 24: 274.45 25: 551.15 For the above run, using strace I can look for the number of untracked directories opened and can verify that it matches the expected 2^($depth+1)-2 (the sum of 2^1 + 2^2 + 2^3 + ... + 2^$depth). After this fix, with strace I can verify that the number of untracked directories that are opened drops to just $depth, and the timings all drop to 0.00. In fact, it isn't until a depth of 190 nested directories that it sometimes starts reporting a time of 0.01 seconds and doesn't consistently report 0.01 seconds until there are 240 nested directories. The previous code would have taken 17.55 * 2^220 / (60*60*24*365) = 9.4 * 10^59 YEARS to have completed the 240 nested directories case. It's not often that you get to speed something up by a factor of 3*10^69. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>