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

[Johannes Schindelin]

Hi Junio,

On Mon, 25 Jan 2016, Junio C Hamano wrote:

> Eric Sunshine  writes:
> 
> > On Sun, Jan 24, 2016 at 9:03 PM, Junio C Hamano  wrote:
> >> 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 ...
> > ...
> > 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.
> 
> Ah, OK, thanks for clarifying it.  Presumably you would then use
> "checkout @//b" to switch to it, and "log @//b" to look at its
> hsitory.  When you read "git branch" output and see "@/b" in it, you
> would also not complain thinking "oh I thought I created "@//b", not
> with a single branch!".
> 
> Then no issues on allowing "checkout -b @//b" to create a branch
> "@/b" from me.

I actually disabled the test for MINGW instead, as I agree that we do not
want to test MSYS2 in our regression tests.

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Johannes Schindelin]

Hi Junio,

On Mon, 25 Jan 2016, Johannes Schindelin wrote:

> On Sun, 24 Jan 2016, Junio C Hamano wrote:
> 
> > Johannes Schindelin  writes:
> > 
> > > From: Thomas Braun 
> > >
> > > 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 
> > >
> > > Signed-off-by: Thomas Braun 
> > > Signed-off-by: Johannes Schindelin 
> > > ---
> > 
> > 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Johannes Schindelin]

Hi Junio,

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

> Johannes Schindelin  writes:
> 
> > From: Thomas Braun 
> >
> > 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 
> >
> > Signed-off-by: Thomas Braun 
> > Signed-off-by: Johannes Schindelin 
> > ---
> 
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Eric Sunshine  writes:

> On Sun, Jan 24, 2016 at 9:03 PM, Junio C Hamano  wrote:
>> 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 ...
> ...
> 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.

Ah, OK, thanks for clarifying it.  Presumably you would then use
"checkout @//b" to switch to it, and "log @//b" to look at its
hsitory.  When you read "git branch" output and see "@/b" in it, you
would also not complain thinking "oh I thought I created "@//b", not
with a single branch!".

Then no issues on allowing "checkout -b @//b" to create a branch
"@/b" from me.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Johannes Schindelin  writes:

> From: Thomas Braun 
>
> 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 
>
> Signed-off-by: Thomas Braun 
> Signed-off-by: Johannes Schindelin 
> ---

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?


>  t/t1508-at-combinations.sh | 13 +++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh
> index 078e119..1d9fd7b 100755
> --- a/t/t1508-at-combinations.sh
> +++ b/t/t1508-at-combinations.sh
> @@ -29,13 +29,22 @@ fail() {
>   "$@" failure
>  }
>  
> +if test_have_prereq MINGW
> +then
> + # MSYS2 interprets `@/abc` to be a file list, and wants to substitute
> + # the Unix-y path with a Windows one (e.g. @C:\msys64\abc)
> + AT_SLASH=@//at-test
> +else
> + AT_SLASH=@/at-test
> +fi
> +
>  test_expect_success 'setup' '
>   test_commit master-one &&
>   test_commit master-two &&
>   git checkout -b upstream-branch &&
>   test_commit upstream-one &&
>   test_commit upstream-two &&
> - git checkout -b @/at-test &&
> + git checkout -b $AT_SLASH &&
>   git checkout -b @@/at-test &&
>   git checkout -b @at-test &&
>   git checkout -b old-branch &&
> @@ -64,7 +73,7 @@ check "@{-1}@{u}@{1}" commit master-one
>  check "@" commit new-two
>  check "@@{u}" ref refs/heads/upstream-branch
>  check "@@/at-test" ref refs/heads/@@/at-test
> -check "@/at-test" ref refs/heads/@/at-test
> +check "$AT_SLASH" ref refs/heads/@/at-test
>  check "@at-test" ref refs/heads/@at-test
>  nonsense "@{u}@{-1}"
>  nonsense "@{0}@{0}"
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Eric Sunshine]

On Sun, Jan 24, 2016 at 9:03 PM, Junio C Hamano  wrote:
> Johannes Schindelin  writes:
>> 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 
>
> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html