{"thread":{"id":"35596","subject":"[PATCH] Fix safe_create_leading_directories() for Windows","startedAt":"2014-01-02T17:22:01Z","lastAt":"2014-01-21T20:09:08Z","messageCount":22,"participants":["Sebastian Schuberth","Johannes Schindelin","John Keeping","Junio C Hamano","Erik Faye-Lund"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"232562","messageId":"52C5A039.6030408@gmail.com","threadId":"35596","inReplyTo":null,"subject":"[PATCH] Fix safe_create_leading_directories() for Windows","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2014-01-02T17:22:01Z","receivedAt":"2014-01-02T17:22:01Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"See https://github.com/msysgit/git/pull/80.\n\nSigned-off-by: Johannes Schindelin \nSigned-off-by: Sebastian Schuberth \n---\n sha1_file.c | 15 +++++++++------\n 1 file changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 760dd60..2114c58 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -110,12 +110,15 @@ int safe_create_leading_directories(char *path)\n \tchar *pos = path + offset_1st_component(path);\n \tstruct stat st;\n \n-\twhile (pos) {\n-\t\tpos = strchr(pos, '/');\n-\t\tif (!pos)\n-\t\t\tbreak;\n-\t\twhile (*++pos == '/')\n-\t\t\t;\n+\twhile (*pos) {\n+\t\twhile (!is_dir_sep(*pos)) {\n+\t\t\t++pos;\n+\t\t\tif (!*pos)\n+\t\t\t\tbreak;\n+\t\t}\n+\t\t/* skip consecutive directory separators */\n+\t\twhile (is_dir_sep(*pos))\n+\t\t\t++pos;\n \t\tif (!*pos)\n \t\t\tbreak;\n \t\t*--pos = '\\0';\n-- \n1.8.4.msysgit.0.1.ge705bba.dirty\n"},{"id":"232563","messageId":"alpine.DEB.1.00.1401021826120.1191@s15462909.onlinehome-server.info","threadId":"35596","inReplyTo":"52C5A039.6030408@gmail.com","subject":"Re: [PATCH] Fix safe_create_leading_directories() for Windows","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-01-02T17:33:31Z","receivedAt":"2014-01-02T17:33:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 2 Jan 2014, Sebastian Schuberth wrote:\n\n> See https://github.com/msysgit/git/pull/80.\n\nThanks Sebastian!\n\nHowever, since the git.git project is not comfortable with the concept of\npull requests (which is why you submitted this patch via mail), I believe\nthat we have to explain the rationale in the commit message. So here goes\nmy attempt:\n\n-- snip --\nOn Linux, we can get away with assuming that the directory separator is a\nforward slash, but that is wrong in general. For that purpose, the\nis_dir_sep() function was introduced a long time ago. By using it in\nsafe_create_leading_directories(), we proof said function for use on\nplatforms where the directory separator is different from Linux'.\n-- snap --\n\nHmm?\nDscho\n"},{"id":"232566","messageId":"52C5ABDE.1010006@gmail.com","threadId":"35596","inReplyTo":"alpine.DEB.1.00.1401021826120.1191@s15462909.onlinehome-server.info","subject":"Re: [PATCH] Fix safe_create_leading_directories() for Windows","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2014-01-02T18:11:42Z","receivedAt":"2014-01-02T18:11:42Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On 02.01.2014 18:33, Johannes Schindelin wrote:\n\n> -- snip --\n> On Linux, we can get away with assuming that the directory separator is a\n> forward slash, but that is wrong in general. For that purpose, the\n> is_dir_sep() function was introduced a long time ago. By using it in\n> safe_create_leading_directories(), we proof said function for use on\n> platforms where the directory separator is different from Linux'.\n> -- snap --\n\nWhile I'd be fine with this, I do not think we really need it. As you \nsay, is_dir_sep() has been introduced a long time ago, so people should \nbe aware of it, and it should also be immediately clear from the diff \nwhy using it is better than hard-coding '/'.\n\nThat said, I see any further explanations on top of the commit message \ntitle is an added bonus, and as \"just\" a bonus a link to a pull request \nshould be fine. You don't need to understand or appreciate the concept \nof pull requests in order to follow the link and read the text in there.\n\n-- \nSebastian Schuberth\n"},{"id":"232567","messageId":"20140102181857.GG7608@serenity.lan","threadId":"35596","inReplyTo":"52C5ABDE.1010006@gmail.com","subject":"Re: [PATCH] Fix safe_create_leading_directories() for Windows","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2014-01-02T18:18:57Z","receivedAt":"2014-01-02T18:18:57Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Thu, Jan 02, 2014 at 07:11:42PM +0100, Sebastian Schuberth wrote:\n> On 02.01.2014 18:33, Johannes Schindelin wrote:\n> \n> > -- snip --\n> > On Linux, we can get away with assuming that the directory separator is a\n> > forward slash, but that is wrong in general. For that purpose, the\n> > is_dir_sep() function was introduced a long time ago. By using it in\n> > safe_create_leading_directories(), we proof said function for use on\n> > platforms where the directory separator is different from Linux'.\n> > -- snap --\n> \n> While I'd be fine with this, I do not think we really need it. As you \n> say, is_dir_sep() has been introduced a long time ago, so people should \n> be aware of it, and it should also be immediately clear from the diff \n> why using it is better than hard-coding '/'.\n> \n> That said, I see any further explanations on top of the commit message \n> title is an added bonus, and as \"just\" a bonus a link to a pull request \n> should be fine. You don't need to understand or appreciate the concept \n> of pull requests in order to follow the link and read the text in there.\n\nThe commit message serves as an historical record of why a change was\nmade; depending on an external service to provide this information when\nit can quite easily be included in the commit itself lessens the value\nof the commit message.\n\nIf you look at other commits in git.git you will see that there is a\nstrong preference for summarising the discussion and rationale for a\ncommit in its message.\n"},{"id":"232572","messageId":"xmqqtxdmp39a.fsf@gitster.dls.corp.google.com","threadId":"35596","inReplyTo":"alpine.DEB.1.00.1401021826120.1191@s15462909.onlinehome-server.info","subject":"Re: [PATCH] Fix safe_create_leading_directories() for Windows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-02T19:55:45Z","receivedAt":"2014-01-02T19:55:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Hi,\n>\n> On Thu, 2 Jan 2014, Sebastian Schuberth wrote:\n>\n>> See https://github.com/msysgit/git/pull/80.\n>\n> Thanks Sebastian!\n>\n> However, since the git.git project is not comfortable with the concept of\n> pull requests (which is why you submitted this patch via mail), I believe\n> that we have to explain the rationale in the commit message. So here goes\n> my attempt:\n\nThanks; the conclusion is correct --- you need a good commit\nmessage in the recorded history. That does not have anything to do\nwith integrating with pulling from subsystem maintainers, which we\nregularly do.\n\n> On Linux, we can get away with assuming that the directory separator is a\n> forward slash, but that is wrong in general. For that purpose, the\n> is_dir_sep() function was introduced a long time ago. By using it in\n> safe_create_leading_directories(), we proof said function for use on\n> platforms where the directory separator is different from Linux'.\n\nPerhaps with s|Linux|POSIX|, but more importantly, was there a\nspecific error scenario that triggered this change?\n\nMy quick reading of \"git grep\" suggests that the callsites of this\nfunction all assume that they are to use slashes as directory\nseparators, and it may be that it is a bug in the specific caller\nthat throws a backslash-separated paths to it.\n"},{"id":"232576","messageId":"52C5CE13.2020506@gmail.com","threadId":"35596","inReplyTo":"20140102181857.GG7608@serenity.lan","subject":"Re: [PATCH] Fix safe_create_leading_directories() for Windows","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2014-01-02T20:37:39Z","receivedAt":"2014-01-02T20:37:39Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On 02.01.2014 19:18, John Keeping wrote:\n\n>> That said, I see any further explanations on top of the commit message\n>> title is an added bonus, and as \"just\" a bonus a link to a pull request\n>> should be fine. You don't need to understand or appreciate the concept\n>> of pull requests in order to follow the link and read the text in there.\n>\n> The commit message serves as an historical record of why a change was\n> made; depending on an external service to provide this information when\n> it can quite easily be included in the commit itself lessens the value\n> of the commit message.\n\nSure. My point just was that IMHO the commit does not *depend* on the \ninformation provided in the link; for me the commit was simply \nself-evident, and I just added link as optional information, not to \nreplace any inline text that I would have written otherwise.\n\n-- \nSebastian Schuberth\n"},{"id":"232577","messageId":"alpine.DEB.1.00.1401022135030.1191@s15462909.onlinehome-server.info","threadId":"35596","inReplyTo":"xmqqtxdmp39a.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] Fix safe_create_leading_directories() for Windows","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-01-02T20:42:29Z","receivedAt":"2014-01-02T20:42:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 2 Jan 2014, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > On Thu, 2 Jan 2014, Sebastian Schuberth wrote:\n> >\n> >> See https://github.com/msysgit/git/pull/80.\n> >\n> > Thanks Sebastian!\n> >\n> > However, since the git.git project is not comfortable with the concept\n> > of pull requests (which is why you submitted this patch via mail), I\n> > believe that we have to explain the rationale in the commit message.\n> > So here goes my attempt:\n> \n> Thanks; the conclusion is correct --- you need a good commit message in\n> the recorded history. That does not have anything to do with\n> integrating with pulling from subsystem maintainers, which we regularly\n> do.\n\nI should have pointed out that I was talking about the concept of pull\nrequests, not the concept of accepting pull requests from dedicated\nsubsystem maintainers.\n\n> > On Linux, we can get away with assuming that the directory separator\n> > is a forward slash, but that is wrong in general. For that purpose,\n> > the is_dir_sep() function was introduced a long time ago. By using it\n> > in safe_create_leading_directories(), we proof said function for use\n> > on platforms where the directory separator is different from Linux'.\n> \n> Perhaps with s|Linux|POSIX|,\n\nNo, for two reasons:\n\n* OpenVMS supports POSIX, yes? And OpenVMS does not have the forward slash\n as directory separator but instead the dot, yes?\n\n* it would be dishonest. The reason is not that we looked at the POSIX\n standard when we determined how to implement\n safe_create_leading_directories(), but at Linux.\n\n> but more importantly, was there a specific error scenario that triggered\n> this change?\n\nYes, the concrete use case that triggered the pull request whose change we\ndid not accept but instead would prefer Sebastian's patch is where a user\ncalled\n\n\tgit clone C:\\directory\n\nin cmd.exe.\n\nCiao,\nJohannes\n"},{"id":"232578","messageId":"alpine.DEB.1.00.1401022143310.1191@s15462909.onlinehome-server.info","threadId":"35596","inReplyTo":"52C5ABDE.1010006@gmail.com","subject":"Re: [PATCH] Fix safe_create_leading_directories() for Windows","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-01-02T20:46:38Z","receivedAt":"2014-01-02T20:46:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Sebastian,\n\nOn Thu, 2 Jan 2014, Sebastian Schuberth wrote:\n\n> On 02.01.2014 18:33, Johannes Schindelin wrote:\n> \n> > -- snip --\n> > On Linux, we can get away with assuming that the directory separator is a\n> > forward slash, but that is wrong in general. For that purpose, the\n> > is_dir_sep() function was introduced a long time ago. By using it in\n> > safe_create_leading_directories(), we proof said function for use on\n> > platforms where the directory separator is different from Linux'.\n> > -- snap --\n> \n> While I'd be fine with this, I do not think we really need it.\n\nI also would have been fine with your commit message. But I knew Junio\nwouldn't be.\n\n> As you say, is_dir_sep() has been introduced a long time ago, so people\n> should be aware of it, and it should also be immediately clear from the\n> diff why using it is better than hard-coding '/'.\n> \n> That said, I see any further explanations on top of the commit message\n> title is an added bonus, and as \"just\" a bonus a link to a pull request\n> should be fine. You don't need to understand or appreciate the concept\n> of pull requests in order to follow the link and read the text in there.\n\nWell, you and I both know how easy GitHub's pull request made things for\nus as well as for contributors. I really cannot thank Erik enough for\nbullying me into using and accepting them.\n\nBut the commit messages of the merged pull requests are not exactly\nstellar when you want the repositories to be self-contained, which is\nhowever how I remember things are expected in git.git.\n\nCiao,\nDscho\n"},{"id":"232580","messageId":"52C5D0AB.7050309@gmail.com","threadId":"35596","inReplyTo":"xmqqtxdmp39a.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] Fix safe_create_leading_directories() for Windows","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2014-01-02T20:48:43Z","receivedAt":"2014-01-02T20:48:43Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On 02.01.2014 20:55, Junio C Hamano wrote:\n\n> Thanks; the conclusion is correct --- you need a good commit\n> message in the recorded history. That does not have anything to do\n> with integrating with pulling from subsystem maintainers, which we\n> regularly do.\n\nI'll send a v2 which adds a proper commits message inline.\n\n> Perhaps with s|Linux|POSIX|, but more importantly, was there a\n> specific error scenario that triggered this change?\n\nYes, cloning to a non-existent directory with Windows paths fails like:\n\nfatal: could not create work tree dir 'c:\\foo\\bar'.: No such file or directory\n\nSeems 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?\n\n-- \nSebastian Schuberth\n"},{"id":"232581","messageId":"52C5D201.5070008@gmail.com","threadId":"35596","inReplyTo":"52C5D0AB.7050309@gmail.com","subject":"[PATCH v2] Fix safe_create_leading_directories() for Windows","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2014-01-02T20:54:25Z","receivedAt":"2014-01-02T20:54:25Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"When cloning to a directory \"C:\\foo\\bar\" from Windows' cmd.exe where \"foo\"\ndoes not exist yet, Git would throw an error like\n\nfatal: could not create work tree dir 'c:\\foo\\bar'.: No such file or directory\n\nFix this by not hard-coding a platform specific directory separator into\nsafe_create_leading_directories().\n\nSigned-off-by: Johannes Schindelin \nSigned-off-by: Sebastian Schuberth \n---\n sha1_file.c | 15 +++++++++------\n 1 file changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 760dd60..2114c58 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -110,12 +110,15 @@ int safe_create_leading_directories(char *path)\n \tchar *pos = path + offset_1st_component(path);\n \tstruct stat st;\n \n-\twhile (pos) {\n-\t\tpos = strchr(pos, '/');\n-\t\tif (!pos)\n-\t\t\tbreak;\n-\t\twhile (*++pos == '/')\n-\t\t\t;\n+\twhile (*pos) {\n+\t\twhile (!is_dir_sep(*pos)) {\n+\t\t\t++pos;\n+\t\t\tif (!*pos)\n+\t\t\t\tbreak;\n+\t\t}\n+\t\t/* skip consecutive directory separators */\n+\t\twhile (is_dir_sep(*pos))\n+\t\t\t++pos;\n \t\tif (!*pos)\n \t\t\tbreak;\n \t\t*--pos = '\\0';\n-- \n1.8.4.msysgit.0.1.ge705bba.dirty\n"},{"id":"232584","messageId":"xmqqha9mozvc.fsf@gitster.dls.corp.google.com","threadId":"35596","inReplyTo":"52C5D0AB.7050309@gmail.com","subject":"Re: [PATCH] Fix safe_create_leading_directories() for Windows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-02T21:08:55Z","receivedAt":"2014-01-02T21:08:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sebastian Schuberth writes:\n\n> On 02.01.2014 20:55, Junio C Hamano wrote:\n>\n>> Thanks; the conclusion is correct --- you need a good commit\n>> message in the recorded history. That does not have anything to do\n>> with integrating with pulling from subsystem maintainers, which we\n>> regularly do.\n>\n> I'll send a v2 which adds a proper commits message inline.\n>\n>> Perhaps with s|Linux|POSIX|, but more importantly, was there a\n>> specific error scenario that triggered this change?\n>\n> Yes, cloning to a non-existent directory with Windows paths fails like:\n>\n> fatal: could not create work tree dir 'c:\\foo\\bar'.: No such file or directory\n\nOK. That was why I wanted to see (and Dscho correctly guessed) a\ngood description in the proposed log message to see what problem the\nchange is trying to address, so that we can judge if the change is\ntackling the right problem.\n\n> Seems like the path to clone to is taken as-is from argv in\n> cmd_clone(). So maybe another solution would be to replace all\n> backslashes with forward slashes already there?\n\nThat sounds like a workable alternative, and it might even be a\npreferable solution but if and only if clone's use of the function\nto create paths that lead to a new working tree location is the only\n(ab)use of the function. That was what I meant when I said \"it may\nbe that it is a bug in the specific caller\". AFAIK, the function\nwas meant to be used only on paths we internally generate, and the\npaths we internally generate all are slash separated, in line with\nhow paths are stored in the index.\n\nIf we are going to change the meaning of the function so that it can\nnow take any random path in platform-specific convention that may be\nincompatible with the internal notion of paths Git has (i.e. what is\npassed to safe_create_leading_directories() may have to be massaged\ninto a slash-separated form before it can be used in the index and\nparsed to be stuffed into trees), it is fine to do so as long as all\nthe codepaths understands the new world order, but my earlier \"git\ngrep\" hits did not tell me that such a change is warranted.\n\nThanks.\n"},{"id":"232585","messageId":"alpine.DEB.1.00.1401022209310.1191@s15462909.onlinehome-server.info","threadId":"35596","inReplyTo":"52C5D201.5070008@gmail.com","subject":"Re: [PATCH v2] Fix safe_create_leading_directories() for Windows","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-01-02T21:10:06Z","receivedAt":"2014-01-02T21:10:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 2 Jan 2014, Sebastian Schuberth wrote:\n\n> When cloning to a directory \"C:\\foo\\bar\" from Windows' cmd.exe where \"foo\"\n> does not exist yet, Git would throw an error like\n> \n> fatal: could not create work tree dir 'c:\\foo\\bar'.: No such file or directory\n> \n> Fix this by not hard-coding a platform specific directory separator into\n> safe_create_leading_directories().\n> \n> Signed-off-by: Johannes Schindelin \n> Signed-off-by: Sebastian Schuberth \n\nIt would be nice to have links to the original discussion, but I guess\nthat that would not be accepted.\n\nCiao,\nDscho\n"},{"id":"232588","messageId":"alpine.DEB.1.00.1401022212150.1191@s15462909.onlinehome-server.info","threadId":"35596","inReplyTo":"xmqqha9mozvc.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] Fix safe_create_leading_directories() for Windows","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-01-02T21:19:41Z","receivedAt":"2014-01-02T21:19:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 2 Jan 2014, Junio C Hamano wrote:\n\n> If we are going to change the meaning of the function so that it can\n> now take any random path in platform-specific convention\n\nNote that nothing in the function name or documentation suggests\notherwise.\n\n> that may be incompatible with the internal notion of paths Git has (i.e.\n> what is passed to safe_create_leading_directories() may have to be\n> massaged into a slash-separated form before it can be used in the index\n\nThe safe_create_leading_directories() function never interacts with the\nindex, so you find me quite puzzled as to your objection.\n\n> and parsed to be stuffed into trees), it is fine to do so as long as all\n> the codepaths understands the new world order, but my earlier \"git grep\"\n> hits did not tell me that such a change is warranted.\n\nYou call safe_create_leading_directories() with an argument that is\nsupposed to be the final path, correct? So what exactly is wrong with\nsafe_create_leading_directories() creating all the directories necessary\nso that we can write to the path afterwards, *even* if that path is\ninterpreted in a platform-dependent manner (as one would actually expect\nit to)?\n\nLast time I checked we did not make a fuss about\nsafe_create_leading_directories() interpreting the argument in a\ncase-insensitive fashion on certain setups, either. So it is not exactly a\nnew thing that the paths are interpreted in a platform-dependent manner.\n\nCiao,\nJohannes\n"},{"id":"232589","messageId":"xmqq8uuyoz5g.fsf@gitster.dls.corp.google.com","threadId":"35596","inReplyTo":"52C5D201.5070008@gmail.com","subject":"Re: [PATCH v2] Fix safe_create_leading_directories() for Windows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-02T21:24:27Z","receivedAt":"2014-01-02T21:24:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sebastian Schuberth writes:\n\n> When cloning to a directory \"C:\\foo\\bar\" from Windows' cmd.exe where \"foo\"\n> does not exist yet, Git would throw an error like\n>\n> fatal: could not create work tree dir 'c:\\foo\\bar'.: No such file or directory\n>\n> Fix this by not hard-coding a platform specific directory separator into\n> safe_create_leading_directories().\n>\n> Signed-off-by: Johannes Schindelin \n> Signed-off-by: Sebastian Schuberth \n> ---\n> sha1_file.c | 15 +++++++++------\n> 1 file changed, 9 insertions(+), 6 deletions(-)\n>\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 760dd60..2114c58 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -110,12 +110,15 @@ int safe_create_leading_directories(char *path)\n> \tchar *pos = path + offset_1st_component(path);\n> \tstruct stat st;\n> \n> -\twhile (pos) {\n> -\t\tpos = strchr(pos, '/');\n> -\t\tif (!pos)\n> -\t\t\tbreak;\n> -\t\twhile (*++pos == '/')\n> -\t\t\t;\n> +\twhile (*pos) {\n> +\t\twhile (!is_dir_sep(*pos)) {\n> +\t\t\t++pos;\n> +\t\t\tif (!*pos)\n> +\t\t\t\tbreak;\n> +\t\t}\n> +\t\t/* skip consecutive directory separators */\n> +\t\twhile (is_dir_sep(*pos))\n> +\t\t\t++pos;\n> \t\tif (!*pos)\n> \t\t\tbreak;\n> \t\t*--pos = '\\0';\n\nThis has a bit of conflict with another topic in flight; I think I\nresolved it correctly, but please double check. The following is\nhow it would apply on top of 'pu'.\n\n sha1_file.c | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 131ca97..9e686eb 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -113,11 +113,12 @@ int safe_create_leading_directories(char *path)\n \n \twhile (!retval && next_component) {\n \t\tstruct stat st;\n-\t\tchar *slash = strchr(next_component, '/');\n-\n-\t\tif (!slash)\n+\t\tchar *slash = next_component;\n+\t\twhile (!is_dir_sep(*slash))\n+\t\t\t++slash;\n+\t\tif (!*slash)\n \t\t\treturn 0;\n-\t\twhile (*(slash + 1) == '/')\n+\t\twhile (is_dir_sep(*(slash + 1)))\n \t\t\tslash++;\n \t\tnext_component = slash + 1;\n \t\tif (!*next_component)\n"},{"id":"232592","messageId":"xmqqzjnenjcd.fsf@gitster.dls.corp.google.com","threadId":"35596","inReplyTo":"xmqq8uuyoz5g.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2] Fix safe_create_leading_directories() for Windows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-02T21:51:14Z","receivedAt":"2014-01-02T21:51:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> This has a bit of conflict with another topic in flight; I think I\n> resolved it correctly, but please double check. The following is\n> how it would apply on top of 'pu'.\n>\n> sha1_file.c | 9 +++++----\n> 1 file changed, 5 insertions(+), 4 deletions(-)\n>\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 131ca97..9e686eb 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -113,11 +113,12 @@ int safe_create_leading_directories(char *path)\n> \n> \twhile (!retval && next_component) {\n> \t\tstruct stat st;\n> -\t\tchar *slash = strchr(next_component, '/');\n> -\n> -\t\tif (!slash)\n> +\t\tchar *slash = next_component;\n> +\t\twhile (!is_dir_sep(*slash))\n\nGaah; we need to check for the end of string here, i.e.\n\n\t\twhile (*slash && !is_dir_sep(*slash))\n\nwill be what I'll queue on 'pu' for today.\n\n> +\t\t\t++slash;\n> +\t\tif (!*slash)\n> \t\t\treturn 0;\n> -\t\twhile (*(slash + 1) == '/')\n> +\t\twhile (is_dir_sep(*(slash + 1)))\n> \t\t\tslash++;\n> \t\tnext_component = slash + 1;\n> \t\tif (!*next_component)\n"},{"id":"232594","messageId":"xmqqsit6nid3.fsf@gitster.dls.corp.google.com","threadId":"35596","inReplyTo":"xmqqzjnenjcd.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2] Fix safe_create_leading_directories() for Windows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-02T22:12:24Z","receivedAt":"2014-01-02T22:12:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Junio C Hamano writes:\n>\n>> This has a bit of conflict with another topic in flight; I think I\n>> resolved it correctly, but please double check. The following is\n>> how it would apply on top of 'pu'.\n>>\n>> sha1_file.c | 9 +++++----\n>> 1 file changed, 5 insertions(+), 4 deletions(-)\n>>\n>> diff --git a/sha1_file.c b/sha1_file.c\n>> index 131ca97..9e686eb 100644\n>> --- a/sha1_file.c\n>> +++ b/sha1_file.c\n>> @@ -113,11 +113,12 @@ int safe_create_leading_directories(char *path)\n>> \n>> \twhile (!retval && next_component) {\n>> \t\tstruct stat st;\n>> -\t\tchar *slash = strchr(next_component, '/');\n>> -\n>> -\t\tif (!slash)\n>> +\t\tchar *slash = next_component;\n>> +\t\twhile (!is_dir_sep(*slash))\n>\n> Gaah; we need to check for the end of string here, i.e.\n>\n> \t\twhile (*slash && !is_dir_sep(*slash))\n>\n> will be what I'll queue on 'pu' for today.\n>\n>> +\t\t\t++slash;\n>> +\t\tif (!*slash)\n>> \t\t\treturn 0;\n>> -\t\twhile (*(slash + 1) == '/')\n>> +\t\twhile (is_dir_sep(*(slash + 1)))\n>> \t\t\tslash++;\n>> \t\tnext_component = slash + 1;\n>> \t\tif (!*next_component)\n\nAnother thing I noticed (but I won't fix it up myself today, as I am\ndeep into today's integration cycle already) is that we temporarily\nreplace the slash with NUL and then restore them before we return,\nbut the restoration is done with the slash. If we were to go in the\ndirection of this patch, you may want to update that one to use\nwhatever dir-sep was used in the input string.\n"},{"id":"232807","messageId":"CABPQNSaBbcgpE98j7mChvu6N7QqCG0CEmUgvJRURXLTdUTyFGg@mail.gmail.com","threadId":"35596","inReplyTo":"alpine.DEB.1.00.1401022143310.1191@s15462909.onlinehome-server.info","subject":"Re: [PATCH] Fix safe_create_leading_directories() for Windows","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2014-01-07T15:43:22Z","receivedAt":"2014-01-07T15:43:22Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Thu, Jan 2, 2014 at 9:46 PM, Johannes Schindelin\n wrote:\n> Hi Sebastian,\n>\n> On Thu, 2 Jan 2014, Sebastian Schuberth wrote:\n>\n>> On 02.01.2014 18:33, Johannes Schindelin wrote:\n>>\n>> > -- snip --\n>> > On Linux, we can get away with assuming that the directory separator is a\n>> > forward slash, but that is wrong in general. For that purpose, the\n>> > is_dir_sep() function was introduced a long time ago. By using it in\n>> > safe_create_leading_directories(), we proof said function for use on\n>> > platforms where the directory separator is different from Linux'.\n>> > -- snap --\n>>\n>> While I'd be fine with this, I do not think we really need it.\n>\n> I also would have been fine with your commit message. But I knew Junio\n> wouldn't be.\n>\n>> As you say, is_dir_sep() has been introduced a long time ago, so people\n>> should be aware of it, and it should also be immediately clear from the\n>> diff why using it is better than hard-coding '/'.\n>>\n>> That said, I see any further explanations on top of the commit message\n>> title is an added bonus, and as \"just\" a bonus a link to a pull request\n>> should be fine. You don't need to understand or appreciate the concept\n>> of pull requests in order to follow the link and read the text in there.\n>\n> Well, you and I both know how easy GitHub's pull request made things for\n> us as well as for contributors. I really cannot thank Erik enough for\n> bullying me into using and accepting them.\n\nHuh? I don't think you refer to me, because I really dislike them (and\nI always have IIRC).\n"},{"id":"232821","messageId":"alpine.DEB.1.00.1401071854330.20365@s15462909.onlinehome-server.info","threadId":"35596","inReplyTo":"CABPQNSaBbcgpE98j7mChvu6N7QqCG0CEmUgvJRURXLTdUTyFGg@mail.gmail.com","subject":"Re: [PATCH] Fix safe_create_leading_directories() for Windows","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-01-07T17:56:14Z","receivedAt":"2014-01-07T17:56:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 7 Jan 2014, Erik Faye-Lund wrote:\n\n> On Thu, Jan 2, 2014 at 9:46 PM, Johannes Schindelin\n> wrote:\n>\n> > Well, you and I both know how easy GitHub's pull request made things\n> > for us as well as for contributors. I really cannot thank Erik enough\n> > for bullying me into using and accepting them.\n> \n> Huh? I don't think you refer to me, because I really dislike them (and I\n> always have IIRC).\n\nAh yes, I misremembered. You were actually opposed to using them and I\nthought we should be pragmatic to encourage contributions.\n\nIn any case, I do think that the contributions we got via pull requests\nwere in general contributions we would not otherwise have gotten.\n\nSorry for my mistake!\nDscho\n"},{"id":"232865","messageId":"CAHGBnuND3YEuw7TacH24PB2XP=G3eRniG2n9DEjja5gi9RedBw@mail.gmail.com","threadId":"35596","inReplyTo":"alpine.DEB.1.00.1401071854330.20365@s15462909.onlinehome-server.info","subject":"Re: [PATCH] Fix safe_create_leading_directories() for Windows","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2014-01-07T21:49:56Z","receivedAt":"2014-01-07T21:49:56Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Tue, Jan 7, 2014 at 6:56 PM, Johannes Schindelin\n wrote:\n\n>> > Well, you and I both know how easy GitHub's pull request made things\n>> > for us as well as for contributors. I really cannot thank Erik enough\n>> > for bullying me into using and accepting them.\n>>\n>> Huh? I don't think you refer to me, because I really dislike them (and I\n>> always have IIRC).\n>\n> Ah yes, I misremembered. You were actually opposed to using them and I\n> thought we should be pragmatic to encourage contributions.\n\nNot that it matters too much, but I guess it was me who talked Dscho\ninto moving to GitHub and using / accepting pull requests :-)\n\n> In any case, I do think that the contributions we got via pull requests\n> were in general contributions we would not otherwise have gotten.\n\nI absolutely think so, too.\n\n-- \nSebastian Schuberth\n"},{"id":"233361","messageId":"CAHGBnuMDyxapRdfRN2PuHE5ULeGUft7J-EroXcxjpzzn3Kpi7Q@mail.gmail.com","threadId":"35596","inReplyTo":"alpine.DEB.1.00.1401022212150.1191@s15462909.onlinehome-server.info","subject":"Re: [PATCH] Fix safe_create_leading_directories() for Windows","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2014-01-19T07:00:02Z","receivedAt":"2014-01-19T07:00:02Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Thu, Jan 2, 2014 at 10:19 PM, Johannes Schindelin\n wrote:\n\n>> and parsed to be stuffed into trees), it is fine to do so as long as all\n>> the codepaths understands the new world order, but my earlier \"git grep\"\n>> hits did not tell me that such a change is warranted.\n>\n> You call safe_create_leading_directories() with an argument that is\n> supposed to be the final path, correct? So what exactly is wrong with\n> safe_create_leading_directories() creating all the directories necessary\n> so that we can write to the path afterwards, *even* if that path is\n> interpreted in a platform-dependent manner (as one would actually expect\n> it to)?\n>\n> Last time I checked we did not make a fuss about\n> safe_create_leading_directories() interpreting the argument in a\n> case-insensitive fashion on certain setups, either. So it is not exactly a\n> new thing that the paths are interpreted in a platform-dependent manner.\n\nSorry for the late reply. I just want to add that on top of that, I\nwould expect any function called *safe*_create_leading_directories()\nin a cross-platform project like Git (yes, effectively Git has become\na cross-platform project by now) to gracefully handle\nplatform-specific input. In other words, I would expect the word\n\"safe\" to be applied not only to the output (the creation of the\ndirectory hierarchy with any missing parent directories) but also to\nthe input (the arguments to the function).\n\n-- \nSebastian Schuberth\n"},{"id":"233363","messageId":"CAHGBnuNoTrRwnjp7ZqMgveLHZeV68cxOqawf7nWo7gnAAYfSOw@mail.gmail.com","threadId":"35596","inReplyTo":"xmqqha9mozvc.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] Fix safe_create_leading_directories() for Windows","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2014-01-19T07:26:33Z","receivedAt":"2014-01-19T07:26:33Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Thu, Jan 2, 2014 at 10:08 PM, Junio C Hamano wrote:\n\n>> Seems like the path to clone to is taken as-is from argv in\n>> cmd_clone(). So maybe another solution would be to replace all\n>> backslashes with forward slashes already there?\n>\n> That sounds like a workable alternative, and it might even be a\n> preferable solution but if and only if clone's use of the function\n> to create paths that lead to a new working tree location is the only\n> (ab)use of the function. That was what I meant when I said \"it may\n> be that it is a bug in the specific caller\". AFAIK, the function\n\nI think Dscho made valid points in his other mail that the better\nsolution still is to make safe_create_leading_directories() actually\nsafe, also regarding its arguments.\n\n-- \nSebastian Schuberth\n"},{"id":"233457","messageId":"xmqqeh41umfv.fsf@gitster.dls.corp.google.com","threadId":"35596","inReplyTo":"CAHGBnuNoTrRwnjp7ZqMgveLHZeV68cxOqawf7nWo7gnAAYfSOw@mail.gmail.com","subject":"Re: [PATCH] Fix safe_create_leading_directories() for Windows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-21T20:09:08Z","receivedAt":"2014-01-21T20:09:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sebastian Schuberth writes:\n\n> On Thu, Jan 2, 2014 at 10:08 PM, Junio C Hamano wrote:\n>\n>>> Seems like the path to clone to is taken as-is from argv in\n>>> cmd_clone(). So maybe another solution would be to replace all\n>>> backslashes with forward slashes already there?\n>>\n>> That sounds like a workable alternative, and it might even be a\n>> preferable solution but if and only if clone's use of the function\n>> to create paths that lead to a new working tree location is the only\n>> (ab)use of the function. That was what I meant when I said \"it may\n>> be that it is a bug in the specific caller\". AFAIK, the function\n>\n> I think Dscho made valid points in his other mail that the better\n> solution still is to make safe_create_leading_directories() actually\n> safe, also regarding its arguments.\n\nSorry, but I think I misremebered the reason why we have that\nfunction.\n\nThere are two reasons we may need to do a rough equivalent of \n\n\tsystem(\"mkdir -p $(dirname a/b/c)\")\n\ngiven an argument 'a/b/c' in our codebase.\n\n 1. We would want to be able to create 'c' such that we can later\n refer to \"a/b/c\" to retrieve what we wrote there, so we need to\n make sure that \"a/b/\" refers to a writable directory.\n\n 2. We were told by the index (or a caller that will then later\n update the index in a consistent way) that 'a/b/c' must exist in\n the working tree, so we need to make sure that \"a/\" and \"a/b/\"\n are both directories (not a symlink to a directory elsewhere).\n\nSeeing hits \"git grep safe_create_leading_directories\" from apply\nand merge-recursive led me to incorrectly assume that this function\nwas meant to do the latter [*1*].\n\nBut the original purpose of safe_create_leading_directories() in\nsha1_file.c was to \"mkdir -p\" inside .git/ directories when writing\nto refs e.g. \"refs/heads/foo/bar\", and for this kind of use, we must\nhonor existing symlinks. \".git/refs\" may be a symbolic link in a\nworking tree managed by \"new-workdir\" to the real repository, and we\ndo not want to unlink && mkdir it.\n\nWe even have an \"offset-1st-component\" call in the function, which\nis a clear sign that we would want this as usable for the full path\nin the filesystem, which is an indication that this should not be\nused to create paths in the working tree.\n\nSo I think it is the right thing to do to allow the function accept\nplatform specific path here, and it is OK for \"clone\" to call it to\ncreate the path leading to the location of the new repository.\n\nA related tangent is that somebody needs to audit the callers to\nmake sure this function is _not_ used to create leading paths in the\nworking tree. If a merge tells us to create \"foo/bar/baz.test\", we\nshould not do an equivalent of \"mkdir -p foo/bar && cat >baz.test\";\nwe must make sure foo/ and foo/bar are real directories without any\nsymbolic link in the leading paths components (the same thing can be\nsaid for \"apply\"). I have a suspicion that the two hits from apply\nand merge-recursive are showing an existing bug that is not platform\nspecific.\n\nThanks.\n\n\n[Footnote]\n\n*1* In such a context, getting \"a/b\\c/d\" and failing to make sure\n\"a/\" is a directory that has \"b\\c/\" as its immediate subdirectory is\na bug---especially, splitting at the backslash between 'b' and 'c'\nand creating 'a/b/c/' is not acceptable. The platform code should\ninstead say \"sorry, we cannot do that\" if it cannot create a\ndirectory entry \"b\\c\" in the directory \"a/\" (which would make sure\nsuch a non-portable path in projects will be detected).\n"}]}