Re: [PATCH] t3700 (add): add failing test for add with submodules

From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Apr 9, 2013, 09:19 UTC
Message-ID: <CALkWK0m_82ThYKxcr=YHGBnOaHr9yBpYFHxx2KrnQkWD_zjp1A@mail.gmail.com>
In-Reply-To: <20130408213006.GD9649@sigill.intra.peff.net>

Jeff King wrote:

Show 5 quoted lines

> 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?