mingw: support absolute paths without a drive prefix
cover mingw: support absolute paths without a drive prefix
On Windows, when using a path like \hello\world, it is silently understood to be absolute, but relative to the current directory's drive.
Let's support that, too.
Johannes Schindelin (2):
mingw: demonstrate a problem with certain absolute paths
mingw: allow absolute paths without drive prefix
compat/mingw.c | 10 +++++++++- t/t5580-clone-push-unc.sh | 19 ++++++++++++++----- 2 files changed, 23 insertions(+), 6 deletions(-)
base-commit: 5d826e972970a784bd7a7bdf587512510097b8c7
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-96%2Fdscho%2Fdrive-prefix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-96/dscho/drive-prefix-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/96
Signature · 1 line
gitgitgadget
PATCH 1/2 mingw: demonstrate a problem with certain absolute paths
From: Johannes Schindelin <[email protected]>
On Windows, there are several categories of absolute paths. One such category starts with a backslash and is implicitly relative to the drive associated with the current working directory. Example:
c: git clone https://github.com/git-for-windows/git \G4W
should clone into C:\G4W.
There is currently a problem with that, in that mingw_mktemp() does not expect the _wmktemp() function to prefix the absolute path with the drive prefix, and as a consequence, the resulting path does not fit into the originally-passed string buffer. The symptom is a "Result too large" error.
Reported by Juan Carlos Arevalo Baeza.
--- t/t5580-clone-push-unc.sh | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)
t/t5580-clone-push-unc.sh +14 -5
@@ -17,14 +17,11 @@ fi UNCPATH="$(winpwd)" case "$UNCPATH" in [A-Z]:*) + WITHOUTDRIVE="${UNCPATH#?:}" # Use administrative share e.g. \\localhost\C$\git-sdk-64\usr\src\git # (we use forward slashes here because MSYS2 and Git accept them, and # they are easier on the eyes) - UNCPATH="//localhost/${UNCPATH%%:*}\$/${UNCPATH#?:}" - test -d "$UNCPATH" || { - skip_all='could not access administrative share; skipping' - test_done - } + UNCPATH="//localhost/${UNCPATH%%:*}\$$WITHOUTDRIVE" ;; *) skip_all='skipping UNC path tests, cannot determine current path as UNC'
@@ -32,6 +29,18 @@ case "$UNCPATH" in ;; esac +test_expect_failure 'clone into absolute path lacking a drive prefix' ' + USINGBACKSLASHES="$(echo "$WITHOUTDRIVE"/without-drive-prefix | + tr / \\\\)" && + git clone . "$USINGBACKSLASHES" && + test -f without-drive-prefix/.git/HEAD +' + +test -d "$UNCPATH" || { + skip_all='could not access administrative share; skipping' + test_done +} + test_expect_success setup ' test_commit initial '
Signature · 1 line
gitgitgadget
PATCH 2/2 mingw: allow absolute paths without drive prefix
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
When specifying an absolute path without a drive prefix, we convert that path internally. Let's make sure that we handle that case properly, too ;-)
This fixes the command
git clone https://github.com/git-for-windows/git \G4W
--- compat/mingw.c | 10 +++++++++- t/t5580-clone-push-unc.sh | 2 +- 2 files changed, 10 insertions(+), 2 deletions(-)
compat/mingw.c +9 -1
@@ -928,11 +928,19 @@ unsigned int sleep (unsigned int seconds)char *mingw_mktemp (char *template) {wchar_t wtemplate[MAX_PATH ]; +int offset =0 ; +if (xutftowcs_path (wtemplate, template) <0 )return NULL ; + +if (is_dir_sep (template[0 ]) && !is_dir_sep (template[1 ]) && +iswalpha (wtemplate[0 ]) && wtemplate[1 ] ==L':' ) { +/* We have an absolute path missing the drive prefix */ + offset =2 ; + }if (!_wmktemp (wtemplate))return NULL ; -if (xwcstoutf (template, wtemplate,strlen (template) +1 ) <0 ) +if (xwcstoutf (template, wtemplate + offset,strlen (template) +1 ) <0 )return NULL ;return template; }
t/t5580-clone-push-unc.sh +1 -1
@@ -29,7 +29,7 @@ case "$UNCPATH" in ;; esac -test_expect_failure 'clone into absolute path lacking a drive prefix' ' +test_expect_success 'clone into absolute path lacking a drive prefix' ' USINGBACKSLASHES="$(echo "$WITHOUTDRIVE"/without-drive-prefix | tr / \\\\)" && git clone . "$USINGBACKSLASHES" &&
Signature · 1 line
gitgitgadget
Re: [PATCH 2/2] mingw: allow absolute paths without drive prefix
Am 10.12.18 um 19:47 schrieb Johannes Schindelin via GitGitGadget:
Quoted 22 lines
From: Johannes Schindelin <[email protected]>
When specifying an absolute path without a drive prefix, we convert that path internally. Let's make sure that we handle that case properly, too ;-)
This fixes the command
git clone https://github.com/git-for-windows/git \G4W
Signed-off-by Johannes Schindelin <[email protected]>--- compat/mingw.c | 10 +++++++++- t/t5580-clone-push-unc.sh | 2 +- 2 files changed, 10 insertions(+), 2 deletions(-)compat/mingw.c +0
-0@@ -928,11 +928,19 @@ unsigned int sleep (unsigned int seconds)char *mingw_mktemp (char *template)
compat/mingw.c unsigned int sleep (unsigned int seconds) · line 938
Quoted 10 lines
compat/mingw.c +6
-0{wchar_t wtemplate[MAX_PATH ]; +int offset =0 ; +if (xutftowcs_path (wtemplate, template) <0 )return NULL ; + +if (is_dir_sep (template[0 ]) && !is_dir_sep (template[1 ]) && +iswalpha (wtemplate[0 ]) && wtemplate[1 ] ==L':' ) { +/* We have an absolute path missing the drive prefix */
This comment is true for the source part, template, but I can't find where the destination, wtemplate, suddenly gets the drive prefix. As far as I can see, xutftowcs_path() just does a plain textual conversion without any interpretation of the text as path. Can you explain it?
BTW, iswalpha() is not restricted to ASCII letters, I would rewrite it as (wtemplate[0] <= 127 && isalpha(wtemplate[0]).
Quoted 15 lines
compat/mingw.c +3
-1+ offset =2 ; + }if (!_wmktemp (wtemplate))return NULL ; -if (xwcstoutf (template, wtemplate,strlen (template) +1 ) <0 ) +if (xwcstoutf (template, wtemplate + offset,strlen (template) +1 ) <0 )return NULL ;return template; }t/t5580-clone-push-unc.sh +0
-0
t/t5580-clone-push-unc.sh case "$UNCPATH" in · line 35
Quoted 8 lines
t/t5580-clone-push-unc.sh +1
-1@@ -29,7 +29,7 @@ case "$UNCPATH" in ;; esac -test_expect_failure 'clone into absolute path lacking a drive prefix' ' +test_expect_success 'clone into absolute path lacking a drive prefix' ' USINGBACKSLASHES="$(echo "$WITHOUTDRIVE"/without-drive-prefix | tr / \\\\)" && git clone . "$USINGBACKSLASHES" &&
-- Hannes
Re: [PATCH 2/2] mingw: allow absolute paths without drive prefix
Hi Hannes,
Quoted 13 lines · Johannes Sixt
Quoted 13 lines
compat/mingw.c +3
-0@@ -928,11 +928,19 @@ unsigned int sleep (unsigned int seconds)char *mingw_mktemp (char *template) {wchar_t wtemplate[MAX_PATH ]; +int offset =0 ; +if (xutftowcs_path (wtemplate, template) <0 )return NULL ; +
Quoted 8 lines
compat/mingw.c unsigned int sleep (unsigned int seconds) · line 938
Quoted 3 lines
compat/mingw.c +3
-0+if (is_dir_sep (template[0 ]) && !is_dir_sep (template[1 ]) && +iswalpha (wtemplate[0 ]) && wtemplate[1 ] ==L':' ) { +/* We have an absolute path missing the drive prefix */ This comment is true for the source part, template, but I can't find where the destination, wtemplate, suddenly gets the drive prefix. As far as I can see,
xutftowcs_path()just does a plain textual conversion without any interpretation of the text as path. Can you explain it?
It is legal on Windows for such a path to lack the drive prefix, also in the wide-character version. So the explanation is: even wtemplate won't get the drive prefix. It does not need to.
Quoted 2 lines
BTW,
iswalpha()is not restricted to ASCII letters, I would rewrite it as (wtemplate[0] <= 127 &&isalpha(wtemplate[0]).
Very good point! Will fix.
Thanks,
Dscho
Re: [PATCH 2/2] mingw: allow absolute paths without drive prefix
Am 11.12.18 um 12:25 schrieb Johannes Schindelin:
Quoted 23 lines · Johannes Sixt
Quoted 13 lines
Quoted 13 lines
compat/mingw.c +3
-0@@ -928,11 +928,19 @@ unsigned int sleep (unsigned int seconds)char *mingw_mktemp (char *template) {wchar_t wtemplate[MAX_PATH ]; +int offset =0 ; +if (xutftowcs_path (wtemplate, template) <0 )return NULL ; +Quoted 8 lines
compat/mingw.c unsigned int sleep (unsigned int seconds) · line 938
Quoted 3 lines
compat/mingw.c +3
-0+if (is_dir_sep (template[0 ]) && !is_dir_sep (template[1 ]) && +iswalpha (wtemplate[0 ]) && wtemplate[1 ] ==L':' ) { +/* We have an absolute path missing the drive prefix */ This comment is true for the source part, template, but I can't find where the destination, wtemplate, suddenly gets the drive prefix. As far as I can see,
xutftowcs_path()just does a plain textual conversion without any interpretation of the text as path. Can you explain it?
Quoted 3 lines
It is legal on Windows for such a path to lack the drive prefix, also in the wide-character version. So the explanation is: even
wtemplatewon't get the drive prefix. It does not need to.
I'm sorry, my question was extremely fuzzy. I actually wanted to know how the condition that you introduce in this patch can ever be true.
And after looking at the Git for Windows code, I could answer it myself: it cannot. Not with this patch alone. In GfW, there is additional code in xutftowcs_path() that massages wtemplate to receive a drive prefix; but vanilla Git does not have that code, so that is_dir_sep(template[0]) and iswalpha(wtemplate[0]) can never be true at the same time at this point.
-- Hannes
Re: [PATCH 2/2] mingw: allow absolute paths without drive prefix
Quoted 16 lines · Johannes Sixt
Quoted 9 lines
Quoted 8 lines
compat/mingw.c unsigned int sleep (unsigned int seconds) · line 938
Quoted 3 lines
compat/mingw.c +3
-0+if (is_dir_sep (template[0 ]) && !is_dir_sep (template[1 ]) && +iswalpha (wtemplate[0 ]) && wtemplate[1 ] ==L':' ) { +/* We have an absolute path missing the drive prefix */ This comment is true for the source part, template, but I can't find where the destination, wtemplate, suddenly gets the drive prefix. As far as I can see,
xutftowcs_path()just does a plain textual conversion without any interpretation of the text as path. Can you explain it?Quoted 3 lines
It is legal on Windows for such a path to lack the drive prefix, also in the wide-character version. So the explanation is: even
wtemplatewon't get the drive prefix. It does not need to.I'm sorry, my question was extremely fuzzy. I actually wanted to know how the condition that you introduce in this patch can ever be true.
Quoted 6 lines
And after looking at the Git for Windows code, I could answer it
myself: it cannot. Not with this patch alone. In GfW, there is additional code inxutftowcs_path()that massages wtemplate to receive a drive prefix; but vanilla Git does not have that code, so thatis_dir_sep(template[0])andiswalpha(wtemplate[0])can never be true at the same time at this point.
So,... what's the conclusion? The patch in the context of my tree would be a no-op, and we'd need a prerequisite change to the support function to accompany this patch to be effective?
Re: [PATCH 2/2] mingw: allow absolute paths without drive prefix
Am 13.12.18 um 03:48 schrieb Junio C Hamano:
Quoted 3 lines
So,... what's the conclusion? The patch in the context of my tree would be a no-op, and we'd need a prerequisite change to the support function to accompany this patch to be effective?
Correct, that is my conclusion.
-- Hannes
Re: [PATCH 2/2] mingw: allow absolute paths without drive prefix
Am 13.12.18 um 07:25 schrieb Johannes Sixt:
Quoted 6 lines
Am 13.12.18 um 03:48 schrieb Junio C Hamano:
Quoted 3 lines
So,... what's the conclusion? The patch in the context of my tree would be a no-op, and we'd need a prerequisite change to the support function to accompany this patch to be effective?
Correct, that is my conclusion.
Moreover, there is no problem with your tree to begin with. I cloned into a destination without a drive letter:
C:\>git clone R:\j6t\expat.git \Temp\expat
Cloning into '\Temp\expat'...
done.
and it works fine. If I understood the description in patch 1/2 correctly, this should have failed.
-- Hannes
Re: [PATCH 2/2] mingw: allow absolute paths without drive prefix
Hi Hannes,
Quoted 11 lines · Johannes Sixt
Am 13.12.18 um 07:25 schrieb Johannes Sixt:
Quoted 6 lines
Am 13.12.18 um 03:48 schrieb Junio C Hamano:
Quoted 3 lines
So,... what's the conclusion? The patch in the context of my tree would be a no-op, and we'd need a prerequisite change to the support function to accompany this patch to be effective?
Correct, that is my conclusion.
Moreover, there is no problem with your tree to begin with. I cloned into a destination without a drive letter:
Quoted 6 lines
C:\>git clone R:\j6t\expat.git \Temp\expat
Cloning into '\Temp\expat'...
done.and it works fine. If I understood the description in patch 1/2 correctly, this should have failed.
Thank you for the analysis. I'll look which patches in Git for Windows introduced that regression, then, and fold this here patch series into that one.
Thanks,
Dscho