From: Ramkumar Ramachandra Date: Tue, 09 Apr 2013 09:19:24 GMT Subject: Re: [PATCH] t3700 (add): add failing test for add with submodules Message-ID: In-Reply-To: <20130408213006.GD9649@sigill.intra.peff.net> Jeff King wrote: > That is not actually a submodule, but rather just a repo that happens to > be inside our working tree. I know the distinction is subtle, but > according to the thread I linked to above, we may actually treat paths > with gitlinked index entries separately already (I did not try it, > though). Agreed. treat_gitlink() dies if there is a gitlink cache_entry matching any of the pathspecs; it does one thing well, and promises what it does: however, its core logic in check_path_for_gitlink() can easily be moved into lstat_cache_matchlen() as that is more generic (checks index and worktree). die_if_path_beyond_symlink() is the perfect example to replicate. Today, there is one more caller of die_if_path_beyond_symlink(): check-ignore, so we must patch that too. On a slightly related note treat_path() also contains the logic for checking for a git repository in the worktree. Unfortunately, the code cannot be reused because it checks for a '.git' in a dirent. On the wording issue, a submodule is a submodule whether in-index or otherwise. I would write two different tests: one for in-worktree submodule and another for in-index submodule, and name them appropriately. Does that make sense?