[PATCH] Fix safe_create_leading_directories() for Windows

[PATCH] Fix safe_create_leading_directories() for Windows

[Sebastian Schuberth]

See https://github.com/msysgit/git/pull/80.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
---
 sha1_file.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 760dd60..2114c58 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -110,12 +110,15 @@ int safe_create_leading_directories(char *path)
 	char *pos = path + offset_1st_component(path);
 	struct stat st;
 
-	while (pos) {
-		pos = strchr(pos, '/');
-		if (!pos)
-			break;
-		while (*++pos == '/')
-			;
+	while (*pos) {
+		while (!is_dir_sep(*pos)) {
+			++pos;
+			if (!*pos)
+				break;
+		}
+		/* skip consecutive directory separators */
+		while (is_dir_sep(*pos))
+			++pos;
 		if (!*pos)
 			break;
 		*--pos = '\0';
-- 
1.8.4.msysgit.0.1.ge705bba.dirty

Re: [PATCH] Fix safe_create_leading_directories() for Windows

[Johannes Schindelin]

Hi,

On Thu, 2 Jan 2014, Sebastian Schuberth wrote:

> See https://github.com/msysgit/git/pull/80.

Thanks Sebastian!

However, since the git.git project is not comfortable with the concept of
pull requests (which is why you submitted this patch via mail), I believe
that we have to explain the rationale in the commit message. So here goes
my attempt:

-- snip --
On Linux, we can get away with assuming that the directory separator is a
forward slash, but that is wrong in general. For that purpose, the
is_dir_sep() function was introduced a long time ago. By using it in
safe_create_leading_directories(), we proof said function for use on
platforms where the directory separator is different from Linux'.
-- snap --

Hmm?
Dscho

Re: [PATCH] Fix safe_create_leading_directories() for Windows

[Sebastian Schuberth]

On 02.01.2014 18:33, Johannes Schindelin wrote:

> -- snip --
> On Linux, we can get away with assuming that the directory separator is a
> forward slash, but that is wrong in general. For that purpose, the
> is_dir_sep() function was introduced a long time ago. By using it in
> safe_create_leading_directories(), we proof said function for use on
> platforms where the directory separator is different from Linux'.
> -- snap --

While I'd be fine with this, I do not think we really need it. As you 
say, is_dir_sep() has been introduced a long time ago, so people should 
be aware of it, and it should also be immediately clear from the diff 
why using it is better than hard-coding '/'.

That said, I see any further explanations on top of the commit message 
title is an added bonus, and as "just" a bonus a link to a pull request 
should be fine. You don't need to understand or appreciate the concept 
of pull requests in order to follow the link and read the text in there.

-- 
Sebastian Schuberth

Re: [PATCH] Fix safe_create_leading_directories() for Windows

[John Keeping]

On Thu, Jan 02, 2014 at 07:11:42PM +0100, Sebastian Schuberth wrote:
> On 02.01.2014 18:33, Johannes Schindelin wrote:
> 
> > -- snip --
> > On Linux, we can get away with assuming that the directory separator is a
> > forward slash, but that is wrong in general. For that purpose, the
> > is_dir_sep() function was introduced a long time ago. By using it in
> > safe_create_leading_directories(), we proof said function for use on
> > platforms where the directory separator is different from Linux'.
> > -- snap --
> 
> While I'd be fine with this, I do not think we really need it. As you 
> say, is_dir_sep() has been introduced a long time ago, so people should 
> be aware of it, and it should also be immediately clear from the diff 
> why using it is better than hard-coding '/'.
> 
> That said, I see any further explanations on top of the commit message 
> title is an added bonus, and as "just" a bonus a link to a pull request 
> should be fine. You don't need to understand or appreciate the concept 
> of pull requests in order to follow the link and read the text in there.

The commit message serves as an historical record of why a change was
made; depending on an external service to provide this information when
it can quite easily be included in the commit itself lessens the value
of the commit message.

If you look at other commits in git.git you will see that there is a
strong preference for summarising the discussion and rationale for a
commit in its message.

Re: [PATCH] Fix safe_create_leading_directories() for Windows

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi,
>
> On Thu, 2 Jan 2014, Sebastian Schuberth wrote:
>
>> See https://github.com/msysgit/git/pull/80.
>
> Thanks Sebastian!
>
> However, since the git.git project is not comfortable with the concept of
> pull requests (which is why you submitted this patch via mail), I believe
> that we have to explain the rationale in the commit message. So here goes
> my attempt:

Thanks; the conclusion is correct --- you need a good commit
message in the recorded history.  That does not have anything to do
with integrating with pulling from subsystem maintainers, which we
regularly do.

> On Linux, we can get away with assuming that the directory separator is a
> forward slash, but that is wrong in general. For that purpose, the
> is_dir_sep() function was introduced a long time ago. By using it in
> safe_create_leading_directories(), we proof said function for use on
> platforms where the directory separator is different from Linux'.

Perhaps with s|Linux|POSIX|, but more importantly, was there a
specific error scenario that triggered this change?

My quick reading of "git grep" suggests that the callsites of this
function all assume that they are to use slashes as directory
separators, and it may be that it is a bug in the specific caller
that throws a backslash-separated paths to it.

Re: [PATCH] Fix safe_create_leading_directories() for Windows

[Sebastian Schuberth]

On 02.01.2014 19:18, John Keeping wrote:

>> That said, I see any further explanations on top of the commit message
>> title is an added bonus, and as "just" a bonus a link to a pull request
>> should be fine. You don't need to understand or appreciate the concept
>> of pull requests in order to follow the link and read the text in there.
>
> The commit message serves as an historical record of why a change was
> made; depending on an external service to provide this information when
> it can quite easily be included in the commit itself lessens the value
> of the commit message.

Sure. My point just was that IMHO the commit does not *depend* on the 
information provided in the link; for me the commit was simply 
self-evident, and I just added link as optional information, not to 
replace any inline text that I would have written otherwise.

-- 
Sebastian Schuberth

Re: [PATCH] Fix safe_create_leading_directories() for Windows

[Johannes Schindelin]

Hi Junio,

On Thu, 2 Jan 2014, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Thu, 2 Jan 2014, Sebastian Schuberth wrote:
> >
> >> See https://github.com/msysgit/git/pull/80.
> >
> > Thanks Sebastian!
> >
> > However, since the git.git project is not comfortable with the concept
> > of pull requests (which is why you submitted this patch via mail), I
> > believe that we have to explain the rationale in the commit message.
> > So here goes my attempt:
> 
> Thanks; the conclusion is correct --- you need a good commit message in
> the recorded history.  That does not have anything to do with
> integrating with pulling from subsystem maintainers, which we regularly
> do.

I should have pointed out that I was talking about the concept of pull
requests, not the concept of accepting pull requests from dedicated
subsystem maintainers.

> > On Linux, we can get away with assuming that the directory separator
> > is a forward slash, but that is wrong in general. For that purpose,
> > the is_dir_sep() function was introduced a long time ago. By using it
> > in safe_create_leading_directories(), we proof said function for use
> > on platforms where the directory separator is different from Linux'.
> 
> Perhaps with s|Linux|POSIX|,

No, for two reasons:

* OpenVMS supports POSIX, yes? And OpenVMS does not have the forward slash
  as directory separator but instead the dot, yes?

* it would be dishonest. The reason is not that we looked at the POSIX
  standard when we determined how to implement
  safe_create_leading_directories(), but at Linux.

> but more importantly, was there a specific error scenario that triggered
> this change?

Yes, the concrete use case that triggered the pull request whose change we
did not accept but instead would prefer Sebastian's patch is where a user
called

	git clone <URL> C:\directory

in cmd.exe.

Ciao,
Johannes

Re: [PATCH] Fix safe_create_leading_directories() for Windows

[Johannes Schindelin]

Hi Sebastian,

On Thu, 2 Jan 2014, Sebastian Schuberth wrote:

> On 02.01.2014 18:33, Johannes Schindelin wrote:
> 
> > -- snip --
> > On Linux, we can get away with assuming that the directory separator is a
> > forward slash, but that is wrong in general. For that purpose, the
> > is_dir_sep() function was introduced a long time ago. By using it in
> > safe_create_leading_directories(), we proof said function for use on
> > platforms where the directory separator is different from Linux'.
> > -- snap --
> 
> While I'd be fine with this, I do not think we really need it.

I also would have been fine with your commit message. But I knew Junio
wouldn't be.

> As you say, is_dir_sep() has been introduced a long time ago, so people
> should be aware of it, and it should also be immediately clear from the
> diff why using it is better than hard-coding '/'.
> 
> That said, I see any further explanations on top of the commit message
> title is an added bonus, and as "just" a bonus a link to a pull request
> should be fine. You don't need to understand or appreciate the concept
> of pull requests in order to follow the link and read the text in there.

Well, you and I both know how easy GitHub's pull request made things for
us as well as for contributors. I really cannot thank Erik enough for
bullying me into using and accepting them.

But the commit messages of the merged pull requests are not exactly
stellar when you want the repositories to be self-contained, which is
however how I remember things are expected in git.git.

Ciao,
Dscho

Re: [PATCH] Fix safe_create_leading_directories() for Windows

[Sebastian Schuberth]

On 02.01.2014 20:55, Junio C Hamano wrote:

> Thanks; the conclusion is correct --- you need a good commit
> message in the recorded history.  That does not have anything to do
> with integrating with pulling from subsystem maintainers, which we
> regularly do.

I'll send a v2 which adds a proper commits message inline.

> Perhaps with s|Linux|POSIX|, but more importantly, was there a
> specific error scenario that triggered this change?

Yes, cloning to a non-existent directory with Windows paths fails like:

fatal: could not create work tree dir 'c:\foo\bar'.: No such file or directory

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?

-- 
Sebastian Schuberth

[PATCH v2] Fix safe_create_leading_directories() for Windows

[Sebastian Schuberth]

When cloning to a directory "C:\foo\bar" from Windows' cmd.exe where "foo"
does not exist yet, Git would throw an error like

fatal: could not create work tree dir 'c:\foo\bar'.: No such file or directory

Fix this by not hard-coding a platform specific directory separator into
safe_create_leading_directories().

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
---
 sha1_file.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 760dd60..2114c58 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -110,12 +110,15 @@ int safe_create_leading_directories(char *path)
 	char *pos = path + offset_1st_component(path);
 	struct stat st;
 
-	while (pos) {
-		pos = strchr(pos, '/');
-		if (!pos)
-			break;
-		while (*++pos == '/')
-			;
+	while (*pos) {
+		while (!is_dir_sep(*pos)) {
+			++pos;
+			if (!*pos)
+				break;
+		}
+		/* skip consecutive directory separators */
+		while (is_dir_sep(*pos))
+			++pos;
 		if (!*pos)
 			break;
 		*--pos = '\0';
-- 
1.8.4.msysgit.0.1.ge705bba.dirty

Re: [PATCH] Fix safe_create_leading_directories() for Windows

[Junio C Hamano]

Sebastian Schuberth <sschuberth@gmail.com> writes:

> On 02.01.2014 20:55, Junio C Hamano wrote:
>
>> Thanks; the conclusion is correct --- you need a good commit
>> message in the recorded history.  That does not have anything to do
>> with integrating with pulling from subsystem maintainers, which we
>> regularly do.
>
> I'll send a v2 which adds a proper commits message inline.
>
>> Perhaps with s|Linux|POSIX|, but more importantly, was there a
>> specific error scenario that triggered this change?
>
> Yes, cloning to a non-existent directory with Windows paths fails like:
>
> fatal: could not create work tree dir 'c:\foo\bar'.: No such file or directory

OK.  That was why I wanted to see (and Dscho correctly guessed) a
good description in the proposed log message to see what problem the
change is trying to address, so that we can judge if the change is
tackling the right problem.

> 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
was meant to be used only on paths we internally generate, and the
paths we internally generate all are slash separated, in line with
how paths are stored in the index.

If we are going to change the meaning of the function so that it can
now take any random path in platform-specific convention that may be
incompatible with the internal notion of paths Git has (i.e. what is
passed to safe_create_leading_directories() may have to be massaged
into a slash-separated form before it can be used in the index and
parsed to be stuffed into trees), it is fine to do so as long as all
the codepaths understands the new world order, but my earlier "git
grep" hits did not tell me that such a change is warranted.

Thanks.

Re: [PATCH v2] Fix safe_create_leading_directories() for Windows

[Johannes Schindelin]

Hi,

On Thu, 2 Jan 2014, Sebastian Schuberth wrote:

> When cloning to a directory "C:\foo\bar" from Windows' cmd.exe where "foo"
> does not exist yet, Git would throw an error like
> 
> fatal: could not create work tree dir 'c:\foo\bar'.: No such file or directory
> 
> Fix this by not hard-coding a platform specific directory separator into
> safe_create_leading_directories().
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>

It would be nice to have links to the original discussion, but I guess
that that would not be accepted.

Ciao,
Dscho

Re: [PATCH] Fix safe_create_leading_directories() for Windows

[Johannes Schindelin]

Hi Junio,

On Thu, 2 Jan 2014, Junio C Hamano wrote:

> If we are going to change the meaning of the function so that it can
> now take any random path in platform-specific convention

Note that nothing in the function name or documentation suggests
otherwise.

> that may be incompatible with the internal notion of paths Git has (i.e.
> what is passed to safe_create_leading_directories() may have to be
> massaged into a slash-separated form before it can be used in the index

The safe_create_leading_directories() function never interacts with the
index, so you find me quite puzzled as to your objection.

> and parsed to be stuffed into trees), it is fine to do so as long as all
> the codepaths understands the new world order, but my earlier "git grep"
> hits did not tell me that such a change is warranted.

You call safe_create_leading_directories() with an argument that is
supposed to be the final path, correct? So what exactly is wrong with
safe_create_leading_directories() creating all the directories necessary
so that we can write to the path afterwards, *even* if that path is
interpreted in a platform-dependent manner (as one would actually expect
it to)?

Last time I checked we did not make a fuss about
safe_create_leading_directories() interpreting the argument in a
case-insensitive fashion on certain setups, either. So it is not exactly a
new thing that the paths are interpreted in a platform-dependent manner.

Ciao,
Johannes

Re: [PATCH v2] Fix safe_create_leading_directories() for Windows

[Junio C Hamano]

Sebastian Schuberth <sschuberth@gmail.com> writes:

> When cloning to a directory "C:\foo\bar" from Windows' cmd.exe where "foo"
> does not exist yet, Git would throw an error like
>
> fatal: could not create work tree dir 'c:\foo\bar'.: No such file or directory
>
> Fix this by not hard-coding a platform specific directory separator into
> safe_create_leading_directories().
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
> ---
>  sha1_file.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/sha1_file.c b/sha1_file.c
> index 760dd60..2114c58 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -110,12 +110,15 @@ int safe_create_leading_directories(char *path)
>  	char *pos = path + offset_1st_component(path);
>  	struct stat st;
>  
> -	while (pos) {
> -		pos = strchr(pos, '/');
> -		if (!pos)
> -			break;
> -		while (*++pos == '/')
> -			;
> +	while (*pos) {
> +		while (!is_dir_sep(*pos)) {
> +			++pos;
> +			if (!*pos)
> +				break;
> +		}
> +		/* skip consecutive directory separators */
> +		while (is_dir_sep(*pos))
> +			++pos;
>  		if (!*pos)
>  			break;
>  		*--pos = '\0';

This has a bit of conflict with another topic in flight; I think I
resolved it correctly, but please double check.  The following is
how it would apply on top of 'pu'.

 sha1_file.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 131ca97..9e686eb 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -113,11 +113,12 @@ int safe_create_leading_directories(char *path)
 
 	while (!retval && next_component) {
 		struct stat st;
-		char *slash = strchr(next_component, '/');
-
-		if (!slash)
+		char *slash = next_component;
+		while (!is_dir_sep(*slash))
+			++slash;
+		if (!*slash)
 			return 0;
-		while (*(slash + 1) == '/')
+		while (is_dir_sep(*(slash + 1)))
 			slash++;
 		next_component = slash + 1;
 		if (!*next_component)

Re: [PATCH v2] Fix safe_create_leading_directories() for Windows

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> This has a bit of conflict with another topic in flight; I think I
> resolved it correctly, but please double check.  The following is
> how it would apply on top of 'pu'.
>
>  sha1_file.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/sha1_file.c b/sha1_file.c
> index 131ca97..9e686eb 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -113,11 +113,12 @@ int safe_create_leading_directories(char *path)
>  
>  	while (!retval && next_component) {
>  		struct stat st;
> -		char *slash = strchr(next_component, '/');
> -
> -		if (!slash)
> +		char *slash = next_component;
> +		while (!is_dir_sep(*slash))

Gaah; we need to check for the end of string here, i.e.

		while (*slash && !is_dir_sep(*slash))

will be what I'll queue on 'pu' for today.

> +			++slash;
> +		if (!*slash)
>  			return 0;
> -		while (*(slash + 1) == '/')
> +		while (is_dir_sep(*(slash + 1)))
>  			slash++;
>  		next_component = slash + 1;
>  		if (!*next_component)

Re: [PATCH v2] Fix safe_create_leading_directories() for Windows

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> This has a bit of conflict with another topic in flight; I think I
>> resolved it correctly, but please double check.  The following is
>> how it would apply on top of 'pu'.
>>
>>  sha1_file.c | 9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/sha1_file.c b/sha1_file.c
>> index 131ca97..9e686eb 100644
>> --- a/sha1_file.c
>> +++ b/sha1_file.c
>> @@ -113,11 +113,12 @@ int safe_create_leading_directories(char *path)
>>  
>>  	while (!retval && next_component) {
>>  		struct stat st;
>> -		char *slash = strchr(next_component, '/');
>> -
>> -		if (!slash)
>> +		char *slash = next_component;
>> +		while (!is_dir_sep(*slash))
>
> Gaah; we need to check for the end of string here, i.e.
>
> 		while (*slash && !is_dir_sep(*slash))
>
> will be what I'll queue on 'pu' for today.
>
>> +			++slash;
>> +		if (!*slash)
>>  			return 0;
>> -		while (*(slash + 1) == '/')
>> +		while (is_dir_sep(*(slash + 1)))
>>  			slash++;
>>  		next_component = slash + 1;
>>  		if (!*next_component)

Another thing I noticed (but I won't fix it up myself today, as I am
deep into today's integration cycle already) is that we temporarily
replace the slash with NUL and then restore them before we return,
but the restoration is done with the slash.  If we were to go in the
direction of this patch, you may want to update that one to use
whatever dir-sep was used in the input string.

Re: [PATCH] Fix safe_create_leading_directories() for Windows

[Erik Faye-Lund]

On Thu, Jan 2, 2014 at 9:46 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Sebastian,
>
> On Thu, 2 Jan 2014, Sebastian Schuberth wrote:
>
>> On 02.01.2014 18:33, Johannes Schindelin wrote:
>>
>> > -- snip --
>> > On Linux, we can get away with assuming that the directory separator is a
>> > forward slash, but that is wrong in general. For that purpose, the
>> > is_dir_sep() function was introduced a long time ago. By using it in
>> > safe_create_leading_directories(), we proof said function for use on
>> > platforms where the directory separator is different from Linux'.
>> > -- snap --
>>
>> While I'd be fine with this, I do not think we really need it.
>
> I also would have been fine with your commit message. But I knew Junio
> wouldn't be.
>
>> As you say, is_dir_sep() has been introduced a long time ago, so people
>> should be aware of it, and it should also be immediately clear from the
>> diff why using it is better than hard-coding '/'.
>>
>> That said, I see any further explanations on top of the commit message
>> title is an added bonus, and as "just" a bonus a link to a pull request
>> should be fine. You don't need to understand or appreciate the concept
>> of pull requests in order to follow the link and read the text in there.
>
> Well, you and I both know how easy GitHub's pull request made things for
> us as well as for contributors. I really cannot thank Erik enough for
> bullying me into using and accepting them.

Huh? I don't think you refer to me, because I really dislike them (and
I always have IIRC).

Re: [PATCH] Fix safe_create_leading_directories() for Windows

[Johannes Schindelin]

Hi,

On Tue, 7 Jan 2014, Erik Faye-Lund wrote:

> On Thu, Jan 2, 2014 at 9:46 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>
> > Well, you and I both know how easy GitHub's pull request made things
> > for us as well as for contributors. I really cannot thank Erik enough
> > for bullying me into using and accepting them.
> 
> Huh? I don't think you refer to me, because I really dislike them (and I
> always have IIRC).

Ah yes, I misremembered. You were actually opposed to using them and I
thought we should be pragmatic to encourage contributions.

In any case, I do think that the contributions we got via pull requests
were in general contributions we would not otherwise have gotten.

Sorry for my mistake!
Dscho

Re: [PATCH] Fix safe_create_leading_directories() for Windows

[Sebastian Schuberth]

On Tue, Jan 7, 2014 at 6:56 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:

>> > Well, you and I both know how easy GitHub's pull request made things
>> > for us as well as for contributors. I really cannot thank Erik enough
>> > for bullying me into using and accepting them.
>>
>> Huh? I don't think you refer to me, because I really dislike them (and I
>> always have IIRC).
>
> Ah yes, I misremembered. You were actually opposed to using them and I
> thought we should be pragmatic to encourage contributions.

Not that it matters too much, but I guess it was me who talked Dscho
into moving to GitHub and using / accepting pull requests :-)

> In any case, I do think that the contributions we got via pull requests
> were in general contributions we would not otherwise have gotten.

I absolutely think so, too.

-- 
Sebastian Schuberth

Re: [PATCH] Fix safe_create_leading_directories() for Windows

[Sebastian Schuberth]

On Thu, Jan 2, 2014 at 10:19 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:

>> and parsed to be stuffed into trees), it is fine to do so as long as all
>> the codepaths understands the new world order, but my earlier "git grep"
>> hits did not tell me that such a change is warranted.
>
> You call safe_create_leading_directories() with an argument that is
> supposed to be the final path, correct? So what exactly is wrong with
> safe_create_leading_directories() creating all the directories necessary
> so that we can write to the path afterwards, *even* if that path is
> interpreted in a platform-dependent manner (as one would actually expect
> it to)?
>
> Last time I checked we did not make a fuss about
> safe_create_leading_directories() interpreting the argument in a
> case-insensitive fashion on certain setups, either. So it is not exactly a
> new thing that the paths are interpreted in a platform-dependent manner.

Sorry for the late reply. I just want to add that on top of that, I
would expect any function called *safe*_create_leading_directories()
in a cross-platform project like Git (yes, effectively Git has become
a cross-platform project by now) to gracefully handle
platform-specific input. In other words, I would expect the word
"safe" to be applied not only to the output (the creation of the
directory hierarchy with any missing parent directories) but also to
the input (the arguments to the function).

-- 
Sebastian Schuberth

Re: [PATCH] Fix safe_create_leading_directories() for Windows

[Sebastian Schuberth]

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.

-- 
Sebastian Schuberth

Re: [PATCH] Fix safe_create_leading_directories() for Windows

[Junio C Hamano]

Sebastian Schuberth <sschuberth@gmail.com> writes:

> 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).