Re: [PATCH v2 3/3] clean: improve performance when removing lots of directories

[Jeff King <peff@peff.net>]

On Fri, Apr 17, 2015 at 08:15:40PM +0200, erik elfström wrote:

> > Doesn't this implementation get confused by modern submodule
> > checkouts and descend into and clean their working tree, though?
> > Module M with path P would have a directory P in the working tree of
> > the top-level project, and P/.git is a regular file that will fail
> > "is_git_directory()" test but records the location of the real
> > submodule repository i.e. ".git/modules/M" via the "gitdir:"
> > mechanism.
> >
> 
> Yes, there is a problem here. I've added the test below and it fails after
> my change by cleaning sub2 (sub1 is not cleaned). Are there more cases
> here that I should test for?

I wonder about the opposite case, too (finding more repos than we used
to).

It looks like your patches will find bare repositories in the tree,
whereas the current code does not (it only cares about ".git"). AFAIK,
submodules will never exist as bare in the working tree. And I have seen
repositories which embed bare repos as test cases. Admittedly this is
because I work on projects that are related to git itself, but I don't
see a reason to regress this case if the submodule code doesn't get any
benefit.

> Base on the previous discussion of the patch topic I can see 3 options
> for how to fix this:
> 
> Option 1:
>  Plug the hole in my new is_git_repository function. A quick and dirty
>  fix that passes the above test would be:

I think that makes sense. It would be nice if you could just call
read_gitfile, but that function is very anxious to die on error. So the
prerequisite step would probably be to refactor that into a
read_gitfile_gently that returns an error code.

-Peff

PS Thank you for working on this. I have been quiet because I haven't
   had a chance to look over your patches carefully yet, but overall
   they look very promising.