Re: [PATCH v6 1/6] submodules: add helper functions to determine presence of submodules

[Brandon Williams <bmwill@google.com>]

On 12/01, Jeff King wrote:
> On Thu, Dec 01, 2016 at 10:46:23AM -0800, Junio C Hamano wrote:
> 
> > > mkpath() is generally an unsafe function because it uses a static
> > > buffer, but it's handy and safe for handing values to syscalls like
> > > this.
> > 
> > I think your "unsafe" is not about thread-safety but about "the
> > caller cannot rely on returned value staying valid for long haul".
> > If this change since v5 is about thread-safety, I am not sure if it
> > is safe to use mkpath here.
> 
> Oh, good point. I meant "staying valid", but somehow totally forgot that
> we cared about thread reentrancy here. As if I hadn't just spent an hour
> debugging a thread problem.
> 
> My suggestion is clearly nonsense.
> 
> > I am a bit wary of making the check too sketchy like this, but this
> > is not about determining if a random "path" that has ".git" in a
> > superproject working tree is a submodule or not (that information
> > primarily comes from the superproject index), so I tend to agree
> > with the patch that it is sufficient to check presence of ".git"
> > alone.
> 
> The real danger is that it is a different check than the child process
> is going to use, so they may disagree (see the almost-infinite-loop
> discussion elsewhere).
> 
> It feels quite hacky, but checking:
> 
>   if (is_git_directory(suspect))
> 	return 1; /* actual git dir */
>   if (!stat(suspect, &st) && S_ISREG(st.st_mode))
> 	return 1; /* gitfile; may or may not be valid */
>   return 0;
> 
> is a little more robust, because the child process will happily skip a
> non-repo ".git" and keep walking back up to the superproject. Whereas if
> it sees any ".git" file, even if it is bogus, it will barf then and
> there.
> 
> I'm actually not sure if that latter behavior is a bug or not. I don't
> think it was really planned out, and it obviously is inconsistent with
> the other repo-discovery cases. But it is a convenient side effect for
> submodules, and I doubt anybody is bothered by it in practice.
> 
> -Peff

I think this more robust check is probably a good idea, that way we
don't step into a submodule with a .git directory that isn't really a
.git dir.

-- 
Brandon Williams