Re: [PATCH 13/19] mingw: outsmart MSYS2's path substitution in t1508

Johannes Schindelin [off-list ref] writes:

From: Thomas Braun <redacted>

A string of the form "@/abcd" is considered a file path
by the msys layer and therefore translated to a Windows path.

Here the trick is to double the slashes.

The MSYS2 patch translation can be studied by calling

	test-path-utils print_path <path>

Signed-off-by: Thomas Braun <redacted>
Signed-off-by: Johannes Schindelin <redacted>
---

This feels wrong.

The point of this test is that you can ask to checkout a branch
whose name is a strangely looking "@/at-test", and a ref whose name
is "refs/heads/@/at-test" indeed is created.

The current "checkout" may be lazy and not signal an error for a
branch name with two consecutive slashes, but I wouldn't be
surprised if we tighten that later, and more importantly, I do not
think we ever promised users if you asked a branch "a//b" to be
created, we would create "refs/heads/a/b".

The new test hardcodes and promises such an incompatible behaviour,
i.e. a request to create "@//b" results in "@/b" created, only to
users on MINGW, fracturing the expectations of the Git userbase.

Wouldn't it be better to declare "On other people's Git, @/foo is
just as normal a branch name as a/foo, but on MINGW @/foo cannot be
used" by skipping some tests using prerequisites instead?

On Sun, Jan 24, 2016 at 9:03 PM, Junio C Hamano [off-list ref] wrote:

Johannes Schindelin [off-list ref] writes:

quoted

A string of the form "@/abcd" is considered a file path
by the msys layer and therefore translated to a Windows path.

Here the trick is to double the slashes.

The MSYS2 patch translation can be studied by calling

      test-path-utils print_path <path>

This feels wrong.

The point of this test is that you can ask to checkout a branch
whose name is a strangely looking "@/at-test", and a ref whose name
is "refs/heads/@/at-test" indeed is created.

The current "checkout" may be lazy and not signal an error for a
branch name with two consecutive slashes, but I wouldn't be
surprised if we tighten that later, and more importantly, I do not
think we ever promised users if you asked a branch "a//b" to be
created, we would create "refs/heads/a/b".

The new test hardcodes and promises such an incompatible behaviour,
i.e. a request to create "@//b" results in "@/b" created, only to
users on MINGW, fracturing the expectations of the Git userbase.

What the commit message doesn't explain is that the MSYS2 Bash shell
automatically translates command-line arguments which look like
absolute POSIX paths into Windows paths on behalf of the program being
invoked. For instance, given the command-line:

    myprog /fish

the shell will translate /fish to a Windows pathname, such as C:\fish
before invoking myprog.exe. While this is often the desired behavior,
sometimes the argument represents something other than a path, and the
translation is unwanted. MSYS provides a way to suppress the behavior
manually by doubling the slash, so:

    myprog //fish

will invoke myprog.exe with literal argument /fish (notice the single slash).

This commit message is trying to say that MSYS shell undesirably sees
@/fish as an absolute path, thus tries translating it to a Windows
path, such as @C:\fish. The only way to suppress this unwanted
translation is to manually double the slash, hence the patch makes the
test use @//fish which, when finally seen by the program, is just
@/fish, as was intended in the first place. So, doubling the slash on
MINGW is not promising incompatible behavior for MINGW users; it's
just working around unwanted path translation of the shell.

Hi Junio,

On Sun, 24 Jan 2016, Junio C Hamano wrote:

Johannes Schindelin [off-list ref] writes:

quoted

From: Thomas Braun <redacted>

A string of the form "@/abcd" is considered a file path
by the msys layer and therefore translated to a Windows path.

Here the trick is to double the slashes.

The MSYS2 patch translation can be studied by calling

	test-path-utils print_path <path>

Signed-off-by: Thomas Braun <redacted>
Signed-off-by: Johannes Schindelin <redacted>
---

This feels wrong.

The point of this test is that you can ask to checkout a branch
whose name is a strangely looking "@/at-test", and a ref whose name
is "refs/heads/@/at-test" indeed is created.

The current "checkout" may be lazy and not signal an error for a
branch name with two consecutive slashes, but I wouldn't be
surprised if we tighten that later, and more importantly, I do not
think we ever promised users if you asked a branch "a//b" to be
created, we would create "refs/heads/a/b".

The new test hardcodes and promises such an incompatible behaviour,
i.e. a request to create "@//b" results in "@/b" created, only to
users on MINGW, fracturing the expectations of the Git userbase.

Wouldn't it be better to declare "On other people's Git, @/foo is
just as normal a branch name as a/foo, but on MINGW @/foo cannot be
used" by skipping some tests using prerequisites instead?

As Eric points out, this is not so much a behavior on Git as of the MSYS2
Bash. In fact, if you call `git.exe checkout -b @/at-test` from a cmd
window, it works just as advertised.

But your comment made me inspect the entire t9100 again, wondering why
things work when we copy the contents instead of symlinking them. And you
know what, even if I could have sworn that I verified for every patch in
this series that it is actually necessary to pass the test suite, it is
*not* necessary.

So I backed it out and it won't be part of v2 anymore.

Ciao,
Dscho

Hi Junio,

On Mon, 25 Jan 2016, Johannes Schindelin wrote:

On Sun, 24 Jan 2016, Junio C Hamano wrote:

quoted

Johannes Schindelin [off-list ref] writes:

quoted

From: Thomas Braun <redacted>

A string of the form "@/abcd" is considered a file path
by the msys layer and therefore translated to a Windows path.

Here the trick is to double the slashes.

The MSYS2 patch translation can be studied by calling

	test-path-utils print_path <path>

Signed-off-by: Thomas Braun <redacted>
Signed-off-by: Johannes Schindelin <redacted>
---

This feels wrong.

The point of this test is that you can ask to checkout a branch
whose name is a strangely looking "@/at-test", and a ref whose name
is "refs/heads/@/at-test" indeed is created.

The current "checkout" may be lazy and not signal an error for a
branch name with two consecutive slashes, but I wouldn't be
surprised if we tighten that later, and more importantly, I do not
think we ever promised users if you asked a branch "a//b" to be
created, we would create "refs/heads/a/b".

The new test hardcodes and promises such an incompatible behaviour,
i.e. a request to create "@//b" results in "@/b" created, only to
users on MINGW, fracturing the expectations of the Git userbase.

Wouldn't it be better to declare "On other people's Git, @/foo is
just as normal a branch name as a/foo, but on MINGW @/foo cannot be
used" by skipping some tests using prerequisites instead?

As Eric points out, this is not so much a behavior on Git as of the MSYS2
Bash. In fact, if you call `git.exe checkout -b @/at-test` from a cmd
window, it works just as advertised.

But your comment made me inspect the entire t9100 again, wondering why
things work when we copy the contents instead of symlinking them. And you
know what, even if I could have sworn that I verified for every patch in
this series that it is actually necessary to pass the test suite, it is
*not* necessary.

So I backed it out and it won't be part of v2 anymore.

Whoops. This was meant to be a comment on your comment on 12/19. I'll
reply to the appropriate mail...

As to the patch 13/19 that we are discussing here, I agree that it is
better to simply skip the test with the offending argument. See

	https://github.com/dscho/git/commit/ca5edbe

Ciao,
Dscho