Re: [PATCH] Fix safe_create_leading_directories() for Windows

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 21, 2014, 20:09 UTC
Message-ID: <xmqqeh41umfv.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CAHGBnuNoTrRwnjp7ZqMgveLHZeV68cxOqawf7nWo7gnAAYfSOw@mail.gmail.com>

Sebastian Schuberth <sschuberth@gmail.com> writes:

Show 15 quoted lines

> On Thu, Jan 2, 2014 at 10:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>>> Seems like the path to clone to is taken as-is from argv in
>>> cmd_clone(). So maybe another solution would be to replace all
>>> backslashes with forward slashes already there?
>>
>> That sounds like a workable alternative, and it might even be a
>> preferable solution but if and only if clone's use of the function
>> to create paths that lead to a new working tree location is the only
>> (ab)use of the function.  That was what I meant when I said "it may
>> be that it is a bug in the specific caller".  AFAIK, the function
>
> I think Dscho made valid points in his other mail that the better
> solution still is to make safe_create_leading_directories() actually
> safe, also regarding its arguments.

Sorry, but I think I misremebered the reason why we have that function.

There are two reasons we may need to do a rough equivalent of

	system("mkdir -p $(dirname a/b/c)")

given an argument 'a/b/c' in our codebase.

 1. We would want to be able to create 'c' such that we can later
    refer to "a/b/c" to retrieve what we wrote there, so we need to
    make sure that "a/b/" refers to a writable directory.

 2. We were told by the index (or a caller that will then later
    update the index in a consistent way) that 'a/b/c' must exist in
    the working tree, so we need to make sure that "a/" and "a/b/"
    are both directories (not a symlink to a directory elsewhere).

Seeing hits "git grep safe_create_leading_directories" from apply and merge-recursive led me to incorrectly assume that this function was meant to do the latter [*1*].

But the original purpose of safe_create_leading_directories() in sha1_file.c was to "mkdir -p" inside .git/ directories when writing to refs e.g. "refs/heads/foo/bar", and for this kind of use, we must honor existing symlinks. ".git/refs" may be a symbolic link in a working tree managed by "new-workdir" to the real repository, and we do not want to unlink && mkdir it.

We even have an "offset-1st-component" call in the function, which is a clear sign that we would want this as usable for the full path in the filesystem, which is an indication that this should not be used to create paths in the working tree.

So I think it is the right thing to do to allow the function accept platform specific path here, and it is OK for "clone" to call it to create the path leading to the location of the new repository.

A related tangent is that somebody needs to audit the callers to make sure this function is _not_ used to create leading paths in the working tree. If a merge tells us to create "foo/bar/baz.test", we should not do an equivalent of "mkdir -p foo/bar && cat >baz.test"; we must make sure foo/ and foo/bar are real directories without any symbolic link in the leading paths components (the same thing can be said for "apply"). I have a suspicion that the two hits from apply and merge-recursive are showing an existing bug that is not platform specific.

Thanks.

[Footnote]

*1* In such a context, getting "a/b\c/d" and failing to make sure "a/" is a directory that has "b\c/" as its immediate subdirectory is a bug---especially, splitting at the backslash between 'b' and 'c' and creating 'a/b/c/' is not acceptable. The platform code should instead say "sorry, we cannot do that" if it cannot create a directory entry "b\c" in the directory "a/" (which would make sure such a non-portable path in projects will be detected).