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