Clean cleanups
cover Clean cleanups
This fixes the performance regression reported at [1], and also performs a few other cleanups found while investigating the issue.
[1]
https://lore.kernel.org/git/CAJB88a23uU2WfB0mnB9NfNbtgmABhNOWNOEMBt7rRVu7uL_C9A@mail.gmail.com/
Elijah Newren (4):
dir: fix a few confusing comments
dir, clean: avoid disallowed behavior
clean: consolidate handling of ignored parameters
clean: optimize and document cases where we recurse into
subdirectories
builtin/clean.c | 49 ++++++++++++++++++++++++++++++++++++++----------- dir.c | 15 ++++++++++----- 2 files changed, 48 insertions(+), 16 deletions(-)
base-commit: 20514004ddf1a3528de8933bc32f284e175e1012
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-806%2Fnewren%2Fclean-cleanups-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-806/newren/clean-cleanups-v1
Pull-Request: https://github.com/git/git/pull/806
Signature · 1 line
gitgitgadget
PATCH 1/4 dir: fix a few confusing comments
From: Elijah Newren <[email protected]>
--- dir.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
dir.c +6 -5
@@ -1820,7 +1820,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,* to recurse into untracked/ignored directories if either of the * following bits is set: * - DIR_SHOW_IGNORED_TOO (because then we need to determine if -* there are ignored directories below) +* there are ignored entries below) * - DIR_HIDE_EMPTY_DIRECTORIES (because we have to determine if * the directory is empty) */
@@ -1838,10 +1838,11 @@ static enum path_treatment treat_directory(struct dir_struct *dir,return path_excluded;/* -* If we have we don't want to know the all the paths under an -* untracked or ignored directory, we still need to go into the -* directory to determine if it is empty (because an empty directory -* should be path_none instead of path_excluded or path_untracked). +* Even if we don't want to know all the paths under an untracked or +* ignored directory, we may still need to go into the directory to +* determine if it is empty (because with DIR_HIDE_EMPTY_DIRECTORIES, +* an empty directory should be path_none instead of path_excluded or +* path_untracked). */ check_only = ((dir->flags &DIR_HIDE_EMPTY_DIRECTORIES ) && !(dir->flags &DIR_SHOW_IGNORED_TOO ));
Signature · 1 line
gitgitgadget
PATCH 2/4 dir, clean: avoid disallowed behavior
In reply to Elijah Newren via GitGitGadgetFrom: Elijah Newren <[email protected]>
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.
--- builtin/clean.c | 2 +- dir.c | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-)
builtin/clean.c +1 -1
@@ -954,7 +954,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix) remove_directories =1 ; } -if (remove_directories) +if (remove_directories && !ignored_only) dir.flags |=DIR_SHOW_IGNORED_TOO |DIR_KEEP_UNTRACKED_CONTENTS ;if (read_cache () <0 )
dir.c +4 -0
@@ -193,6 +193,10 @@ int fill_directory(struct dir_struct *dir,const char *prefix;size_t prefix_len; +unsigned exclusive_flags =DIR_SHOW_IGNORED |DIR_SHOW_IGNORED_TOO ; +if ((dir->flags & exclusive_flags) == exclusive_flags) +BUG ("DIR_SHOW_IGNORED and DIR_SHOW_IGNORED_TOO are exclusive" ); +/* * Calculate common prefix for the pathspec, and * use that to optimize the directory walk
Signature · 1 line
gitgitgadget
PATCH 3/4 clean: consolidate handling of ignored parameters
In reply to Elijah Newren via GitGitGadgetFrom: Elijah Newren <[email protected]>
I spent a long time trying to figure out how and whether the code worked with different values of ignore, ignore_only, and remove_directories. After lots of time setting up lots of testcases, sifting through lots of print statements, and walking through the debugger, I finally realized that one piece of code related to how it was all setup was found in clean.c rather than dir.c. Make a change that would have made it easier for me to do the extra testing by putting this handling in one spot.
--- builtin/clean.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-)
builtin/clean.c +7 -9
@@ -924,12 +924,6 @@ int cmd_clean(int argc, const char **argv, const char *prefix) 0);memset (&dir,0 ,sizeof (dir)); -if (ignored_only) - dir.flags |=DIR_SHOW_IGNORED ; - -if (ignored && ignored_only) -die (_ ("-x and -X cannot be used together" )); -if (!interactive && !dry_run && !force) {if (config_set) die(_("clean.requireForce set to true and neither -i, -n, nor -f given; "
@@ -946,6 +940,13 @@ int cmd_clean(int argc, const char **argv, const char *prefix) dir.flags |=DIR_SHOW_OTHER_DIRECTORIES ; +if (ignored && ignored_only) +die (_ ("-x and -X cannot be used together" )); +if (!ignored) +setup_standard_excludes (&dir); +if (ignored_only) + dir.flags |=DIR_SHOW_IGNORED ; +if (argc) {/* * Remaining args implies pathspecs specified, and we should
@@ -960,9 +961,6 @@ int cmd_clean(int argc, const char **argv, const char *prefix)if (read_cache () <0 )die (_ ("index file corrupt" )); -if (!ignored) -setup_standard_excludes (&dir); - pl =add_pattern_list (&dir,EXC_CMDL ,"--exclude option" );for (i =0 ; i < exclude_list.nr; i++)add_pattern (exclude_list.items[i].string,"" ,0 , pl, -(i+1 ));
Signature · 1 line
gitgitgadget
PATCH 4/4 clean: optimize and document cases where we recurse into subdirectories
In reply to Elijah Newren via GitGitGadgetFrom: Elijah Newren <[email protected]>
Commit 6b1db43109 ("clean: teach clean -d to preserve ignored paths", 2017-05-23) added the following code block (among others) to git-clean:
if (remove_directories)
dir.flags |= DIR_SHOW_IGNORED_TOO | DIR_KEEP_UNTRACKED_CONTENTS;The reason for these flags is well documented in the commit message, but isn't obvious just from looking at the code. Add some explanations to the code to make it clearer.
Further, it appears git-2.26 did not correctly handle this combination of flags from git-clean. With both these flags and without DIR_SHOW_IGNORED_TOO_MODE_MATCHING set, git is supposed to recurse into all untracked AND ignored directories. git-2.26.0 clearly was not doing that. I don't know the full reasons for that or whether git < 2.27.0 had additional unknown bugs because of that misbehavior, because I don't feel it's worth digging into. As per the huge changes and craziness documented in commit 8d92fb2927 ("dir: replace exponential algorithm with a linear one", 2020-04-01), the old algorithm was a mess and was thrown out. What I can say is that git-2.27.0 correctly recurses into untracked AND ignored directories with that combination.
However, in clean's case we don't need to recurse into ignored directories; that is just a waste of time. Thus, when git-2.27.0 started correctly handling those flags, we got a performance regression report. Rather than relying on other bugs in fill_directory()'s former logic to provide the behavior of skipping ignored directories, make use of the DIR_SHOW_IGNORED_TOO_MODE_MATCHING value specifically added in commit eec0f7f2b7 ("status: add option to show ignored files differently", 2017-10-30) for this purpose.
--- builtin/clean.c | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-)
builtin/clean.c +31 -2
@@ -955,8 +955,37 @@ int cmd_clean(int argc, const char **argv, const char *prefix) remove_directories =1 ; } -if (remove_directories && !ignored_only) - dir.flags |=DIR_SHOW_IGNORED_TOO |DIR_KEEP_UNTRACKED_CONTENTS ; +if (remove_directories && !ignored_only) { +/* +* We need to know about ignored files too: +* +* If (ignored), then we will delete ignored files as well. +* +* If (!ignored), then even though we not are doing +* anything with ignored files, we need to know about them +* so that we can avoid deleting a directory of untracked +* files that also contains an ignored file within it. +* +* For the (!ignored) case, since we only need to avoid +* deleting ignored files, we can set +* DIR_SHOW_IGNORED_TOO_MODE_MATCHING in order to avoid +* recursing into a directory which is itself ignored. +*/ + dir.flags |=DIR_SHOW_IGNORED_TOO ; +if (!ignored) + dir.flags |=DIR_SHOW_IGNORED_TOO_MODE_MATCHING ; + +/* +* Let the fill_directory() machinery know that we aren't +* just recursing to collect the ignored files; we want all +* the untracked ones so that we can delete them. (Note: +* we could also set DIR_KEEP_UNTRACKED_CONTENTS when +* ignored_only is true, since DIR_KEEP_UNTRACKED_CONTENTS +* only has effect in combination with DIR_SHOW_IGNORED_TOO. It makes +* the code clearer to exclude it, though. +*/ + dir.flags |=DIR_KEEP_UNTRACKED_CONTENTS ; + }if (read_cache () <0 )die (_ ("index file corrupt" ));
Signature · 1 line
gitgitgadget
Re: [PATCH 4/4] clean: optimize and document cases where we recurse into subdirectories
Quoted 3 lines · Elijah Newren via GitGitGadget
...
However, in clean's case we don't need to recurse into ignored directories; that is just a waste of time.
Nice.
Quoted 5 lines
Rather than relying on other bugs in fill_directory()'s former logic to provide the behavior of skipping ignored directories, make use of the
DIR_SHOW_IGNORED_TOO_MODE_MATCHINGvalue specifically added in commiteec0f7f2b7("status: add option to show ignored files differently", 2017-10-30) for this purpose.
Thanks.
Re: [PATCH 4/4] clean: optimize and document cases where we recurse into subdirectories
Catching up on this - thanks Elijah for jumping on this bug and fixing it immediately. I'll let you know if I see similar bugs in the future.
Thanks,
Brian
Quoted 17 lines · Junio C Hamano
Quoted 3 lines · Elijah Newren via GitGitGadget
...
However, in clean's case we don't need to recurse into ignored directories; that is just a waste of time.Nice.
Quoted 5 lines
Rather than relying on other bugs in fill_directory()'s former logic to provide the behavior of skipping ignored directories, make use of the
DIR_SHOW_IGNORED_TOO_MODE_MATCHINGvalue specifically added in commiteec0f7f2b7("status: add option to show ignored files differently", 2017-10-30) for this purpose.Thanks.