[v3,01/12] t7300: add testcases showing failure to clean specified pathspecs
diff mbox series

Message ID 20190912221240.18057-2-newren@gmail.com
State New
Headers show
Series
  • Fix some git clean issues
Related show

Commit Message

Elijah Newren Sept. 12, 2019, 10:12 p.m. UTC
Someone brought me a testcase where multiple git-clean invocations were
required to clean out unwanted files:
  mkdir d{1,2}
  touch d{1,2}/ut
  touch d1/t && git add d1/t
With this setup, the user would need to run
  git clean -ffd */ut
twice to delete both ut files.

A little testing showed some interesting variants:
  * If only one of those two ut files existed (either one), then only one
    clean command would be necessary.
  * If both directories had tracked files, then only one git clean would
    be necessary to clean both files.
  * If both directories had no tracked files then the clean command above
    would never clean either of the untracked files despite the pathspec
    explicitly calling both of them out.

A bisect showed that the failure to clean out the files started with
commit cf424f5fd89b ("clean: respect pathspecs with "-d", 2014-03-10).
However, that pointed to a separate issue: while the "-d" flag was used
by the original user who showed me this problem, that flag should have
been irrelevant to this problem.  Testing again without the "-d" flag
showed that the same buggy behavior exists without using that flag, and
has in fact existed since before cf424f5fd89b.

Add testcases showing that multiple untracked files within entirely
untracked directories cannot be cleaned when specifying these files to
git clean via pathspecs.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 t/t7300-clean.sh | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

Comments

Junio C Hamano Sept. 13, 2019, 6:54 p.m. UTC | #1
Elijah Newren <newren@gmail.com> writes:

> +test_expect_failure 'git clean handles being told what to clean' '
> +	mkdir -p d1 d2 &&
> +	touch d1/ut d2/ut &&
> +	git clean -f */ut &&
> +	test_path_is_missing d1/ut &&
> +	test_path_is_missing d2/ut
> +'

Looks like d1 and d2 are new directories and the paths we see in the
test are the only ones that are involved (i.e. we do not rely on any
leftover cruft in d[12]/ from previous tests).  If so, perhaps it is
easier to follow by starting the tests with "rm -fr d1 d2 &&" or
something to assure the readers of the script (not this patch, but
the resulting file down the road) about the isolation?  The same
comment applies to the remainder.

Also, you talked about tracked paths in the proposed log message; do
they not participate in reproducing the issue(s)?

Thanks.


> +test_expect_failure 'git clean handles being told what to clean, with -d' '
> +	mkdir -p d1 d2 &&
> +	touch d1/ut d2/ut &&
> +	git clean -ffd */ut &&
> +	test_path_is_missing d1/ut &&
> +	test_path_is_missing d2/ut
> +'
> +
> +test_expect_failure 'git clean works if a glob is passed without -d' '
> +	mkdir -p d1 d2 &&
> +	touch d1/ut d2/ut &&
> +	git clean -f "*ut" &&
> +	test_path_is_missing d1/ut &&
> +	test_path_is_missing d2/ut
> +'
> +
> +test_expect_failure 'git clean works if a glob is passed with -d' '
> +	mkdir -p d1 d2 &&
> +	touch d1/ut d2/ut &&
> +	git clean -ffd "*ut" &&
> +	test_path_is_missing d1/ut &&
> +	test_path_is_missing d2/ut
> +'
> +
>  test_expect_success MINGW 'handle clean & core.longpaths = false nicely' '
>  	test_config core.longpaths false &&
>  	a50=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa &&
Elijah Newren Sept. 13, 2019, 7:10 p.m. UTC | #2
On Fri, Sep 13, 2019 at 11:54 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> > +test_expect_failure 'git clean handles being told what to clean' '
> > +     mkdir -p d1 d2 &&
> > +     touch d1/ut d2/ut &&
> > +     git clean -f */ut &&
> > +     test_path_is_missing d1/ut &&
> > +     test_path_is_missing d2/ut
> > +'
>
> Looks like d1 and d2 are new directories and the paths we see in the
> test are the only ones that are involved (i.e. we do not rely on any
> leftover cruft in d[12]/ from previous tests).  If so, perhaps it is
> easier to follow by starting the tests with "rm -fr d1 d2 &&" or
> something to assure the readers of the script (not this patch, but
> the resulting file down the road) about the isolation?  The same
> comment applies to the remainder.

Makes sense.

> Also, you talked about tracked paths in the proposed log message; do
> they not participate in reproducing the issue(s)?

If there is only one directory which has no tracked files, then the
user can clean up the files -- but confusingly, they have to issue the
same git-clean command multiple times.  If multiple directories have
no untracked files, git-clean will never clean them out.  I probably
didn't do a very good job explaining that although I started with the
case with one tracked, that I view the case without any as the more
general case -- and that solving it solves both problems.  I could
probably make that clearer in the commit message.  (Or maybe just add
more testcases even if slightly duplicative, I guess.)
Junio C Hamano Sept. 13, 2019, 8:29 p.m. UTC | #3
Elijah Newren <newren@gmail.com> writes:

>> Also, you talked about tracked paths in the proposed log message; do
>> they not participate in reproducing the issue(s)?
>
> If there is only one directory which has no tracked files, then the
> user can clean up the files -- but confusingly, they have to issue the
> same git-clean command multiple times.  If multiple directories have
> no untracked files, git-clean will never clean them out.  I probably
> didn't do a very good job explaining that although I started with the
> case with one tracked, that I view the case without any as the more
> general case -- and that solving it solves both problems.  I could
> probably make that clearer in the commit message.  (Or maybe just add
> more testcases even if slightly duplicative, I guess.)

My comment/puzzlement indeed was the lack of any tracked file in
your tests, even though the log message did talk about one's
presence making a difference in the outcome.

Thanks.

Patch
diff mbox series

diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index d01fd120ab..2c254c773c 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -691,6 +691,38 @@  test_expect_failure 'git clean -d skips nested repo containing ignored files' '
 	test_path_is_file nested-repo-with-ignored-file/file
 '
 
+test_expect_failure 'git clean handles being told what to clean' '
+	mkdir -p d1 d2 &&
+	touch d1/ut d2/ut &&
+	git clean -f */ut &&
+	test_path_is_missing d1/ut &&
+	test_path_is_missing d2/ut
+'
+
+test_expect_failure 'git clean handles being told what to clean, with -d' '
+	mkdir -p d1 d2 &&
+	touch d1/ut d2/ut &&
+	git clean -ffd */ut &&
+	test_path_is_missing d1/ut &&
+	test_path_is_missing d2/ut
+'
+
+test_expect_failure 'git clean works if a glob is passed without -d' '
+	mkdir -p d1 d2 &&
+	touch d1/ut d2/ut &&
+	git clean -f "*ut" &&
+	test_path_is_missing d1/ut &&
+	test_path_is_missing d2/ut
+'
+
+test_expect_failure 'git clean works if a glob is passed with -d' '
+	mkdir -p d1 d2 &&
+	touch d1/ut d2/ut &&
+	git clean -ffd "*ut" &&
+	test_path_is_missing d1/ut &&
+	test_path_is_missing d2/ut
+'
+
 test_expect_success MINGW 'handle clean & core.longpaths = false nicely' '
 	test_config core.longpaths false &&
 	a50=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa &&