summaryrefslogtreecommitdiff
path: root/dir.c
AgeCommit message (Collapse)AuthorFilesLines
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>
2020-04-01dir: refactor treat_directory to clarify control flowLibravatar Derrick Stolee1-18/+17
The logic in treat_directory() is handled by a multi-case switch statement, but this switch is very asymmetrical, as the first two cases are simple but the third is more complicated than the rest of the method. In fact, the third case includes a "break" statement that leads to the block of code outside the switch statement. That is the only way to reach that block, as the switch handles all possible values from directory_exists_in_index(); Extract the switch statement into a series of "if" statements. This simplifies the trivial cases, while clarifying how to reach the "show_other_directories" case. This is particularly important as the "show_other_directories" case will expand in a later change. Helped-by: Elijah Newren <newren@gmail.com> 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>
2020-04-01dir: fix confusion based on variable tenseLibravatar Elijah Newren1-13/+13
Despite having contributed several fixes in this area, I have for months (years?) assumed that the "exclude" variable was a directive; this caused me to think of it as a different mode we operate in and left me confused as I tried to build up a mental model around why we'd need such a directive. I mostly tried to ignore it while focusing on the pieces I was trying to understand. Then I finally traced this variable all back to a call to is_excluded(), meaning it was actually functioning as an adjective. In particular, it was a checked property ("Does this path match a rule in .gitignore?"), rather than a mode passed in from the caller. Change the variable name to match the part of speech used by the function called to define it, which will hopefully make these bits of code slightly clearer to the next reader. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-04-01dir: fix broken commentLibravatar Elijah Newren1-1/+1
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-04-01dir: consolidate treat_path() and treat_one_path()Libravatar Elijah Newren1-66/+55
Commit 16e2cfa90993 ("read_directory(): further split treat_path()", 2010-01-08) split treat_one_path() out of treat_path(), because treat_leading_path() would not have access to a dirent but wanted to re-use as much of treat_path() as possible. Not re-using all of treat_path() caused other bugs, as noted in commit b9670c1f5e6b ("dir: fix checks on common prefix directory", 2019-12-19). Finally, in commit ad6f2157f951 ("dir: restructure in a way to avoid passing around a struct dirent", 2020-01-16), dirents were removed from treat_path() and other functions entirely. Since the only reason for splitting these functions was the lack of a dirent -- which no longer applies to either function -- and since the split caused problems in the past resulting in us not using treat_one_path() separately anymore, just undo the split. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-04-01dir: fix simple typo in commentLibravatar Elijah Newren1-1/+1
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-03-05Merge branch 'ds/sparse-add'Libravatar Junio C Hamano1-1/+1
"git sparse-checkout" learned a new "add" subcommand. * ds/sparse-add: sparse-checkout: allow one-character directories in cone mode sparse-checkout: work with Windows paths sparse-checkout: create 'add' subcommand sparse-checkout: extract pattern update from 'set' subcommand sparse-checkout: extract add_patterns_from_input()
2020-02-20sparse-checkout: allow one-character directories in cone modeLibravatar Derrick Stolee1-1/+1
In 9e6d3e64 (sparse-checkout: detect short patterns, 2020-01-24), a condition on the minimum length of a cone-mode pattern was introduced. However, this condition was off-by-one. If we have a directory with a single character, say "b", then the command git sparse-checkout set b will correctly add the pattern "/b/" to the sparse-checkout file. When this is interpeted in dir.c, the pattern is "/b" with the PATTERN_FLAG_MUSTBEDIR flag. This string has length two, which satisfies our inclusive inequality (<= 2). The reason for this inequality is that we will start to read the pattern string character-by-character using three char pointers: prev, cur, next. In particular, next is set to the current pattern plus two. The mistake was that next will still be a valid pointer when the pattern length is two, since the string is null-terminated. Make this inequality strict so these patterns work. Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-14Merge branch 'mt/use-passed-repo-more-in-funcs'Libravatar Junio C Hamano1-2/+2
Some codepaths were given a repository instance as a parameter to work in the repository, but passed the_repository instance to its callees, which has been cleaned up (somewhat). * mt/use-passed-repo-more-in-funcs: sha1-file: allow check_object_signature() to handle any repo sha1-file: pass git_hash_algo to hash_object_file() sha1-file: pass git_hash_algo to write_object_file_prepare() streaming: allow open_istream() to handle any repo pack-check: use given repo's hash_algo at verify_packfile() cache-tree: use given repo's hash_algo at verify_one() diff: make diff_populate_filespec() honor its repo argument
2020-02-14Merge branch 'ds/sparse-checkout-harden'Libravatar Junio C Hamano1-4/+75
Some rough edges in the sparse-checkout feature, especially around the cone mode, have been cleaned up. * ds/sparse-checkout-harden: sparse-checkout: fix cone mode behavior mismatch sparse-checkout: improve docs around 'set' in cone mode sparse-checkout: escape all glob characters on write sparse-checkout: use C-style quotes in 'list' subcommand sparse-checkout: unquote C-style strings over --stdin sparse-checkout: write escaped patterns in cone mode sparse-checkout: properly match escaped characters sparse-checkout: warn on globs in cone patterns sparse-checkout: detect short patterns sparse-checkout: cone mode does not recognize "**" sparse-checkout: fix documentation typo for core.sparseCheckoutCone clone: fix --sparse option with URLs sparse-checkout: create leading directories t1091: improve here-docs t1091: use check_files to reduce boilerplate
2020-01-31sparse-checkout: properly match escaped charactersLibravatar Derrick Stolee1-3/+32
In cone mode, the sparse-checkout feature uses hashset containment queries to match paths. Make this algorithm respect escaped asterisk (*) and backslash (\) characters. Create dup_and_filter_pattern() method to convert a pattern by removing escape characters and dropping an optional "/*" at the end. This method is available in dir.h as we will use it in builtin/sparse-checkout.c in a later change. Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-01-31sparse-checkout: warn on globs in cone patternsLibravatar Derrick Stolee1-0/+36
In cone mode, the sparse-checkout commmand will write patterns that allow faster pattern matching. This matching only works if the patterns in the sparse-checkout file are those written by that command. Users can edit the sparse-checkout file and create patterns that cause the cone mode matching to fail. The cone mode patterns may end in "/*" but otherwise an un-escaped asterisk or other glob character is invalid. Add checks to disable cone mode when seeing these values. A later change will properly handle escaped globs. Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-01-31sha1-file: pass git_hash_algo to hash_object_file()Libravatar Matheus Tavares1-2/+2
Allow hash_object_file() to work on arbitrary repos by introducing a git_hash_algo parameter. Change callers which have a struct repository pointer in their scope to pass on the git_hash_algo from the said repo. For all other callers, pass on the_hash_algo, which was already being used internally at hash_object_file(). This functionality will be used in the following patch to make check_object_signature() be able to work on arbitrary repos (which, in turn, will be used to fix an inconsistency at object.c:parse_object()). Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-01-24sparse-checkout: detect short patternsLibravatar Derrick Stolee1-1/+2
In cone mode, the shortest pattern the sparse-checkout command will write into the sparse-checkout file is "/*". This is handled carefully in add_pattern_to_hashsets(), so warn if any other pattern is this short. This will assist future pattern checks by allowing us to assume there are at least three characters in the pattern. Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-01-24sparse-checkout: cone mode does not recognize "**"Libravatar Derrick Stolee1-1/+6
When core.sparseCheckoutCone is enabled, the 'git sparse-checkout set' command creates a restricted set of possible patterns that are used by a custom algorithm to quickly match those patterns. If a user manually edits the sparse-checkout file, then they could create patterns that do not match these expectations. The cone-mode matching algorithm can return incorrect results. The solution is to detect these incorrect patterns, warn that we do not recognize them, and revert to the standard algorithm. Check each pattern for the "**" substring, and revert to the old logic if seen. While technically a "/<dir>/**" pattern matches the meaning of "/<dir>/", it is not one that would be written by the sparse-checkout builtin in cone mode. Attempting to accept that pattern change complicates the logic and instead we punt and do not accept any instance of "**". Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-01-16dir: point treat_leading_path() warning to the right placeLibravatar Jeff King1-3/+3
Commit 777b420347 (dir: synchronize treat_leading_path() and read_directory_recursive(), 2019-12-19) tried to add two warning comments in those functions, pointing at each other. But the one in treat_leading_path() just points at itself. Let's fix that. Since the comment also redirects the reader for more details to "the commit that added this warning", and since we're now modifying the warning (creating a new commit without those details), let's mention the actual commit id. Signed-off-by: Jeff King <peff@peff.net> Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-01-16dir: restructure in a way to avoid passing around a struct direntLibravatar Jeff King1-42/+31
Restructure the code slightly to avoid passing around a struct dirent anywhere, which also enables us to avoid trying to manufacture one. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-01-16dir: treat_leading_path() and read_directory_recursive(), round 2Libravatar Elijah Newren1-0/+4
I was going to title this "dir: more synchronizing of treat_leading_path() and read_directory_recursive()", a nod to commit 777b42034764 ("dir: synchronize treat_leading_path() and read_directory_recursive()", 2019-12-19), but the title was too long. Anyway, first the backstory... fill_directory() has always had a slightly error-prone interface: it returns a subset of paths which *might* match the specified pathspec; it was intended to prune away some paths which didn't match the specified pathspec and keep at least all the ones that did match it. Given this interface, callers were responsible to post-process the results and check whether each actually matched the pathspec. builtin/clean.c did this. It would first prune out duplicates (e.g. if "dir" was returned as well as all files under "dir/", then it would simplify this to just "dir"), and after pruning duplicates it would compare the remaining paths to the specified pathspec(s). This post-processing itself could run into problems, though, as noted in commit 404ebceda01c ("dir: also check directories for matching pathspecs", 2019-09-17): For the case of git-clean and a set of pathspecs of "dir/file" and "more", this caused a problem because we'd end up with dir entries for both of "dir" "dir/file" Then correct_untracked_entries() would try to helpfully prune duplicates for us by removing "dir/file" since it's under "dir", leaving us with "dir" Since the original pathspec only had "dir/file", the only entry left doesn't match and leaves nothing to be removed. (Note that if only one pathspec was specified, e.g. only "dir/file", then the common_prefix_len optimizations in fill_directory would cause us to bypass this problem, making it appear in simple tests that we could correctly remove manually specified pathspecs.) That commit fixed the issue -- when multiple pathspecs were specified -- by making sure fill_directory() wouldn't return both "dir" and "dir/file" outside the common_prefix_len optimization path. This is where it starts to get fun. In commit b9670c1f5e6b ("dir: fix checks on common prefix directory", 2019-12-19), we noticed that the common_prefix_len wasn't doing appropriate checks and letting all kinds of stuff through, resulting in recursing into .git/ directories and other craziness. So it started locking down and doing checks on pathnames within that code path. That continued with commit 777b42034764 ("dir: synchronize treat_leading_path() and read_directory_recursive()", 2019-12-19), which noted the following: Our optimization to avoid calling into read_directory_recursive() when all pathspecs have a common leading directory mean that we need to match the logic that read_directory_recursive() would use if we had just called it from the root. Since it does more than call treat_path() we need to copy that same logic. ...and then it more forcefully addressed the issue with this wonderfully ironic statement: Needing to duplicate logic like this means it is guaranteed someone will eventually need to make further changes and forget to update both locations. It is tempting to just nuke the leading_directory special casing to avoid such bugs and simplify the code, but unpack_trees' verify_clean_subdirectory() also calls read_directory() and does so with a non-empty leading path, so I'm hesitant to try to restructure further. Add obnoxious warnings to treat_leading_path() and read_directory_recursive() to try to warn people of such problems. You would think that with such a strongly worded description, that its author would have actually ensured that the logic in treat_leading_path() and read_directory_recursive() did actually match and that *everything* that was needed had at least been copied over at the time that this paragraph was written. But you'd be wrong, I messed it up by missing part of the logic. Copy the missing bits to fix the new final test in t7300. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-12-25Merge branch 'en/fill-directory-fixes'Libravatar Junio C Hamano1-49/+138
Assorted fixes to the directory traversal API. * en/fill-directory-fixes: dir.c: use st_add3() for allocation size dir: consolidate similar code in treat_directory() dir: synchronize treat_leading_path() and read_directory_recursive() dir: fix checks on common prefix directory dir: break part of read_directory_recursive() out for reuse dir: exit before wildcard fall-through if there is no wildcard dir: remove stray quote character in comment Revert "dir.c: make 'git-status --ignored' work within leading directories" t3011: demonstrate directory traversal failures
2019-12-25Merge branch 'ds/sparse-cone'Libravatar Junio C Hamano1-8/+208
Management of sparsely checked-out working tree has gained a dedicated "sparse-checkout" command. * ds/sparse-cone: (21 commits) sparse-checkout: improve OS ls compatibility sparse-checkout: respect core.ignoreCase in cone mode sparse-checkout: check for dirty status sparse-checkout: update working directory in-process for 'init' sparse-checkout: cone mode should not interact with .gitignore sparse-checkout: write using lockfile sparse-checkout: use in-process update for disable subcommand sparse-checkout: update working directory in-process sparse-checkout: sanitize for nested folders unpack-trees: add progress to clear_ce_flags() unpack-trees: hash less in cone mode sparse-checkout: init and set in cone mode sparse-checkout: use hashmaps for cone patterns sparse-checkout: add 'cone' mode trace2: add region in clear_ce_flags sparse-checkout: create 'disable' subcommand sparse-checkout: add '--stdin' option to set subcommand sparse-checkout: 'set' subcommand clone: add --sparse mode sparse-checkout: create 'init' subcommand ...
2019-12-20dir.c: use st_add3() for allocation sizeLibravatar Junio C Hamano1-1/+1
When preparing a manufactured dirent instance, we add a length of path to the size of struct to decide how many bytes to allocate. Make sure this addition does not wrap-around to cause us underallocate. Suggested-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-12-19dir: consolidate similar code in treat_directory()Libravatar Elijah Newren1-11/+7
Both the DIR_SKIP_NESTED_GIT and DIR_NO_GITLINKS cases were checking for whether a path was actually a nonbare repository. That code could be shared, with just the result of how to act differing between the two cases. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-12-19dir: synchronize treat_leading_path() and read_directory_recursive()Libravatar Elijah Newren1-0/+30
Our optimization to avoid calling into read_directory_recursive() when all pathspecs have a common leading directory mean that we need to match the logic that read_directory_recursive() would use if we had just called it from the root. Since it does more than call treat_path() we need to copy that same logic. Alternatively, we could try to change treat_path to return path_recurse for an untracked directory under the given special circumstances that this logic checks for, but a simple switch results in many test failures such as 'git clean -d' not wiping out untracked but empty directories. To work around that, we'd need the caller of treat_path to check for path_recurse and sometimes special case it into path_untracked. In other words, we'd still have extra logic in both places. Needing to duplicate logic like this means it is guaranteed someone will eventually need to make further changes and forget to update both locations. It is tempting to just nuke the leading_directory special casing to avoid such bugs and simplify the code, but unpack_trees' verify_clean_subdirectory() also calls read_directory() and does so with a non-empty leading path, so I'm hesitant to try to restructure further. Add obnoxious warnings to treat_leading_path() and read_directory_recursive() to try to warn people of such problems. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-12-19dir: fix checks on common prefix directoryLibravatar Elijah Newren1-11/+56
Many years ago, the directory traversing logic had an optimization that would always recurse into any directory that was a common prefix of all the pathspecs without walking the leading directories to get down to the desired directory. Thus, git ls-files -o .git/ # case A would notice that .git/ was a common prefix of all pathspecs (since it is the only pathspec listed), and then traverse into it and start showing unknown files under that directory. Unfortunately, .git/ is not a directory we should be traversing into, which made this optimization problematic. This also affected cases like git ls-files -o --exclude-standard t/ # case B where t/ was in the .gitignore file and thus isn't interesting and shouldn't be recursed into. It also affected cases like git ls-files -o --directory untracked_dir/ # case C where untracked_dir/ is indeed untracked and thus interesting, but the --directory flag means we only want to show the directory itself, not recurse into it and start listing untracked files below it. The case B class of bugs were noted and fixed in commits 16e2cfa90993 ("read_directory(): further split treat_path()", 2010-01-08) and 48ffef966c76 ("ls-files: fix overeager pathspec optimization", 2010-01-08), with the idea being that we first wanted to check whether the common prefix was interesting. The former patch noted that treat_path() couldn't be used when checking the common prefix because treat_path() requires a dir_entry() and we haven't read any directories at the point we are checking the common prefix. So, that patch split treat_one_path() out of treat_path(). The latter patch then created a new treat_leading_path() which duplicated by hand the bits of treat_path() that couldn't be broken out and then called treat_one_path() for the remainder. There were three problems with this approach: * The duplicated logic in treat_leading_path() accidentally missed the check for special paths (such as is_dot_or_dotdot and matching ".git"), causing case A types of bugs to continue to be an issue. * The treat_leading_path() logic assumed we should traverse into anything where path_treatment was not path_none, i.e. it perpetuated class C types of bugs. * It meant we had split logic that needed to kept in sync, running the risk that people introduced new inconsistencies (such as in commit be8a84c52669, which we reverted earlier in this series, or in commit df5bcdf83ae which we'll fix in a subsequent commit) Fix most these problems by making treat_leading_path() not only loop over each leading path component, but calling treat_path() directly on each. To do so, we have to create a synthetic dir_entry, but that only takes a few lines. Then, pay attention to the path_treatment result we get from treat_path() and don't treat path_excluded, path_untracked, and path_recurse all the same as path_recurse. This leaves one remaining problem, the new inconsistency from commit df5bcdf83ae. That will be addressed in a subsequent commit. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-12-16Merge branch 'hw/doc-in-header'Libravatar Junio C Hamano1-2/+0
* hw/doc-in-header: trace2: move doc to trace2.h submodule-config: move doc to submodule-config.h tree-walk: move doc to tree-walk.h trace: move doc to trace.h run-command: move doc to run-command.h parse-options: add link to doc file in parse-options.h credential: move doc to credential.h argv-array: move doc to argv-array.h cache: move doc to cache.h sigchain: move doc to sigchain.h pathspec: move doc to pathspec.h revision: move doc to revision.h attr: move doc to attr.h refs: move doc to refs.h remote: move doc to remote.h and refspec.h sha1-array: move doc to sha1-array.h merge: move doc to ll-merge.h graph: move doc to graph.h and graph.c dir: move doc to dir.h diff: move doc to diff.h and diffcore.h
2019-12-13sparse-checkout: respect core.ignoreCase in cone modeLibravatar Derrick Stolee1-3/+12
When a user uses the sparse-checkout feature in cone mode, they add patterns using "git sparse-checkout set <dir1> <dir2> ..." or by using "--stdin" to provide the directories line-by-line over stdin. This behaviour naturally looks a lot like the way a user would type "git add <dir1> <dir2> ..." If core.ignoreCase is enabled, then "git add" will match the input using a case-insensitive match. Do the same for the sparse-checkout feature. Perform case-insensitive checks while updating the skip-worktree bits during unpack_trees(). This is done by changing the hash algorithm and hashmap comparison methods to optionally use case- insensitive methods. When this is enabled, there is a small performance cost in the hashing algorithm. To tease out the worst possible case, the following was run on a repo with a deep directory structure: git ls-tree -d -r --name-only HEAD | git sparse-checkout set --stdin The 'set' command was timed with core.ignoreCase disabled or enabled. For the repo with a deep history, the numbers were core.ignoreCase=false: 62s core.ignoreCase=true: 74s (+19.3%) For reproducibility, the equivalent test on the Linux kernel repository had these numbers: core.ignoreCase=false: 3.1s core.ignoreCase=true: 3.6s (+16%) Now, this is not an entirely fair comparison, as most users will define their sparse cone using more shallow directories, and the performance improvement from eb42feca97 ("unpack-trees: hash less in cone mode" 2019-11-21) can remove most of the hash cost. For a more realistic test, drop the "-r" from the ls-tree command to store only the first-level directories. In that case, the Linux kernel repository takes 0.2-0.25s in each case, and the deep repository takes one second, plus or minus 0.05s, in each case. Thus, we _can_ demonstrate a cost to this change, but it is unlikely to matter to any reasonable sparse-checkout cone. Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-12-11dir: break part of read_directory_recursive() out for reuseLibravatar Elijah Newren1-23/+37
Create an add_path_to_appropriate_result_list() function from the code at the end of read_directory_recursive() so we can use it elsewhere. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-12-11dir: exit before wildcard fall-through if there is no wildcardLibravatar Elijah Newren1-0/+7
The DO_MATCH_LEADING_PATHSPEC had a fall-through case for if there was a wildcard, noting that we don't yet have enough information to determine if a further paths under the current directory might match due to the presence of wildcards. But if we have no wildcards in our pathspec, then we shouldn't get to that fall-through case. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-12-11dir: remove stray quote character in commentLibravatar Elijah Newren1-1/+1
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-12-11Revert "dir.c: make 'git-status --ignored' work within leading directories"Libravatar Elijah Newren1-3/+0
Commit be8a84c52669 ("dir.c: make 'git-status --ignored' work within leading directories", 2013-04-15) noted that git status --ignored <SOMEPATH> would not list ignored files and directories within <SOMEPATH> if <SOMEPATH> was untracked, and modified the behavior to make it show them. However, it did so via a hack that broke consistency; it would show paths under <SOMEPATH> differently than a simple git status --ignored | grep <SOMEPATH> would show them. A correct fix is slightly more involved, and complicated slightly by this hack, so we revert this commit (but keep corrected versions of the testcases) and will later fix the original bug with a subsequent patch. Some history may be helpful: A very, very similar case to the commit we are reverting was raised in commit 48ffef966c76 ("ls-files: fix overeager pathspec optimization", 2010-01-08); but it actually went in somewhat the opposite direction. In that commit, it mentioned how git ls-files -o --exclude-standard t/ used to show untracked files under t/ even when t/ was ignored, and then changed the behavior to stop showing untracked files under an ignored directory. More importantly, this commit considered keeping this behavior but noted that it would be inconsistent with the behavior when multiple pathspecs were specified and thus rejected it. The reason for this whole inconsistency when one pathspec is specified versus zero or two is because common prefixes of pathspecs are sent through a different set of checks (in treat_leading_path()) than normal file/directory traversal (those go through read_directory_recursive() and treat_path()). As such, for consistency, one needs to check that both codepaths produce the same result. Revert commit be8a84c526691667fc04a8241d93a3de1de298ab, except instead of removing the testcase it added, modify it to check for correct and consistent behavior. A subsequent patch in this series will fix the testcase. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-11-22unpack-trees: hash less in cone modeLibravatar Derrick Stolee1-2/+2
The sparse-checkout feature in "cone mode" can use the fact that the recursive patterns are "connected" to the root via parent patterns to decide if a directory is entirely contained in the sparse-checkout or entirely removed. In these cases, we can skip hashing the paths within those directories and simply set the skipworktree bit to the correct value. Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-11-22sparse-checkout: init and set in cone modeLibravatar Derrick Stolee1-4/+4
To make the cone pattern set easy to use, update the behavior of 'git sparse-checkout (init|set)'. Add '--cone' flag to 'git sparse-checkout init' to set the config option 'core.sparseCheckoutCone=true'. When running 'git sparse-checkout set' in cone mode, a user only needs to supply a list of recursive folder matches. Git will automatically add the necessary parent matches for the leading directories. When testing 'git sparse-checkout set' in cone mode, check the error stream to ensure we do not see any errors. Specifically, we want to avoid the warning that the patterns do not match the cone-mode patterns. Helped-by: Eric Wong <e@80x24.org> Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-11-22sparse-checkout: use hashmaps for cone patternsLibravatar Derrick Stolee1-8/+199
The parent and recursive patterns allowed by the "cone mode" option in sparse-checkout are restrictive enough that we can avoid using the regex parsing. Everything is based on prefix matches, so we can use hashsets to store the prefixes from the sparse-checkout file. When checking a path, we can strip path entries from the path and check the hashset for an exact match. As a test, I created a cone-mode sparse-checkout file for the Linux repository that actually includes every file. This was constructed by taking every folder in the Linux repo and creating the pattern pairs here: /$folder/ !/$folder/*/ This resulted in a sparse-checkout file sith 8,296 patterns. Running 'git read-tree -mu HEAD' on this file had the following performance: core.sparseCheckout=false: 0.21 s (0.00 s) core.sparseCheckout=true: 3.75 s (3.50 s) core.sparseCheckoutCone=true: 0.23 s (0.01 s) The times in parentheses above correspond to the time spent in the first clear_ce_flags() call, according to the trace2 performance traces. While this example is contrived, it demonstrates how these patterns can slow the sparse-checkout feature. Helped-by: Eric Wong <e@80x24.org> Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-11-18dir: move doc to dir.hLibravatar Heba Waly1-2/+0
Move the documentation from Documentation/technical/api-directory-listing.txt to dir.h as it's easier for the developers to find the usage information beside the code instead of looking for it in another doc file. Also documentation/technical/api-directory-listing.txt is removed because the information it has is now redundant and it'll be hard to keep it up to date and synchronized with the documentation in the header files. Signed-off-by: Heba Waly <heba.waly@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-11-10Fix spelling errors in code commentsLibravatar Elijah Newren1-1/+1
Reported-by: Jens Schleusener <Jens.Schleusener@fossies.org> Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-10-11Merge branch 'en/clean-nested-with-ignored'Libravatar Junio C Hamano1-17/+48
"git clean" fixes. * en/clean-nested-with-ignored: dir: special case check for the possibility that pathspec is NULL clean: fix theoretical path corruption clean: rewrap overly long line clean: avoid removing untracked files in a nested git repository clean: disambiguate the definition of -d git-clean.txt: do not claim we will delete files with -n/--dry-run dir: add commentary explaining match_pathspec_item's return value dir: if our pathspec might match files under a dir, recurse into it dir: make the DO_MATCH_SUBMODULE code reusable for a non-submodule case dir: also check directories for matching pathspecs dir: fix off-by-one error in match_pathspec_item dir: fix typo in comment t7300: add testcases showing failure to clean specified pathspecs
2019-10-02dir: special case check for the possibility that pathspec is NULLLibravatar Elijah Newren1-3/+5
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) added calls to match_pathspec() and do_match_pathspec() passing along their pathspec parameter. Both match_pathspec() and do_match_pathspec() assume the pathspec argument they are given is non-NULL. It turns out that unpack-tree.c's verify_clean_subdirectory() calls read_directory() with pathspec == NULL, and it is possible on case insensitive filesystems for that NULL to make it to these new calls to match_pathspec() and do_match_pathspec(). Add appropriate checks on the NULLness of pathspec to avoid a segfault. In case the negation throws anyone off (one of the calls was to do_match_pathspec() while the other was to !match_pathspec(), yet no negation of the NULLness of pathspec is used), there are two ways to understand the differences: * The code already handled the pathspec == NULL cases before this series, and this series only tried to change behavior when there was a pathspec, thus we only want to go into the if-block if pathspec is non-NULL. * One of the calls is for whether to recurse into a subdirectory, the other is for after we've recursed into it for whether we want to remove the subdirectory itself (i.e. the subdirectory didn't match but something under it could have). That difference in situation leads to the slight differences in logic used (well, that and the slightly unusual fact that we don't want empty pathspecs to remove untracked directories by default). Denton found and analyzed one issue and provided the patch for the match_pathspec() call, SZEDER figured out why the issue only reproduced for some folks and not others and provided the testcase, and I looked through the remainder of the series and noted the do_match_pathspec() call that should have the same check. Co-authored-by: Denton Liu <liu.denton@gmail.com> Co-authored-by: SZEDER Gábor <szeder.dev@gmail.com> Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-09-30Merge branch 'ds/include-exclude'Libravatar Junio C Hamano1-136/+148
The internal code originally invented for ".gitignore" processing got reshuffled and renamed to make it less tied to "excluding" and stress more that it is about "matching", as it has been reused for things like sparse checkout specification that want to check if a path is "included". * ds/include-exclude: unpack-trees: rename 'is_excluded_from_list()' treewide: rename 'exclude' methods to 'pattern' treewide: rename 'EXCL_FLAG_' to 'PATTERN_FLAG_' treewide: rename 'struct exclude_list' to 'struct pattern_list' treewide: rename 'struct exclude' to 'struct path_pattern'
2019-09-17clean: avoid removing untracked files in a nested git repositoryLibravatar Elijah Newren1-0/+10
Users expect files in a nested git repository to be left alone unless sufficiently forced (with two -f's). Unfortunately, in certain circumstances, git would delete both tracked (and possibly dirty) files and untracked files within a nested repository. To explain how this happens, let's contrast a couple cases. First, take the following example setup (which assumes we are already within a git repo): git init nested cd nested >tracked git add tracked git commit -m init >untracked cd .. In this setup, everything works as expected; running 'git clean -fd' will result in fill_directory() returning the following paths: nested/ nested/tracked nested/untracked and then correct_untracked_entries() would notice this can be compressed to nested/ and then since "nested/" is a directory, we would call remove_dirs("nested/", ...), which would check is_nonbare_repository_dir() and then decide to skip it. However, if someone also creates an ignored file: >nested/ignored then running 'git clean -fd' would result in fill_directory() returning the same paths: nested/ nested/tracked nested/untracked but correct_untracked_entries() will notice that we had ignored entries under nested/ and thus simplify this list to nested/tracked nested/untracked Since these are not directories, we do not call remove_dirs() which was the only place that had the is_nonbare_repository_dir() safety check -- resulting in us deleting both the untracked file and the tracked (and possibly dirty) file. One possible fix for this issue would be walking the parent directories of each path and checking if they represent nonbare repositories, but that would be wasteful. Even if we added caching of some sort, it's still a waste because we should have been able to check that "nested/" represented a nonbare repository before even descending into it in the first place. Add a DIR_SKIP_NESTED_GIT flag to dir_struct.flags and use it to prevent fill_directory() and friends from descending into nested git repos. With this change, we also modify two regression tests added in commit 91479b9c72f1 ("t7300: add tests to document behavior of clean and nested git", 2015-06-15). That commit, nor its series, nor the six previous iterations of that series on the mailing list discussed why those tests coded the expectation they did. In fact, it appears their purpose was simply to test _existing_ behavior to make sure that the performance changes didn't change the behavior. However, these two tests directly contradicted the manpage's claims that two -f's were required to delete files/directories under a nested git repository. While one could argue that the user gave an explicit path which matched files/directories that were within a nested repository, there's a slippery slope that becomes very difficult for users to understand once you go down that route (e.g. what if they specified "git clean -f -d '*.c'"?) It would also be hard to explain what the exact behavior was; avoid such problems by making it really simple. Also, clean up some grammar errors describing this functionality in the git-clean manpage. Finally, there are still a couple bugs with -ffd not cleaning out enough (e.g. missing the nested .git) and with -ffdX possibly cleaning out the wrong files (paying attention to outer .gitignore instead of inner). This patch does not address these cases at all (and does not change the behavior relative to those flags), it only fixes the handling when given a single -f. See https://public-inbox.org/git/20190905212043.GC32087@szeder.dev/ for more discussion of the -ffd[X?] bugs. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-09-17dir: add commentary explaining match_pathspec_item's return valueLibravatar Elijah Newren1-8/+19
The way match_pathspec_item() handles names and pathspecs with trailing slash characters, in conjunction with special options like DO_MATCH_DIRECTORY and DO_MATCH_LEADING_PATHSPEC were non-obvious, and broken until this patch series. Add a table in a comment explaining the intent of how these work. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-09-17dir: if our pathspec might match files under a dir, recurse into itLibravatar Elijah Newren1-4/+6
For git clean, if a directory is entirely untracked and the user did not specify -d (corresponding to DIR_SHOW_IGNORED_TOO), then we usually do not want to remove that directory and thus do not recurse into it. However, if the user manually specified specific (or even globbed) paths somewhere under that directory to remove, then we need to recurse into the directory to make sure we remove the relevant paths under that directory as the user requested. Note that this does not mean that the recursed-into directory will be added to dir->entries for later removal; as of a few commits earlier in this series, there is another more strict match check that is run after returning from a recursed-into directory before deciding to add it to the list of entries. Therefore, this will only result in files underneath the given directory which match one of the pathspecs being added to the entries list. Two notes of potential interest to future readers: * If we wanted to only recurse into a directory when it is specifically matched rather than matched-via-glob (e.g. '*.c'), then we could do so via making the final non-zero return in match_pathspec_item be MATCHED_RECURSIVELY instead of MATCHED_RECURSIVELY_LEADING_PATHSPEC. (Note that the relative order of MATCHED_RECURSIVELY_LEADING_PATHSPEC and MATCHED_RECURSIVELY are important for such a change.) I was leaving open that possibility while writing an RFC asking for the behavior we want, but even though we don't want it, that knowledge might help you understand the code flow better. * There is a growing amount of logic in read_directory_recursive() for deciding whether to recurse into a subdirectory. However, there is a comment immediately preceding this logic that says to recurse if instructed by treat_path(). It may be better for the logic in read_directory_recursive() to ultimately be moved to treat_path() (or another function it calls, such as treat_directory()), but I have left that for someone else to tackle in the future. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>