Sebastian Schuberth <sschuberth@xxxxxxxxx> writes:
> On Thu, Jan 2, 2014 at 10:08 PM, Junio C Hamano <gitster@xxxxxxxxx> 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).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html