git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH 0/2] mingw: support absolute paths without a drive prefix
@ 2018-12-10 18:47 Johannes Schindelin via GitGitGadget
  2018-12-10 18:47 ` [PATCH 1/2] mingw: demonstrate a problem with certain absolute paths Johannes Schindelin via GitGitGadget
  2018-12-10 18:47 ` [PATCH 2/2] mingw: allow absolute paths without drive prefix Johannes Schindelin via GitGitGadget
  0 siblings, 2 replies; 10+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2018-12-10 18:47 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano

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
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/2] mingw: demonstrate a problem with certain absolute paths
  2018-12-10 18:47 [PATCH 0/2] mingw: support absolute paths without a drive prefix Johannes Schindelin via GitGitGadget
@ 2018-12-10 18:47 ` Johannes Schindelin via GitGitGadget
  2018-12-10 18:47 ` [PATCH 2/2] mingw: allow absolute paths without drive prefix Johannes Schindelin via GitGitGadget
  1 sibling, 0 replies; 10+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2018-12-10 18:47 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t5580-clone-push-unc.sh | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh
index ba548df4a9..c2b0082296 100755
--- a/t/t5580-clone-push-unc.sh
+++ b/t/t5580-clone-push-unc.sh
@@ -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
 '
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 2/2] mingw: allow absolute paths without drive prefix
  2018-12-10 18:47 [PATCH 0/2] mingw: support absolute paths without a drive prefix Johannes Schindelin via GitGitGadget
  2018-12-10 18:47 ` [PATCH 1/2] mingw: demonstrate a problem with certain absolute paths Johannes Schindelin via GitGitGadget
@ 2018-12-10 18:47 ` Johannes Schindelin via GitGitGadget
  2018-12-10 21:58   ` Johannes Sixt
  1 sibling, 1 reply; 10+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2018-12-10 18:47 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Johannes Schindelin

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 */
+		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" &&
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] mingw: allow absolute paths without drive prefix
  2018-12-10 18:47 ` [PATCH 2/2] mingw: allow absolute paths without drive prefix Johannes Schindelin via GitGitGadget
@ 2018-12-10 21:58   ` Johannes Sixt
  2018-12-11 11:25     ` Johannes Schindelin
  0 siblings, 1 reply; 10+ messages in thread
From: Johannes Sixt @ 2018-12-10 21:58 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget
  Cc: git, Junio C Hamano, Johannes Schindelin

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] mingw: allow absolute paths without drive prefix
  2018-12-10 21:58   ` Johannes Sixt
@ 2018-12-11 11:25     ` Johannes Schindelin
  2018-12-11 20:36       ` Johannes Sixt
  0 siblings, 1 reply; 10+ messages in thread
From: Johannes Schindelin @ 2018-12-11 11:25 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Johannes Schindelin via GitGitGadget, git, Junio C Hamano

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] mingw: allow absolute paths without drive prefix
  2018-12-11 11:25     ` Johannes Schindelin
@ 2018-12-11 20:36       ` Johannes Sixt
  2018-12-13  2:48         ` Junio C Hamano
  0 siblings, 1 reply; 10+ messages in thread
From: Johannes Sixt @ 2018-12-11 20:36 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Johannes Schindelin via GitGitGadget, git, Junio C Hamano

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] mingw: allow absolute paths without drive prefix
  2018-12-11 20:36       ` Johannes Sixt
@ 2018-12-13  2:48         ` Junio C Hamano
  2018-12-13  6:25           ` Johannes Sixt
  0 siblings, 1 reply; 10+ messages in thread
From: Junio C Hamano @ 2018-12-13  2:48 UTC (permalink / raw)
  To: Johannes Sixt
  Cc: Johannes Schindelin, Johannes Schindelin via GitGitGadget, git

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?


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] mingw: allow absolute paths without drive prefix
  2018-12-13  2:48         ` Junio C Hamano
@ 2018-12-13  6:25           ` Johannes Sixt
  2018-12-13 19:38             ` Johannes Sixt
  0 siblings, 1 reply; 10+ messages in thread
From: Johannes Sixt @ 2018-12-13  6:25 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Johannes Schindelin, Johannes Schindelin via GitGitGadget, git

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] mingw: allow absolute paths without drive prefix
  2018-12-13  6:25           ` Johannes Sixt
@ 2018-12-13 19:38             ` Johannes Sixt
  2018-12-14 11:31               ` Johannes Schindelin
  0 siblings, 1 reply; 10+ messages in thread
From: Johannes Sixt @ 2018-12-13 19:38 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Johannes Schindelin, Johannes Schindelin via GitGitGadget, git

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] mingw: allow absolute paths without drive prefix
  2018-12-13 19:38             ` Johannes Sixt
@ 2018-12-14 11:31               ` Johannes Schindelin
  0 siblings, 0 replies; 10+ messages in thread
From: Johannes Schindelin @ 2018-12-14 11:31 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Junio C Hamano, Johannes Schindelin via GitGitGadget, git

[-- Attachment #1: Type: text/plain, Size: 877 bytes --]

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2018-12-14 11:31 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-10 18:47 [PATCH 0/2] mingw: support absolute paths without a drive prefix Johannes Schindelin via GitGitGadget
2018-12-10 18:47 ` [PATCH 1/2] mingw: demonstrate a problem with certain absolute paths Johannes Schindelin via GitGitGadget
2018-12-10 18:47 ` [PATCH 2/2] mingw: allow absolute paths without drive prefix Johannes Schindelin via GitGitGadget
2018-12-10 21:58   ` Johannes Sixt
2018-12-11 11:25     ` Johannes Schindelin
2018-12-11 20:36       ` Johannes Sixt
2018-12-13  2:48         ` Junio C Hamano
2018-12-13  6:25           ` Johannes Sixt
2018-12-13 19:38             ` Johannes Sixt
2018-12-14 11:31               ` Johannes Schindelin

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git