From e86bbcf987faad8e487beb814351a404fa80957f Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Tue, 17 Sep 2019 09:35:01 -0700 Subject: [PATCH] clean: disambiguate the definition of -d The -d flag pre-dated git-clean's ability to have paths specified. As such, the default for git-clean was to only remove untracked files in the current directory, and -d existed to allow it to recurse into subdirectories. The interaction of paths and the -d option appears to not have been carefully considered, as evidenced by numerous bugs and a dearth of tests covering such pairings in the testsuite. The definition turns out to be important, so let's look at some of the various ways one could interpret the -d option: A) Without -d, only look in subdirectories which contain tracked files under them; with -d, also look in subdirectories which are untracked for files to clean. B) Without specified paths from the user for us to delete, we need to have some kind of default, so...without -d, only look in subdirectories which contain tracked files under them; with -d, also look in subdirectories which are untracked for files to clean. The important distinction here is that choice B says that the presence or absence of '-d' is irrelevant if paths are specified. The logic behind option B is that if a user explicitly asked us to clean a specified pathspec, then we should clean anything that matches that pathspec. Some examples may clarify. Should git clean -f untracked_dir/file remove untracked_dir/file or not? It seems crazy not to, but a strict reading of option A says it shouldn't be removed. How about git clean -f untracked_dir/file1 tracked_dir/file2 or git clean -f untracked_dir_1/file1 untracked_dir_2/file2 ? Should it remove either or both of these files? Should it require multiple runs to remove both the files listed? (If this sounds like a crazy question to even ask, see the commit message of "t7300: Add some testcases showing failure to clean specified pathspecs" added earlier in this patch series.) What if -ffd were used instead of -f -- should that allow these to be removed? Should it take multiple invocations with -ffd? What if a glob (such as '*tracked*') were used instead of spelling out the directory names? What if the filenames involved globs, such as git clean -f '*.o' or git clean -f '*/*.o' ? The current documentation actually suggests a definition that is slightly different than choice A, and the implementation prior to this series provided something radically different than either choices A or B. (The implementation, though, was clearly just buggy). There may be other choices as well. However, for almost any given choice of definition for -d that I can think of, some of the examples above will appear buggy to the user. The only case that doesn't have negative surprises is choice B: treat a user-specified path as a request to clean all untracked files which match that path specification, including recursing into any untracked directories. Change the documentation and basic implementation to use this definition. There were two regression tests that indirectly depended on the current implementation, but neither was about subdirectory handling. These two tests were introduced in commit 5b7570cfb41c ("git-clean: add tests for relative path", 2008-03-07) which was solely created to add coverage for the changes in commit fb328947c8e ("git-clean: correct printing relative path", 2008-03-07). Both tests specified a directory that happened to have an untracked subdirectory, but both were only checking that the resulting printout of a file that was removed was shown with a relative path. Update these tests appropriately. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- Documentation/git-clean.txt | 10 ++++++---- builtin/clean.c | 8 ++++++++ t/t7300-clean.sh | 2 ++ 3 files changed, 16 insertions(+), 4 deletions(-) diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt index e84ffc9396..3ab749b921 100644 --- a/Documentation/git-clean.txt +++ b/Documentation/git-clean.txt @@ -26,10 +26,12 @@ are affected. OPTIONS ------- -d:: - Remove untracked directories in addition to untracked files. - If an untracked directory is managed by a different Git - repository, it is not removed by default. Use -f option twice - if you really want to remove such a directory. + Normally, when no is specified, git clean will not + recurse into untracked directories to avoid removing too much. + Specify -d to have it recurse into such directories as well. + If any paths are specified, -d is irrelevant; all untracked + files matching the specified paths (with exceptions for nested + git directories mentioned under `--force`) will be removed. -f:: --force:: diff --git a/builtin/clean.c b/builtin/clean.c index d5579da716..68d70e41c0 100644 --- a/builtin/clean.c +++ b/builtin/clean.c @@ -949,6 +949,14 @@ int cmd_clean(int argc, const char **argv, const char *prefix) dir.flags |= DIR_SHOW_OTHER_DIRECTORIES; + if (argc) { + /* + * Remaining args implies pathspecs specified, and we should + * recurse within those. + */ + remove_directories = 1; + } + if (remove_directories) dir.flags |= DIR_SHOW_IGNORED_TOO | DIR_KEEP_UNTRACKED_CONTENTS; diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh index d83aeb7dc2..530dfdab34 100755 --- a/t/t7300-clean.sh +++ b/t/t7300-clean.sh @@ -117,6 +117,7 @@ test_expect_success C_LOCALE_OUTPUT 'git clean with relative prefix' ' would_clean=$( cd docs && git clean -n ../src | + grep part3 | sed -n -e "s|^Would remove ||p" ) && verbose test "$would_clean" = ../src/part3.c @@ -129,6 +130,7 @@ test_expect_success C_LOCALE_OUTPUT 'git clean with absolute path' ' would_clean=$( cd docs && git clean -n "$(pwd)/../src" | + grep part3 | sed -n -e "s|^Would remove ||p" ) && verbose test "$would_clean" = ../src/part3.c