[PATCH] t7300: repair filesystem permissions with test_when_finished

[PATCH] t7300: repair filesystem permissions with test_when_finished

[Jeff King]

We create a directory that cannot be removed, confirm that
it cannot be removed, and then fix it like:

  chmod 0 foo &&
  test_must_fail git clean -d -f &&
  chmod 755 foo

If the middle step fails but leaves the directory (e.g., the
bug is that clean does not notice the failure), this
pollutes the test repo with an unremovable directory. Not
only does this cause further tests to fail, but it means
that "rm -rf" fails on the whole trash directory, and the
user has to intervene manually to even re-run the test script.

We can bump the "chmod 755" recovery to a test_when_finished
block to be sure that it always runs.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t7300-clean.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index 74de814..04118ad 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -426,10 +426,10 @@ test_expect_success SANITY 'removal failure' '
 
 	mkdir foo &&
 	touch foo/bar &&
+	test_when_finished "chmod 755 foo" &&
 	(exec <foo/bar &&
 	 chmod 0 foo &&
-	 test_must_fail git clean -f -d &&
-	 chmod 755 foo)
+	 test_must_fail git clean -f -d)
 '
 
 test_expect_success 'nested git work tree' '
-- 
2.0.0.566.gfe3e6b2

Re: [PATCH] t7300: repair filesystem permissions with test_when_finished

[Jonathan Nieder]

Jeff King wrote:

> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  t/t7300-clean.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Does the later "git clean -d with an unreadable empty directory" test
need the same treatment?

Thanks,
Jonathan

Re: [PATCH] t7300: repair filesystem permissions with test_when_finished

[Jeff King]

On Wed, Jul 02, 2014 at 12:01:59PM -0700, Jonathan Nieder wrote:

> Jeff King wrote:
> 
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
> >  t/t7300-clean.sh | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Does the later "git clean -d with an unreadable empty directory" test
> need the same treatment?

I don't think so, because it is also failing during the experiments I'm
doing and it is not causing the same problem.

-Peff

[RFH] "git clean" deletes excluded files in untracked directories

[Jeff King]

If you have an untracked directory that contains excluded files, like
this:

  mkdir foo
  echo content >foo/one
  echo content >foo/two
  echo "foo/one" >.gitignore

then "git clean -d" will notice that "foo" is untracked and recursively
delete it and its contents, including the ignored "foo/one".

Our stance has always been that ignored files are not precious, so in
that sense it is not a big loss. But "git clean" does provide a "-x"
option, and takes care to avoid deleting ignored files when it is not
given. So I'd argue that we should delete "foo/two" but retain
"foo/one", unless "-x" is given.

I'm not sure of the best way to go about that, though. The patch below
is my first attempt. It stops using DIR_SHOW_OTHER_DIRECTORIES when we
find the list of files to delete, which means we get the full set of
paths (so instead of "foo/", we see "foo/one" and "foo/two" after
calling fill_directory). But then we have to notice that "foo" should go
away if all of its contents do. For that, I use remove_path, which just
walks up the pathname, removing leading directories until we hit a
non-empty one.

But there are a few problems:

  1. Is it possible that we want to remove "foo/bar" but not "foo"? The
     obvious case would be that "foo" is excluded but "foo/bar" is not. I
     guess you could get that with negative excludes, like:

       foo/
       !foo/bar

  2. The output is different (we report each file we are removing,
     rather than just the root directory).

  3. The error handling is different. If we have an unreadable untracked
     directory now, fill_directory does not recurse into it; we get the
     directory name and complain when we cannot remove it. But with this
     patch, fill_directory tries to enter it, can't, and simply fails to
     report any entries inside it (this causes test failures in t7300).

So this is probably not the right way to go about it.  I'm not sure of
the best way.

One option is to actually teach clean's recursive remove_dirs function
to actually check excludes itself for each file. That feels hacky,
though, since we should be checking them already in fill_directory.

Another is to add a flag to fill_directory to indicate that it should
recurse into directories to find excluded files, but not otherwise (we
usually avoid it for reasons of efficiency, but since we would be
immediately entering them to delete anyway, I don't think that's a
concern here). I think this makes the fill_directory logic quite
complicated, though.

Suggestions?

---
diff --git a/builtin/clean.c b/builtin/clean.c
index 9a91515..f159eed 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -914,7 +914,8 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 	if (force > 1)
 		rm_flags = 0;
 
-	dir.flags |= DIR_SHOW_OTHER_DIRECTORIES;
+	if (!remove_directories)
+		dir.flags |= DIR_SHOW_OTHER_DIRECTORIES;
 
 	if (read_cache() < 0)
 		die(_("index file corrupt"));
