Re: git submodules implementation question

[Junio C Hamano <gitster@pobox.com>]

Uma Srinivasan <usrinivasan@twitter.com> writes:

> I think the following fix is still needed to is_submodule_modified():
>
>         strbuf_addf(&buf, "%s/.git", path);
>         git_dir = read_gitfile(buf.buf);
>         if (!git_dir) {
>                 git_dir = buf.buf;
>  ==>               if (!is_git_directory(git_dir)) {
>  ==>                     die("Corrupted .git dir in submodule %s", path);
>  ==>               }
>         }

If it is so important that git_dir is a valid Git
repository after

	git_dir = read_gitfile(buf.buf);
        if (!git_dir)
        	git_dir = buf.buf;

is done to determine what "git_dir" to use, it seems to me that it
does not make much sense to check ONLY dir/.git that is a directory
and leave .git/modules/$name that dir/.git file points at unchecked.

But there is much bigger problem with the above addition, I think.
There also can be a case where dir/ does not even have ".git" in it.
A submodule the user is not interested in will just have an empty
directory there, and immediately after the above three lines I
reproduced above, we have this

	if (!is_directory(git_dir)) {
        	strbuf_release(&buf);
                return 0;
	}

The added check will break the use case.  If anything, that check,
if this code needs to verify that "git_dir" points at a valid Git
repository, should come _after_ that.

Shouldn't "git-status --porcelain" run in the submodule notice that
it is not a valid repository and quickly die anyway?  Why should we
even check before spawning that process in the first place?

I might suggest to update prepare_submodule_repo_env() so that the
spawned process will *NOT* have to guess where the working tree and
repository by exporting GIT_DIR (set to "git_dir" we discover above)
and GIT_WORK_TREE (set to "." as cp.dir is set to the path to the
working tree of the submodule).  That would stop the "git status" to
guess (and fooled by a corrupted dir/.git that is not a git
repository).