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

[Brandon Williams <bmwill@google.com>]

On 12/01, Brandon Williams wrote:
> 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.

Looks like this is a no-go as well...the call to is_git_directory() ends
up calling real_path...which ends up performing the chdir call, which
puts us right back to where we started!  (as a side note I was using
is_git_directory else where...which I now know I can't use)

-- 
Brandon Williams