{"thread":{"id":"49994","subject":"[PATCH 0/2] mingw: support absolute paths without a drive prefix","startedAt":"2018-12-10T18:47:15Z","lastAt":"2018-12-14T11:31:59Z","messageCount":10,"participants":["Johannes Schindelin via GitGitGadget","Johannes Sixt","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"364961","messageId":"pull.96.git.gitgitgadget@gmail.com","threadId":"49994","inReplyTo":null,"subject":"[PATCH 0/2] mingw: support absolute paths without a drive prefix","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-12-10T18:47:13Z","receivedAt":"2018-12-10T18:47:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"On Windows, when using a path like \\hello\\world, it is silently understood\nto be absolute, but relative to the current directory's drive.\n\nLet's support that, too.\n\nJohannes Schindelin (2):\n  mingw: demonstrate a problem with certain absolute paths\n  mingw: allow absolute paths without drive prefix\n\n compat/mingw.c            | 10 +++++++++-\n t/t5580-clone-push-unc.sh | 19 ++++++++++++++-----\n 2 files changed, 23 insertions(+), 6 deletions(-)\n\n\nbase-commit: 5d826e972970a784bd7a7bdf587512510097b8c7\nPublished-As: https://github.com/gitgitgadget/git/releases/tags/pr-96%2Fdscho%2Fdrive-prefix-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-96/dscho/drive-prefix-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/96\n-- \ngitgitgadget\n"},{"id":"364962","messageId":"50ac31ef7f4380f37a0e2d3b75e82b324afee9e3.1544467631.git.gitgitgadget@gmail.com","threadId":"49994","inReplyTo":"pull.96.git.gitgitgadget@gmail.com","subject":"[PATCH 2/2] mingw: allow absolute paths without drive prefix","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-12-10T18:47:15Z","receivedAt":"2018-12-10T18:47:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nWhen specifying an absolute path without a drive prefix, we convert that\npath internally. Let's make sure that we handle that case properly, too\n;-)\n\nThis fixes the command\n\n\tgit clone https://github.com/git-for-windows/git \\G4W\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n compat/mingw.c            | 10 +++++++++-\n t/t5580-clone-push-unc.sh |  2 +-\n 2 files changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 34b3880b29..4d009901d8 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -928,11 +928,19 @@ unsigned int sleep (unsigned int seconds)\n char *mingw_mktemp(char *template)\n {\n \twchar_t wtemplate[MAX_PATH];\n+\tint offset = 0;\n+\n \tif (xutftowcs_path(wtemplate, template) < 0)\n \t\treturn NULL;\n+\n+\tif (is_dir_sep(template[0]) && !is_dir_sep(template[1]) &&\n+\t    iswalpha(wtemplate[0]) && wtemplate[1] == L':') {\n+\t\t/* We have an absolute path missing the drive prefix */\n+\t\toffset = 2;\n+\t}\n \tif (!_wmktemp(wtemplate))\n \t\treturn NULL;\n-\tif (xwcstoutf(template, wtemplate, strlen(template) + 1) < 0)\n+\tif (xwcstoutf(template, wtemplate + offset, strlen(template) + 1) < 0)\n \t\treturn NULL;\n \treturn template;\n }\ndiff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh\nindex c2b0082296..17c38c33a5 100755\n--- a/t/t5580-clone-push-unc.sh\n+++ b/t/t5580-clone-push-unc.sh\n@@ -29,7 +29,7 @@ case \"$UNCPATH\" in\n \t;;\n esac\n \n-test_expect_failure 'clone into absolute path lacking a drive prefix' '\n+test_expect_success 'clone into absolute path lacking a drive prefix' '\n \tUSINGBACKSLASHES=\"$(echo \"$WITHOUTDRIVE\"/without-drive-prefix |\n \t\ttr / \\\\\\\\)\" &&\n \tgit clone . \"$USINGBACKSLASHES\" &&\n-- \ngitgitgadget\n"},{"id":"364963","messageId":"09a73fe1d30c9fedc2f010fe4df75a9cb000389c.1544467631.git.gitgitgadget@gmail.com","threadId":"49994","inReplyTo":"pull.96.git.gitgitgadget@gmail.com","subject":"[PATCH 1/2] mingw: demonstrate a problem with certain absolute paths","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-12-10T18:47:14Z","receivedAt":"2018-12-10T18:47:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nOn Windows, there are several categories of absolute paths. One such\ncategory starts with a backslash and is implicitly relative to the\ndrive associated with the current working directory. Example:\n\n\tc:\n\tgit clone https://github.com/git-for-windows/git \\G4W\n\nshould clone into C:\\G4W.\n\nThere is currently a problem with that, in that mingw_mktemp() does not\nexpect the _wmktemp() function to prefix the absolute path with the\ndrive prefix, and as a consequence, the resulting path does not fit into\nthe originally-passed string buffer. The symptom is a \"Result too large\"\nerror.\n\nReported by Juan Carlos Arevalo Baeza.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n t/t5580-clone-push-unc.sh | 19 ++++++++++++++-----\n 1 file changed, 14 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh\nindex ba548df4a9..c2b0082296 100755\n--- a/t/t5580-clone-push-unc.sh\n+++ b/t/t5580-clone-push-unc.sh\n@@ -17,14 +17,11 @@ fi\n UNCPATH=\"$(winpwd)\"\n case \"$UNCPATH\" in\n [A-Z]:*)\n+\tWITHOUTDRIVE=\"${UNCPATH#?:}\"\n \t# Use administrative share e.g. \\\\localhost\\C$\\git-sdk-64\\usr\\src\\git\n \t# (we use forward slashes here because MSYS2 and Git accept them, and\n \t# they are easier on the eyes)\n-\tUNCPATH=\"//localhost/${UNCPATH%%:*}\\$/${UNCPATH#?:}\"\n-\ttest -d \"$UNCPATH\" || {\n-\t\tskip_all='could not access administrative share; skipping'\n-\t\ttest_done\n-\t}\n+\tUNCPATH=\"//localhost/${UNCPATH%%:*}\\$$WITHOUTDRIVE\"\n \t;;\n *)\n \tskip_all='skipping UNC path tests, cannot determine current path as UNC'\n@@ -32,6 +29,18 @@ case \"$UNCPATH\" in\n \t;;\n esac\n \n+test_expect_failure 'clone into absolute path lacking a drive prefix' '\n+\tUSINGBACKSLASHES=\"$(echo \"$WITHOUTDRIVE\"/without-drive-prefix |\n+\t\ttr / \\\\\\\\)\" &&\n+\tgit clone . \"$USINGBACKSLASHES\" &&\n+\ttest -f without-drive-prefix/.git/HEAD\n+'\n+\n+test -d \"$UNCPATH\" || {\n+\tskip_all='could not access administrative share; skipping'\n+\ttest_done\n+}\n+\n test_expect_success setup '\n \ttest_commit initial\n '\n-- \ngitgitgadget\n\n"},{"id":"364982","messageId":"11b17e5d-e843-463b-77da-263e8e3b7598@kdbg.org","threadId":"49994","inReplyTo":"50ac31ef7f4380f37a0e2d3b75e82b324afee9e3.1544467631.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] mingw: allow absolute paths without drive prefix","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2018-12-10T21:58:28Z","receivedAt":"2018-12-10T21:58:35Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 10.12.18 um 19:47 schrieb Johannes Schindelin via GitGitGadget:\n> From: Johannes Schindelin <johannes.schindelin@gmx.de>\n> \n> When specifying an absolute path without a drive prefix, we convert that\n> path internally. Let's make sure that we handle that case properly, too\n> ;-)\n> \n> This fixes the command\n> \n> \tgit clone https://github.com/git-for-windows/git \\G4W\n> \n> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> ---\n>   compat/mingw.c            | 10 +++++++++-\n>   t/t5580-clone-push-unc.sh |  2 +-\n>   2 files changed, 10 insertions(+), 2 deletions(-)\n> \n> diff --git a/compat/mingw.c b/compat/mingw.c\n> index 34b3880b29..4d009901d8 100644\n> --- a/compat/mingw.c\n> +++ b/compat/mingw.c\n> @@ -928,11 +928,19 @@ unsigned int sleep (unsigned int seconds)\n>   char *mingw_mktemp(char *template)\n>   {\n>   \twchar_t wtemplate[MAX_PATH];\n> +\tint offset = 0;\n> +\n>   \tif (xutftowcs_path(wtemplate, template) < 0)\n>   \t\treturn NULL;\n> +\n> +\tif (is_dir_sep(template[0]) && !is_dir_sep(template[1]) &&\n> +\t    iswalpha(wtemplate[0]) && wtemplate[1] == L':') {\n> +\t\t/* We have an absolute path missing the drive prefix */\n\nThis comment is true for the source part, template, but I can't find \nwhere the destination, wtemplate, suddenly gets the drive prefix. As far \nas I can see, xutftowcs_path() just does a plain textual conversion \nwithout any interpretation of the text as path. Can you explain it?\n\nBTW, iswalpha() is not restricted to ASCII letters, I would rewrite it \nas (wtemplate[0] <= 127 && isalpha(wtemplate[0]).\n\n> +\t\toffset = 2;\n> +\t}\n>   \tif (!_wmktemp(wtemplate))\n>   \t\treturn NULL;\n> -\tif (xwcstoutf(template, wtemplate, strlen(template) + 1) < 0)\n> +\tif (xwcstoutf(template, wtemplate + offset, strlen(template) + 1) < 0)\n>   \t\treturn NULL;\n>   \treturn template;\n>   }\n> diff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh\n> index c2b0082296..17c38c33a5 100755\n> --- a/t/t5580-clone-push-unc.sh\n> +++ b/t/t5580-clone-push-unc.sh\n> @@ -29,7 +29,7 @@ case \"$UNCPATH\" in\n>   \t;;\n>   esac\n>   \n> -test_expect_failure 'clone into absolute path lacking a drive prefix' '\n> +test_expect_success 'clone into absolute path lacking a drive prefix' '\n>   \tUSINGBACKSLASHES=\"$(echo \"$WITHOUTDRIVE\"/without-drive-prefix |\n>   \t\ttr / \\\\\\\\)\" &&\n>   \tgit clone . \"$USINGBACKSLASHES\" &&\n> \n\n-- Hannes\n"},{"id":"365066","messageId":"nycvar.QRO.7.76.6.1812111134250.43@tvgsbejvaqbjf.bet","threadId":"49994","inReplyTo":"11b17e5d-e843-463b-77da-263e8e3b7598@kdbg.org","subject":"Re: [PATCH 2/2] mingw: allow absolute paths without drive prefix","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-12-11T11:25:42Z","receivedAt":"2018-12-11T11:25:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Hannes,\n\nOn Mon, 10 Dec 2018, Johannes Sixt wrote:\n\n> > diff --git a/compat/mingw.c b/compat/mingw.c\n> > index 34b3880b29..4d009901d8 100644\n> > --- a/compat/mingw.c\n> > +++ b/compat/mingw.c\n> > @@ -928,11 +928,19 @@ unsigned int sleep (unsigned int seconds)\n> >   char *mingw_mktemp(char *template)\n> >   {\n> >   \twchar_t wtemplate[MAX_PATH];\n> > +\tint offset = 0;\n> > +\n> >    if (xutftowcs_path(wtemplate, template) < 0)\n> >   \t\treturn NULL;\n> > +\n> > +\tif (is_dir_sep(template[0]) && !is_dir_sep(template[1]) &&\n> > +\t    iswalpha(wtemplate[0]) && wtemplate[1] == L':') {\n> > +\t\t/* We have an absolute path missing the drive prefix */\n> \n> This comment is true for the source part, template, but I can't find\n> where the destination, wtemplate, suddenly gets the drive prefix. As far\n> as I can see, xutftowcs_path() just does a plain textual conversion\n> without any interpretation of the text as path. Can you explain it?\n\nIt is legal on Windows for such a path to lack the drive prefix, also in\nthe wide-character version. So the explanation is: even `wtemplate` won't\nget the drive prefix. It does not need to.\n\n> BTW, iswalpha() is not restricted to ASCII letters, I would rewrite it\n> as (wtemplate[0] <= 127 && isalpha(wtemplate[0]).\n\nVery good point! Will fix.\n\nThanks,\nDscho\n"},{"id":"365117","messageId":"8a484f86-1d43-fc0a-22b4-39c770cda6cb@kdbg.org","threadId":"49994","inReplyTo":"nycvar.QRO.7.76.6.1812111134250.43@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 2/2] mingw: allow absolute paths without drive prefix","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2018-12-11T20:36:06Z","receivedAt":"2018-12-11T20:36:11Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 11.12.18 um 12:25 schrieb Johannes Schindelin:\n> On Mon, 10 Dec 2018, Johannes Sixt wrote:\n>>> diff --git a/compat/mingw.c b/compat/mingw.c\n>>> index 34b3880b29..4d009901d8 100644\n>>> --- a/compat/mingw.c\n>>> +++ b/compat/mingw.c\n>>> @@ -928,11 +928,19 @@ unsigned int sleep (unsigned int seconds)\n>>>    char *mingw_mktemp(char *template)\n>>>    {\n>>>    \twchar_t wtemplate[MAX_PATH];\n>>> +\tint offset = 0;\n>>> +\n>>>     if (xutftowcs_path(wtemplate, template) < 0)\n>>>    \t\treturn NULL;\n>>> +\n>>> +\tif (is_dir_sep(template[0]) && !is_dir_sep(template[1]) &&\n>>> +\t    iswalpha(wtemplate[0]) && wtemplate[1] == L':') {\n>>> +\t\t/* We have an absolute path missing the drive prefix */\n>>\n>> This comment is true for the source part, template, but I can't find\n>> where the destination, wtemplate, suddenly gets the drive prefix. As far\n>> as I can see, xutftowcs_path() just does a plain textual conversion\n>> without any interpretation of the text as path. Can you explain it?\n> \n> It is legal on Windows for such a path to lack the drive prefix, also in\n> the wide-character version. So the explanation is: even `wtemplate` won't\n> get the drive prefix. It does not need to.\n\nI'm sorry, my question was extremely fuzzy. I actually wanted to know \nhow the condition that you introduce in this patch can ever be true.\n\nAnd after looking at the Git for Windows code, I could answer it myself: \nit cannot. Not with this patch alone. In GfW, there is additional code \nin xutftowcs_path() that massages wtemplate to receive a drive prefix; \nbut vanilla Git does not have that code, so that is_dir_sep(template[0]) \nand iswalpha(wtemplate[0]) can never be true at the same time at this point.\n\n-- Hannes\n"},{"id":"365215","messageId":"xmqqmupagn0y.fsf@gitster-ct.c.googlers.com","threadId":"49994","inReplyTo":"8a484f86-1d43-fc0a-22b4-39c770cda6cb@kdbg.org","subject":"Re: [PATCH 2/2] mingw: allow absolute paths without drive prefix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-13T02:48:13Z","receivedAt":"2018-12-13T02:48:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n>>>> +\tif (is_dir_sep(template[0]) && !is_dir_sep(template[1]) &&\n>>>> +\t    iswalpha(wtemplate[0]) && wtemplate[1] == L':') {\n>>>> +\t\t/* We have an absolute path missing the drive prefix */\n>>>\n>>> This comment is true for the source part, template, but I can't find\n>>> where the destination, wtemplate, suddenly gets the drive prefix. As far\n>>> as I can see, xutftowcs_path() just does a plain textual conversion\n>>> without any interpretation of the text as path. Can you explain it?\n>>\n>> It is legal on Windows for such a path to lack the drive prefix, also in\n>> the wide-character version. So the explanation is: even `wtemplate` won't\n>> get the drive prefix. It does not need to.\n>\n> I'm sorry, my question was extremely fuzzy. I actually wanted to know\n> how the condition that you introduce in this patch can ever be true.\n>\n> And after looking at the Git for Windows code, I could answer it\n> myself: it cannot. Not with this patch alone. In GfW, there is\n> additional code in xutftowcs_path() that massages wtemplate to receive\n> a drive prefix; but vanilla Git does not have that code, so that\n> is_dir_sep(template[0]) and iswalpha(wtemplate[0]) can never be true\n> at the same time at this point.\n\nSo,... what's the conclusion?  The patch in the context of my tree\nwould be a no-op, and we'd need a prerequisite change to the support\nfunction to accompany this patch to be effective?\n\n"},{"id":"365228","messageId":"8efe7938-12a9-7db3-8f95-825ee2e32247@kdbg.org","threadId":"49994","inReplyTo":"xmqqmupagn0y.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/2] mingw: allow absolute paths without drive prefix","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2018-12-13T06:25:05Z","receivedAt":"2018-12-13T06:25:10Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 13.12.18 um 03:48 schrieb Junio C Hamano:\n> So,... what's the conclusion?  The patch in the context of my tree\n> would be a no-op, and we'd need a prerequisite change to the support\n> function to accompany this patch to be effective?\n\nCorrect, that is my conclusion.\n\n-- Hannes\n"},{"id":"365287","messageId":"70faf8fb-5e98-9247-9955-71b8fc983567@kdbg.org","threadId":"49994","inReplyTo":"8efe7938-12a9-7db3-8f95-825ee2e32247@kdbg.org","subject":"Re: [PATCH 2/2] mingw: allow absolute paths without drive prefix","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2018-12-13T19:38:14Z","receivedAt":"2018-12-13T19:38:18Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 13.12.18 um 07:25 schrieb Johannes Sixt:\n> Am 13.12.18 um 03:48 schrieb Junio C Hamano:\n>> So,... what's the conclusion?  The patch in the context of my tree\n>> would be a no-op, and we'd need a prerequisite change to the support\n>> function to accompany this patch to be effective?\n> \n> Correct, that is my conclusion.\n\nMoreover, there is no problem with your tree to begin with. I cloned \ninto a destination without a drive letter:\n\nC:\\>git clone R:\\j6t\\expat.git \\Temp\\expat\nCloning into '\\Temp\\expat'...\ndone.\n\nand it works fine. If I understood the description in patch 1/2 \ncorrectly, this should have failed.\n\n-- Hannes\n"},{"id":"365345","messageId":"nycvar.QRO.7.76.6.1812141230310.43@tvgsbejvaqbjf.bet","threadId":"49994","inReplyTo":"70faf8fb-5e98-9247-9955-71b8fc983567@kdbg.org","subject":"Re: [PATCH 2/2] mingw: allow absolute paths without drive prefix","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-12-14T11:31:34Z","receivedAt":"2018-12-14T11:31:59Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Hannes,\n\nOn Thu, 13 Dec 2018, Johannes Sixt wrote:\n\n> Am 13.12.18 um 07:25 schrieb Johannes Sixt:\n> > Am 13.12.18 um 03:48 schrieb Junio C Hamano:\n> > > So,... what's the conclusion?  The patch in the context of my tree\n> > > would be a no-op, and we'd need a prerequisite change to the support\n> > > function to accompany this patch to be effective?\n> > \n> > Correct, that is my conclusion.\n> \n> Moreover, there is no problem with your tree to begin with. I cloned into a\n> destination without a drive letter:\n> \n> C:\\>git clone R:\\j6t\\expat.git \\Temp\\expat\n> Cloning into '\\Temp\\expat'...\n> done.\n> \n> and it works fine. If I understood the description in patch 1/2 correctly,\n> this should have failed.\n\nThank you for the analysis. I'll look which patches in Git for Windows\nintroduced that regression, then, and fold this here patch series into\nthat one.\n\nThanks,\nDscho"}]}