Uma Srinivasan <usrinivasan@xxxxxxxxxxx> 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).