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

[Jeff King <peff@peff.net>]

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