Re: [PATCH/RFC 3/3] ls-files/dir: use is_git_repo to detect submodules
On Sun, Dec 06, 2015 at 05:59:26PM +0100, Andreas Krey wrote: > On Sat, 05 Dec 2015 02:37:44 +, Jeff King wrote: > ... > > Hrm. Weird. You'd think it would break with the existing code if I do > > this, then: > > > ... > > - (cd a/b/c; git init) && > > + (cd a/b/c; git init && git commit --allow-empty -m foo) && > > git config remote.origin.url ../foo/bar.git && > > git submodule add ../bar/a/b/c ./a/b/c && > > I tried a -f here instead; did not work either. > > I guess I will first wade through the other failures my 'fix' > causes to see the total damage. The only other one I saw was in the completion tests. And it looked like `git add` failing in a way similar to what I dug into here. So I think it's "just" the one bug. > > We know it is a git dir, but there is no sha1 for us to actually add as > > the gitlink entry. > > > > If that is the case, then there is either some very tricky refactoring > > required, > > Yes, it looks like the return code delivered need to be slightly different > dependent on the user. Maybe. From your patch it looks like the "git-add" code will return it as "untracked". Which makes sense if we then want to add it. But if it has no HEAD commit we _cannot_ add it, as we have no sha1 to stick in the index. It should probably be ignored totally in that case. But that means you have to actually find out if HEAD is valid or not. Which is what the current code is doing. :-/ > > or what we are trying to do here is simply wrong. Maybe it > > would be simpler to just speed up resolve_gitlink_ref with a better data > > structure. > > Which is what I did on square one, but now we already have a real fix > for git clean, and now it's even less advantageous the fix the consequence > (the submodule cache blowing up) instead of the cause (asking for it > in the first place). I think "clean" is a much simpler case. It only wants to know "can I skip this entry as an untracked sub-repo?". And that is similar to what "git status" or "git ls-files" wants to know. But "git add" wants to know "can I _add_ this entry to the index", and that is a different question (but I think answered by the same code that powers ls-files). > PS: I seem to not quite have send-email under control, the envelope from > seems to made the patches not reach the mailing list (nor me in the CC). Hmm, yeah. Obviously they made it to me directly, but the list is a little bit picky. -Peff -- 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
Re: [PATCH/RFC 3/3] ls-files/dir: use is_git_repo to detect submodules
On Sat, 05 Dec 2015 02:37:44 +, Jeff King wrote: ... > Hrm. Weird. You'd think it would break with the existing code if I do > this, then: > ... > - (cd a/b/c; git init) && > + (cd a/b/c; git init && git commit --allow-empty -m foo) && > git config remote.origin.url ../foo/bar.git && > git submodule add ../bar/a/b/c ./a/b/c && I tried a -f here instead; did not work either. I guess I will first wade through the other failures my 'fix' causes to see the total damage. ... > We know it is a git dir, but there is no sha1 for us to actually add as > the gitlink entry. > > If that is the case, then there is either some very tricky refactoring > required, Yes, it looks like the return code delivered need to be slightly different dependent on the user. > or what we are trying to do here is simply wrong. Maybe it > would be simpler to just speed up resolve_gitlink_ref with a better data > structure. Which is what I did on square one, but now we already have a real fix for git clean, and now it's even less advantageous the fix the consequence (the submodule cache blowing up) instead of the cause (asking for it in the first place). I don't think we should let is_git_repository look for a valid(ish) HEAD. Andreas PS: I seem to not quite have send-email under control, the envelope from seems to made the patches not reach the mailing list (nor me in the CC). -- "Totally trivial. Famous last words." From: Linus TorvaldsDate: Fri, 22 Jan 2010 07:29:21 -0800 -- 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
Re: [PATCH/RFC 3/3] ls-files/dir: use is_git_repo to detect submodules
On Fri, Dec 04, 2015 at 07:15:17PM +0100, Andreas Krey wrote: > Using resolve_gitlink_ref to check for submodules > creates submodule list entries even when it isn't > one, and causes O(n^2) runtime behaviour in repos > with many untracked directories. > > Use is_git_repo instead for detection. > > Signed-off-by: Andreas Krey> --- > This looks good, but it breaks test - at least > number 67 ('../bar/a/b/c works with relative local > path - ../foo/bar.git') in t7400-submodule-basic.sh > > I don't really understand yet how to fix that, > but it is due to resolve_gitlink_ref looking > for a valid HEAD which is_git_repo doesn't. Hrm. Weird. You'd think it would break with the existing code if I do this, then: diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh index 540771c..944e9f5 100755 --- a/t/t7400-submodule-basic.sh +++ b/t/t7400-submodule-basic.sh @@ -754,7 +754,7 @@ test_expect_success '../bar/a/b/c works with relative local path - ../foo/bar.gi cp pristine-.git-config .git/config && cp pristine-.gitmodules .gitmodules && mkdir -p a/b/c && - (cd a/b/c; git init) && + (cd a/b/c; git init && git commit --allow-empty -m foo) && git config remote.origin.url ../foo/bar.git && git submodule add ../bar/a/b/c ./a/b/c && git submodule init && But it doesn't. So presumably there is a mismatch where some other code expects that anything which gets marked as a repo in the code you changed can also be resolved to a sha1. I'm not sure where that other code is though (in git-submodule.sh, or elsewhere in add). Perhaps we end up in index_path(), which then barfs? That would explain this (run in the trash directory after the test fails): $ cd reltest && git add a/b/c error: unable to index file a/b/c/ fatal: adding files failed We know it is a git dir, but there is no sha1 for us to actually add as the gitlink entry. If that is the case, then there is either some very tricky refactoring required, or what we are trying to do here is simply wrong. Maybe it would be simpler to just speed up resolve_gitlink_ref with a better data structure. -Peff -- 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