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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
