Re: [PATCH] Fix safe_create_leading_directories() for Windows

2014-01-21 Thread Junio C Hamano 
Sebastian Schuberth sschube...@gmail.com writes:

 On Thu, Jan 2, 2014 at 10:08 PM, Junio C Hamano gits...@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).


--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Fix safe_create_leading_directories() for Windows

2014-01-18 Thread Sebastian Schuberth 
On Thu, Jan 2, 2014 at 10:19 PM, Johannes Schindelin
johannes.schinde...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Fix safe_create_leading_directories() for Windows

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

Re: [PATCH] Fix safe_create_leading_directories() for Windows

2014-01-07 Thread Erik Faye-Lund 
On Thu, Jan 2, 2014 at 9:46 PM, Johannes Schindelin
johannes.schinde...@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).
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Fix safe_create_leading_directories() for Windows

2014-01-07 Thread Johannes Schindelin 
Hi,

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

 On Thu, Jan 2, 2014 at 9:46 PM, Johannes Schindelin
 johannes.schinde...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Fix safe_create_leading_directories() for Windows

2014-01-07 Thread Sebastian Schuberth 
On Tue, Jan 7, 2014 at 6:56 PM, Johannes Schindelin
johannes.schinde...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] Fix safe_create_leading_directories() for Windows

2014-01-02 Thread Sebastian Schuberth 
See https://github.com/msysgit/git/pull/80.

Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
Signed-off-by: Sebastian Schuberth sschube...@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


--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Fix safe_create_leading_directories() for Windows

2014-01-02 Thread 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

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Fix safe_create_leading_directories() for Windows

2014-01-02 Thread 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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Fix safe_create_leading_directories() for Windows

2014-01-02 Thread Junio C Hamano 
Johannes Schindelin johannes.schinde...@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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Fix safe_create_leading_directories() for Windows

2014-01-02 Thread 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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Fix safe_create_leading_directories() for Windows

2014-01-02 Thread Johannes Schindelin 
Hi Junio,

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

 Johannes Schindelin johannes.schinde...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Fix safe_create_leading_directories() for Windows

2014-01-02 Thread Junio C Hamano 
Sebastian Schuberth sschube...@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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Fix safe_create_leading_directories() for Windows

2014-01-02 Thread 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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html