Re: [PATCH/RFC 3/3] ls-files/dir: use is_git_repo to detect submodules

[Jeff King]

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

[Andreas Krey]

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 Torvalds 
Date: 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

[Jeff King]

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