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
