kernel/git - git - PowerEL Git System

Commit Graph

Author	SHA1	Message	Date
Derrick Stolee	5ceb663e92	dir: fix directory-matching bug This reverts the change from `ed49584` (dir: fix pattern matching on dirs, 2021-09-24), which claimed to fix a directory-matching problem without a test case. It turns out to _create_ a bug, but it is a bit subtle. The bug would have been revealed by the first of two tests being added to t0008-ignores.sh. The first uses a pattern "/git/" inside the a/.gitignores file, which matches against 'a/git/foo' but not 'a/git-foo/bar'. This test would fail before the revert. The second test shows what happens if the test instead uses a pattern "git/" and this test passes both before and after the revert. The difference in these two cases are due to how last_matching_pattern_from_list() checks patterns both if they have the PATTERN_FLAG_MUSTBEDIR and PATTERN_FLAG_NODIR flags. In the case of "git/", the PATTERN_FLAG_NODIR is also provided, making the change in behavior in match_pathname() not affect the end result of last_matching_pattern_from_list(). Reported-by: Glen Choo <chooglen@google.com> Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	3 years ago
Matheus Tavares	20141e322c	add, rm, mv: fix bug that prevents the update of non-sparse dirs These three commands recently learned to avoid updating paths outside the sparse checkout even if they are missing the SKIP_WORKTREE bit. This is done using path_in_sparse_checkout(), which checks whether a given path matches the current list of sparsity rules, similar to what clear_ce_flags() does when we run "git sparse checkout init" or "git sparse-checkout reapply". However, clear_ce_flags() uses a recursive approach, applying the match results from parent directories on paths that get the UNDECIDED result, whereas path_in_sparse_checkout() only attempts to match the full path and immediately considers UNDECIDED as NOT_MATCHED. This makes the function miss matches with leading directories. For example, if the user has the sparsity patterns "!/a" and "b/", add, rm, and mv will fail to update the path "a/b/c" and end up displaying a warning about it being outside the sparse checkout even though it isn't. This problem only occurs in full pattern mode as the pattern matching functions never return UNDECIDED for cone mode. To fix this, replicate the recursive behavior of clear_ce_flags() in path_in_sparse_checkout(), falling back to the parent directory match when a path gets the UNDECIDED result. (If this turns out to be too expensive in some cases, we may want to later add some form of caching to accelerate multiple queries within the same directory. This is not implemented in this patch, though.) Also add two tests for each affected command (add, rm, and mv) to check that they behave correctly with the recursive pattern matching. The first test would previously fail without this patch while the second already succeeded. It is added mostly to make sure that we are not breaking the existing pattern matching for directories that are really sparse, and also as a protection against any future regressions. Two other existing tests had to be changed as well: one test in t3602 checks that "git rm -r <dir>" won't remove sparse entries, but it didn't allow the non-sparse entries inside <dir> to be removed. The other one, in t7002, tested that "git mv" would correctly display a warning message for sparse paths, but it accidentally expected the message to include two non-sparse paths as well. Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br> Acked-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	3 years ago
Derrick Stolee	ed4958477b	dir: fix pattern matching on dirs Within match_pathname(), one successful matching category happens when the pattern is equal to its non-wildcard prefix. At this point, we have checked that the input 'pathname' matches the pattern up to the prefix length, and then we subtraced that length from both 'patternlen' and 'namelen'. In the case of a directory match, this prefix match should be sufficient. However, the success condition only cared about _exact_ equality here. Instead, we should allow any path that agrees on this prefix in the case of PATTERN_FLAG_MUSTBEDIR. This case was not tested before because of the way unpack_trees() would match a parent directory before visiting the contained paths. This approach is changing, so we must change this comparison. Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	3 years ago
Derrick Stolee	f6526728f9	dir: select directories correctly When matching a path against a list of patterns, the ones that require a directory match previously did not work when a filename is specified. This was fine when all pattern-matching was done within methods such as unpack_trees() that check a directory before recursing into the contained files. However, other commands will start matching individual files against pattern lists without that recursive approach. The last_matching_pattern_from_list() logic performs some checks on the filetype of a path within the index when the PATTERN_FLAG_MUSTBEDIR flag is set. This works great when setting SKIP_WORKTREE bits within unpack_trees(), but doesn't work well when passing an arbitrary path such as a file within a matching directory. We extract the logic around determining the file type, but attempt to avoid checking the filesystem if the parent directory already matches the sparse-checkout patterns. The new path_matches_dir_pattern() method includes a 'path_parent' parameter that is used to store the parent directory of 'pathname' between multiple pattern matching tests. This is loaded lazily, only on the first pattern it finds that has the PATTERN_FLAG_MUSTBEDIR flag. If we find that a path has a parent directory, we start by checking to see if that parent directory matches the pattern. If so, then we do not need to query the index for the type (which can be expensive). If we find that the parent does not match, then we still must check the type from the index for the given pathname. Note that this does not affect cone mode pattern matching, but instead the more general -- and slower -- full pattern set. Thus, this does not affect the sparse index. Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	3 years ago
Jonathan Tan	ce125d431a	submodule: extract path to submodule gitdir func We currently store each submodule gitdir in ".git/modules/<name>", but this has problems with some submodule naming schemes, as described in a comment in submodule_name_to_gitdir() in this patch. Extract the determination of the location of a submodule's gitdir into its own function submodule_name_to_gitdir(). For now, the problem remains unsolved, but this puts us in a better position for finding a solution. This was motivated, at $DAYJOB, by a part of Android's repo hierarchy [1]. In particular, there is a repo "build", and several repos of the form "build/<name>". This is based on earlier work by Brandon Williams [2]. [1] https://android.googlesource.com/platform/ [2] https://lore.kernel.org/git/20180808223323.79989-2-bmwill@google.com/ Signed-off-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	3 years ago
Derrick Stolee	02155c8c00	sparse-checkout: create helper methods As we integrate the sparse index into more builtins, we occasionally need to check the sparse-checkout patterns to see if a path is within the sparse-checkout cone. Create some helper methods that help initialize the patterns and check for pattern matching to make this easier. The existing callers of commands like get_sparse_checkout_patterns() use a custom 'struct pattern_list' that is not necessarily the one in the 'struct index_state', so there are not many previous uses that could adopt these helpers. There are just two in builtin/add.c and sparse-index.c that can use path_in_sparse_checkout(). We add a path_in_cone_mode_sparse_checkout() as well that will only return false if the path is outside of the sparse-checkout definition _and_ the sparse-checkout patterns are in cone mode. Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	3 years ago
Atharva Raykar	ed86301f68	dir: libify and export helper functions from clone.c These functions can be useful to other parts of Git. Let's move them to dir.c, while renaming them to be make their functionality more explicit. Signed-off-by: Atharva Raykar <raykar.ath@gmail.com> Mentored-by: Christian Couder <christian.couder@gmail.com> Mentored-by: Shourya Shukla <periperidip@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	4 years ago
René Scharfe	7431842325	use fspathhash() everywhere `cf2dc1c238` (speed up alt_odb_usable() with many alternates, 2021-07-07) introduced the function fspathhash() for calculating path hashes while respecting the configuration option core.ignorecase. Call it instead of open-coding it; the resulting code is shorter and less repetitive. Signed-off-by: René Scharfe <l.s.r@web.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>	4 years ago
Derrick Stolee	69bdbdb0ee	dir.c: accept a directory as part of cone-mode patterns When we have sparse directory entries in the index, we want to compare that directory against sparse-checkout patterns. Those pattern matching algorithms are built expecting a file path, not a directory path. This is especially important in the "cone mode" patterns which will match files that exist within the "parent directories" as well as the recursive directory matches. If path_matches_pattern_list() is given a directory, we can add a fake filename ("-") to the directory and get the same results as before, assuming we are in cone mode. Since sparse index requires cone mode patterns, this is an acceptable assumption. Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Derrick Stolee <dstolee@microsoft.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	4 years ago
Eric Wong	cf2dc1c238	speed up alt_odb_usable() with many alternates With many alternates, the duplicate check in alt_odb_usable() wastes many cycles doing repeated fspathcmp() on every existing alternate. Use a khash to speed up lookups by odb->path. Since the kh_put_* API uses the supplied key without duplicating it, we also take advantage of it to replace both xstrdup() and strbuf_release() in link_alt_odb_entry() with strbuf_detach() to avoid the allocation and copy. In a test repository with 50K alternates and each of those 50K alternates having one alternate each (for a total of 100K total alternates); this speeds up lookup of a non-existent blob from over 16 minutes to roughly 2.7 seconds on my busy workstation. Note: all underlying git object directories were small and unpacked with only loose objects and no packs. Having to load packs increases times significantly. Signed-off-by: Eric Wong <e@80x24.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>	4 years ago
Ævar Arnfjörð Bjarmason	ce93a4c612	dir.[ch]: replace dir_init() with DIR_INIT 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>	4 years ago
Elijah Newren	906fc557b7	dir: introduce readdir_skip_dot_and_dotdot() helper 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>	4 years ago
Derrick Stolee	eef814828f	dir: update stale description of treat_directory() 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>	4 years ago
Junio C Hamano	2c9f1bfdb4	Revert "dir: update stale description of treat_directory()" This reverts commit `4e689d8171`, to be replaced with a reworked version.	4 years ago
Junio C Hamano	1df046bcff	Revert "dir: introduce readdir_skip_dot_and_dotdot() helper" This reverts commit `b548f0f156`, to be replaced with a reworked version.	4 years ago
Elijah Newren	b548f0f156	dir: introduce readdir_skip_dot_and_dotdot() helper 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>	4 years ago
Derrick Stolee	4e689d8171	dir: update stale description of treat_directory() 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>	4 years ago
Elijah Newren	dd55fc0df1	dir: traverse into untracked directories if they may have ignored subfiles 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>	4 years ago
Elijah Newren	aa6e1b21e5	dir: avoid unnecessary traversal into ignored directory 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>	4 years ago
Elijah Newren	7fe1ffdafa	dir: report number of visited directories and paths with trace2 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>	4 years ago
Elijah Newren	7f9dd87922	dir: convert trace calls to trace2 equivalents Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	4 years ago
brian m. carlson	14228447c9	hash: provide per-algorithm null OIDs 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>	4 years ago
brian m. carlson	92e2cab96b	Always use oidread to read into struct object_id 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>	4 years ago
Derrick Stolee	d425f65127	dir: ensure full index 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>	4 years ago
Derrick Stolee	847a9e5d4f	*: remove 'const' qualifier for struct index_state 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>	4 years ago
René Scharfe	ca56dadb4b	use CALLOC_ARRAY 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>	4 years ago
Jeff Hostetler	6347d649bc	dir: fix malloc of root untracked_cache_dir 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>	4 years ago
Jeff King	feb9b7792f	exclude: do not respect symlinks for in-tree .gitignore 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>	4 years ago
Jeff King	1679d60bfc	exclude: add flags parameter to add_patterns() 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>	4 years ago
Derrick Stolee	dd23022acb	sparse-checkout: load sparse-checkout patterns 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>	4 years ago
Elijah Newren	6da1a25814	hashmap: provide deallocation function names 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>	4 years ago
Alex Vandiver	e5cf6d3df4	dir.c: fix comments to agree with argument name 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>	4 years ago
Jeff King	842385b8a4	dir.c: drop unused "untracked" from treat_path_fast() 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>	4 years ago
Elijah Newren	eceba53214	dir: fix problematic API to avoid memory leaks 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>	5 years ago
Elijah Newren	dad4f23ce5	dir: make clear_directory() free all relevant memory 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>	5 years ago
Jeff King	8dc3156373	clear_pattern_list(): clear embedded hashmaps 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>	5 years ago
Elijah Newren	ab282aa548	dir: avoid prematurely marking nonbare repositories as matches 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>	5 years ago
Martin Ågren	cada7308ad	dir: check pathspecs before returning `path_excluded` 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>	5 years ago
Elijah Newren	351ea1c3cb	dir, clean: avoid disallowed behavior 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>	5 years ago
Elijah Newren	e6c0be9239	dir: fix a few confusing comments Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	5 years ago
Elijah Newren	f1f061e11d	dir: fix treatment of negated pathspecs 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>	5 years ago
Elijah Newren	95c11ecc73	Fix error-prone fill_directory() API; make it only return matches Traditionally, the expected calling convention for the dir.c API was: fill_directory(&dir, ..., pathspec) foreach entry in dir->entries: if (dir_path_match(entry, pathspec)) process_or_display(entry) This may have made sense once upon a time, because the fill_directory() call could use cheap checks to avoid doing full pathspec matching, and an external caller may have wanted to do other post-processing of the results anyway. However: * this structure makes it easy for users of the API to get it wrong * this structure actually makes it harder to understand fill_directory() and the functions it uses internally. It has tripped me up several times while trying to fix bugs and restructure things. * relying on post-filtering was already found to produce wrong results; pathspec matching had to be added internally for multiple cases in order to get the right results (see commits `404ebceda0` (dir: also check directories for matching pathspecs, 2019-09-17) and `89a1f4aaf7` (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>	5 years ago
Elijah Newren	7f45ab2dca	dir: replace double pathspec matching with single in treat_directory() 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>	5 years ago
Elijah Newren	1684644489	dir: include DIR_KEEP_UNTRACKED_CONTENTS handling in treat_directory() 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>	5 years ago
Elijah Newren	8d92fb2927	dir: replace exponential algorithm with a linear one 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 `404ebceda0` ("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 `df5bcdf83a` ("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: * `3aca58045f` (git-clean.txt: do not claim we will delete files with -n/--dry-run, 2019-09-17) * `09487f2cba` (clean: avoid removing untracked files in a nested git repository, 2019-09-17) * `e86bbcf987` (clean: disambiguate the definition of -d, 2019-09-17) Examples where testcases were declared wrong and changed: * `09487f2cba` (clean: avoid removing untracked files in a nested git repository, 2019-09-17) * `e86bbcf987` (clean: disambiguate the definition of -d, 2019-09-17) * `a2b13367fe` (Revert "dir.c: make 'git-status --ignored' work within leading directories", 2019-12-10) Examples where testcases were clearly inadequate: * `502c386ff9` (t7300-clean: demonstrate deleting nested repo with an ignored file breakage, 2019-08-25) * `7541cc5302` (t7300: add testcases showing failure to clean specified pathspecs, 2019-09-17) * `a5e916c745` (dir: fix off-by-one error in match_pathspec_item, 2019-09-17) * `404ebceda0` (dir: also check directories for matching pathspecs, 2019-09-17) * `09487f2cba` (clean: avoid removing untracked files in a nested git repository, 2019-09-17) * `e86bbcf987` (clean: disambiguate the definition of -d, 2019-09-17) * `452efd11fb` (t3011: demonstrate directory traversal failures, 2019-12-10) * `b9670c1f5e` (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: * `902b90cf42` (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 / (606024365) = 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>	5 years ago
Derrick Stolee	0bbd0e8b52	dir: refactor treat_directory to clarify control flow 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>	5 years ago
Elijah Newren	2df179d3df	dir: fix confusion based on variable tense 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>	5 years ago
Elijah Newren	0126d1415a	dir: fix broken comment Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	5 years ago
Elijah Newren	cd129eed98	dir: consolidate treat_path() and treat_one_path() Commit `16e2cfa909` ("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 `b9670c1f5e` ("dir: fix checks on common prefix directory", 2019-12-19). Finally, in commit `ad6f2157f9` ("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>	5 years ago
Elijah Newren	446f46d8c7	dir: fix simple typo in comment Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	5 years ago

1 2 3 4 5 ...

593 Commits (8a0d52dfd870af50b9c28baf66347f5eaaf14e6e)