Re: git submodules implementation question

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 30, 2016, 17:53 UTC
Message-ID: <xmqqzinu3zyw.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAN5XQftCC+TUm2Jx4q3V9oFbXndtFx3H+daoB3TD3eWUs6s54A@mail.gmail.com>

Uma Srinivasan <usrinivasan@twitter.com> writes:

Show 10 quoted lines

> 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).