@@ -985,7 +986,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 				printf(dry_run ? _(msg_would_remove) : _(msg_remove), qname);
 			}
 		} else {
-			res = dry_run ? 0 : unlink(abs_path.buf);
+			res = dry_run ? 0 : remove_path(abs_path.buf);
 			if (res) {
 				qname = quote_path_relative(item->string, NULL, &buf);
 				warning(_(msg_warn_remove_failed), qname);
diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index 04118ad..647844f 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -527,4 +527,12 @@ test_expect_success 'git clean -d respects pathspecs (pathspec is prefix of dir)
 	test_path_is_dir foobar
 '
 
+test_expect_success 'clean -d does not clean ignored files' '
+	mkdir -p foo &&
+	echo content >foo/file &&
+	echo "foo/*" >.gitignore &&
+	git clean -df </dev/tty &&
+	test_path_is_file foo/file
+'
+
 test_done

Re: [PATCH] t7300: repair filesystem permissions with test_when_finished

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Jul 02, 2014 at 12:01:59PM -0700, Jonathan Nieder wrote:
>
>> Jeff King wrote:
>> 
>> > Signed-off-by: Jeff King <peff@peff.net>
>> > ---
>> >  t/t7300-clean.sh | 4 ++--
>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> Does the later "git clean -d with an unreadable empty directory" test
>> need the same treatment?
>
> I don't think so, because it is also failing during the experiments I'm
> doing and it is not causing the same problem.

It tests that "clean -d" is happy if a blind rmdir(2) removes the
directory.  If it fails for whatever reason (e.g. add "exit(0)" at
the beginning of cmd_clean(), for example) to remove the directory,
we do leave an empty unreadable directory behind.

But as long as that directory is empty, that will not cause us to
remove the trash directory at the end, so we should be OK.

Re: [PATCH] t7300: repair filesystem permissions with test_when_finished

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> It tests that "clean -d" is happy if a blind rmdir(2) removes the
> directory.  If it fails for whatever reason (e.g. add "exit(0)" at
> the beginning of cmd_clean(), for example) to remove the directory,
> we do leave an empty unreadable directory behind.
>
> But as long as that directory is empty, that will not cause us to
> remove the trash directory at the end, so we should be OK.

s/remove/fail to &/; sorry for the noise.

Re: [RFH] "git clean" deletes excluded files in untracked directories

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> If you have an untracked directory that contains excluded files, like
> this:
>
>   mkdir foo
>   echo content >foo/one
>   echo content >foo/two
>   echo "foo/one" >.gitignore
>
> then "git clean -d" will notice that "foo" is untracked and recursively
> delete it and its contents, including the ignored "foo/one".

Hmph, starting from an empty repository and doing the above four
commands, and "git clean" without "-d" does not bother removing
either foo/one or foo/two.  Is this correct?

It gets worse.  Following the above four commands and then these two:

    >foo/three
    git add foo/two

and "git clean" (with or without "-d") suddenly notices that
"foo/three" is expendable, but not foo/one nor foo/two, which sounds
about right.

Honestly, as I do not use "git clean" myself, I do not know what the
"right" behaviour is for that command.  Anything it does seems just
arbitrary and wrong to me ;-)

Re: [RFH] "git clean" deletes excluded files in untracked directories

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
>
>> If you have an untracked directory that contains excluded files, like
>> this:
>>
>>   mkdir foo
>>   echo content >foo/one
>>   echo content >foo/two
>>   echo "foo/one" >.gitignore
>>
>> then "git clean -d" will notice that "foo" is untracked and recursively
>> delete it and its contents, including the ignored "foo/one".
>
> Hmph, starting from an empty repository and doing the above four
> commands, and "git clean" without "-d" does not bother removing
> either foo/one or foo/two.  Is this correct?
>
> It gets worse.  Following the above four commands and then these two:
>
>     >foo/three
>     git add foo/two
>
> and "git clean" (with or without "-d") suddenly notices that
> "foo/three" is expendable, but not foo/one nor foo/two, which sounds
> about right.
>
> Honestly, as I do not use "git clean" myself, I do not know what the
> "right" behaviour is for that command.  Anything it does seems just
> arbitrary and wrong to me ;-)

Having said that...

Let's play my usual "I do not know what I am doing because I do not
use it myself, but instead of leaving the mess other people created
that may harm users, let's try to clean it up", and the first step
for that is to clarify what is wrong about it, and what, if
anything, could be a more sensible behaviour.

"git clean" with or without "-d" in my two-command added to your
four-command example notices .gitignore and foo/three are to be
cleaned, and further with "-x" makes foo/one eligible for
removal.  I think these are all sensible.

"git clean" without "-d" in your four-command example says
".gitignore is untracked and can be cleaned", but is "foo/two" that
is not tracked fundamentally different from ".gitignore"?

The only difference I can see is one is at the top-level while
the other is not.  What bad things could happen if we remove
"foo/two" with or without "-d", making "-d" a no-op?

I know "-d" talks about "untracked directory", but notice that with
your four-command example, there is no "git add"; there is no
difference between the top-level and "foo/" in tracked-ness, so it
is a meaningless distinction.  I could be pursuaded to buy a
proposal to make "-d" almost no-op except that a directory that
becomes empty after running "git clean [-x]" is rmdir(2)'ed and
without "-d" the empty directory is kept.

Would such a pair of changes bring some sanity to the command?

No matter what we do, I suspect that people expect various
combinations of options to "git clean" has some parallel to
"ls-files -o" with various combinations of options, to allow them to
list paths that will be cleaned with "ls-files", store them away,
and then run "clean", or something, so any "fix" would involve
updating "ls-files" in a way consistent with the updated behaviour
of "clean".