| Message ID | 50ac31ef7f4380f37a0e2d3b75e82b324afee9e3.1544467631.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mingw: support absolute paths without a drive prefix | expand |
Am 10.12.18 um 19:47 schrieb Johannes Schindelin via GitGitGadget: > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > 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 <johannes.schindelin@gmx.de> > --- > compat/mingw.c | 10 +++++++++- > t/t5580-clone-push-unc.sh | 2 +- > 2 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/compat/mingw.c b/compat/mingw.c > index 34b3880b29..4d009901d8 100644 > --- a/compat/mingw.c > +++ b/compat/mingw.c > @@ -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 */ 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]). > + 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; > } > diff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh > index c2b0082296..17c38c33a5 100755 > --- a/t/t5580-clone-push-unc.sh > +++ b/t/t5580-clone-push-unc.sh > @@ -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
Hi Hannes, On Mon, 10 Dec 2018, Johannes Sixt wrote: > > diff --git a/compat/mingw.c b/compat/mingw.c > > index 34b3880b29..4d009901d8 100644 > > --- a/compat/mingw.c > > +++ b/compat/mingw.c > > @@ -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 */ > > 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. > 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
Am 11.12.18 um 12:25 schrieb Johannes Schindelin: > On Mon, 10 Dec 2018, Johannes Sixt wrote: >>> diff --git a/compat/mingw.c b/compat/mingw.c >>> index 34b3880b29..4d009901d8 100644 >>> --- a/compat/mingw.c >>> +++ b/compat/mingw.c >>> @@ -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 */ >> >> 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. 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
Johannes Sixt <j6t@kdbg.org> writes: >>>> + 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. > > 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. 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?
Am 13.12.18 um 03:48 schrieb Junio C Hamano: > 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
Am 13.12.18 um 07:25 schrieb Johannes Sixt: > Am 13.12.18 um 03:48 schrieb Junio C Hamano: >> 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
Hi Hannes, On Thu, 13 Dec 2018, Johannes Sixt wrote: > Am 13.12.18 um 07:25 schrieb Johannes Sixt: > > Am 13.12.18 um 03:48 schrieb Junio C Hamano: > > > 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. 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
diff --git a/compat/mingw.c b/compat/mingw.c index 34b3880b29..4d009901d8 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -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; } diff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh index c2b0082296..17c38c33a5 100755 --- a/t/t5580-clone-push-unc.sh +++ b/t/t5580-clone-push-unc.sh @@ -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" &&