git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* Re: [ANNOUNCE] Git v2.16.0-rc1
  2018-01-05 23:30 [ANNOUNCE] Git v2.16.0-rc1 Junio C Hamano
@ 2018-01-06 12:59 ` Johannes Schindelin
  2018-01-06 22:01   ` Johannes Schindelin
  0 siblings, 1 reply; 8+ messages in thread
From: Johannes Schindelin @ 2018-01-06 12:59 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, git-packagers, git-for-windows

Hi team,

On Fri, 5 Jan 2018, Junio C Hamano wrote:

> A release candidate Git v2.16.0-rc1 is now available for testing
> at the usual places.  It is comprised of 455 non-merge commits
> since v2.15.0, contributed by 79 people, 23 of which are new faces.

I rebased Git for Windows' thicket of patch series on top, and I already
got this when running t0021:

	# failed 10 among 26 test(s)
	1..26

It also seems that the v2.16.0-rc1 *without* Git for Windows' patches on
top displays the very same problem: the test suite does *not* pass on
Windows.

As it is, I am not willing to wreck my weekend, so this will have to wait
until Monday (and Git for Windows v2.16.0-rc1 with it).

Ciao,
Johannes

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

* Re: [ANNOUNCE] Git v2.16.0-rc1
  2018-01-06 12:59 ` Johannes Schindelin
@ 2018-01-06 22:01   ` Johannes Schindelin
  2018-01-09 21:42     ` Junio C Hamano
  0 siblings, 1 reply; 8+ messages in thread
From: Johannes Schindelin @ 2018-01-06 22:01 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, git-packagers, git-for-windows

Hi again,

On Sat, 6 Jan 2018, Johannes Schindelin wrote:

> On Fri, 5 Jan 2018, Junio C Hamano wrote:
> 
> > A release candidate Git v2.16.0-rc1 is now available for testing
> > at the usual places.  It is comprised of 455 non-merge commits
> > since v2.15.0, contributed by 79 people, 23 of which are new faces.
> 
> I rebased Git for Windows' thicket of patch series on top, and I already
> got this when running t0021:
> 
> 	# failed 10 among 26 test(s)
> 	1..26

I actually was able to figure it out: it was the GITPERLLIB problem I
described earlier, where we run (once again) into trouble with Git's
expectations that everything behaves like Linux, including colon-separated
path lists.

The test suite passed, so I kicked off a build which should be available
soon at:

https://github.com/git-for-windows/git/releases/tag/v2.16.0-rc1.windows.1

The patch to work around the GITPERLLIB issue looks like this:

-- snipsnap --
Subject: [PATCH] mingw: handle GITPERLLIB in t0021 in a Windows-compatible
way

Git's assumption that all path lists are colon-separated is not only
wrong on Windows, it is not even an assumption that is compatible with
POSIX.

In the interest of time, let's not try to fix this properly but simply
work around the obvious breakage on Windows, where the MSYS2 Bash used
by Git for Windows to interpret the Git's Unix shell scripts will
automagically convert path lists in the environment to
semicolon-separated lists of Windows paths (with drive letter and the
corresponding colon and all that jazz).

In other words, we simply look whether there is a semicolon in
GITPERLLIB and split by semicolons if found instead of colons. This is
not fool-proof, of course, as the path list could consist of a single
path. But that is not the case in Git for Windows' test suite, there are
always two paths in GITPERLLIB.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t0021/rot13-filter.pl | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
index f1678851de9..470107248eb 100644
--- a/t/t0021/rot13-filter.pl
+++ b/t/t0021/rot13-filter.pl
@@ -31,7 +31,22 @@
 #
 
 use 5.008;
-use lib (split(/:/, $ENV{GITPERLLIB}));
+sub gitperllib {
+	# Git assumes that all path lists are Unix-y colon-separated ones. But
+	# when the Git for Windows executes the test suite, its MSYS2 Bash
+	# calls git.exe, and colon-separated path lists are converted into
+	# Windows-y semicolon-separated lists of *Windows* paths (which
+	# naturally contain a colon after the drive letter, so splitting by
+	# colons simply does not cut it).
+	#
+	# Detect semicolon-separated path list and handle them appropriately.
+
+	if ($ENV{GITPERLLIB} =~ /;/) {
+		return split(/;/, $ENV{GITPERLLIB});
+	}
+	return split(/:/, $ENV{GITPERLLIB});
+}
+use lib (gitperllib());
 use strict;
 use warnings;
 use IO::File;
-- 
2.16.0.rc0.windows.1



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

* Re: [ANNOUNCE] Git v2.16.0-rc1
  2018-01-06 22:01   ` Johannes Schindelin
@ 2018-01-09 21:42     ` Junio C Hamano
  2018-01-09 23:55       ` Junio C Hamano
  2018-01-10 17:37       ` Johannes Schindelin
  0 siblings, 2 replies; 8+ messages in thread
From: Junio C Hamano @ 2018-01-09 21:42 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: git, git-packagers, git-for-windows

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
> index f1678851de9..470107248eb 100644
> --- a/t/t0021/rot13-filter.pl
> +++ b/t/t0021/rot13-filter.pl
> @@ -31,7 +31,22 @@
>  #
>  
>  use 5.008;
> -use lib (split(/:/, $ENV{GITPERLLIB}));
> +sub gitperllib {
> +...
> +	if ($ENV{GITPERLLIB} =~ /;/) {
> +		return split(/;/, $ENV{GITPERLLIB});
> +	}
> +	return split(/:/, $ENV{GITPERLLIB});
> +}

This cannot be the whole story for a few reasons.

 - In t/test-lib.sh we see this:

   GITPERLLIB="$GIT_BUILD_DIR"/perl/blib/lib:"$GIT_BUILD_DIR"/perl/blib/arch/auto/Git
   export GITPERLLIB

   If this part wants to split with ';', then the joining needs to
   be done with ';' to match, no?

 - In addition to t0021, there are similar split with colon in 0202,
   9000 and 9700, yet I am getting the feeling that you observed the
   issue only in0021, to which I do not think of a good explanation
   why.


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

* Re: [ANNOUNCE] Git v2.16.0-rc1
  2018-01-09 21:42     ` Junio C Hamano
@ 2018-01-09 23:55       ` Junio C Hamano
  2018-01-10 17:40         ` Johannes Schindelin
  2018-01-10 17:37       ` Johannes Schindelin
  1 sibling, 1 reply; 8+ messages in thread
From: Junio C Hamano @ 2018-01-09 23:55 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: git, git-packagers, git-for-windows

Junio C Hamano <gitster@pobox.com> writes:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
>> index f1678851de9..470107248eb 100644
>> --- a/t/t0021/rot13-filter.pl
>> +++ b/t/t0021/rot13-filter.pl
>> @@ -31,7 +31,22 @@
>>  #
>>  
>>  use 5.008;
>> -use lib (split(/:/, $ENV{GITPERLLIB}));
>> +sub gitperllib {
>> +...
>> +	if ($ENV{GITPERLLIB} =~ /;/) {
>> +		return split(/;/, $ENV{GITPERLLIB});
>> +	}
>> +	return split(/:/, $ENV{GITPERLLIB});
>> +}
>
> This cannot be the whole story for a few reasons.
>
>  - In t/test-lib.sh we see this:
>
>    GITPERLLIB="$GIT_BUILD_DIR"/perl/blib/lib:"$GIT_BUILD_DIR"/perl/blib/arch/auto/Git
>    export GITPERLLIB
>
>    If this part wants to split with ';', then the joining needs to
>    be done with ';' to match, no?
>
>  - In addition to t0021, there are similar split with colon in 0202,
>    9000 and 9700, yet I am getting the feeling that you observed the
>    issue only in0021, to which I do not think of a good explanation
>    why.

This somehow vaguely rang a bell, and I dug this thing up from the
archive, [*1*] which ended like so:

    >> In our C code, we have "#define PATH_SEP ';'", and encourage
    >> our code to be careful and use it.  Is there something
    >> similar for Perl scripts, I wonder.
    >>
    > We probably should find a better solution to allow this to
    > work with windows style paths...? I know that python provides
    > os.pathsep, but I haven't seen an equivalent for perl yet.
    >
    > The Env[1] core modules suggests using
    > $Config::Config{path_sep}[2]..  maybe we should be using this?

    I was testing this recently on the Perl included with Git for
    Windows and it returns : for the path separator even though it's
    on Windows, so I don't think that would work. The Perl in Git
    for Windows seems to want UNIX-style inputs (something Dscho
    seemed to allude to in his response earlier.). I'm not sure why
    it's that way, but he probably knows.

Your initial response in this thread made it sound as if -rc1 is the
only thing that changed, but looking at the differences between -rc0
and -rc1, which does not touch t0021 or any other instances of
"split(/:/, $ENV{GITPERLLIB})", I am wondering if it is possible
that perhaps the way Perl is built for GfW has been changed recently
and we can safely and sanely use $Config::Config{path_sep} (contrary
to what was found in late Oct in the message quoted above) now?

In any case, I'd prefer this issue to be resolved properly before
-rc2; a patch to t0021/rot13-filter.pl alone does not smell like a
"proper solution" that is based on the understanding of the root
cause (and that is why I spent time digging the list archive).

Thanks.


[Reference]

*1* https://public-inbox.org/git/CAGyf7-EjKaHgwkN9trO4mFvba9odbWCzA9Jh0Pk6ZE6FOskOYg@mail.gmail.com/




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

* Re: [ANNOUNCE] Git v2.16.0-rc1
  2018-01-09 21:42     ` Junio C Hamano
  2018-01-09 23:55       ` Junio C Hamano
@ 2018-01-10 17:37       ` Johannes Schindelin
  2018-01-10 17:54         ` [git-for-windows] " Johannes Sixt
  1 sibling, 1 reply; 8+ messages in thread
From: Johannes Schindelin @ 2018-01-10 17:37 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, git-packagers, git-for-windows

Hi Junio,

On Tue, 9 Jan 2018, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
> > index f1678851de9..470107248eb 100644
> > --- a/t/t0021/rot13-filter.pl
> > +++ b/t/t0021/rot13-filter.pl
> > @@ -31,7 +31,22 @@
> >  #
> >  
> >  use 5.008;
> > -use lib (split(/:/, $ENV{GITPERLLIB}));
> > +sub gitperllib {
> > +...
> > +	if ($ENV{GITPERLLIB} =~ /;/) {
> > +		return split(/;/, $ENV{GITPERLLIB});
> > +	}
> > +	return split(/:/, $ENV{GITPERLLIB});
> > +}
> 
> This cannot be the whole story for a few reasons.
> 
>  - In t/test-lib.sh we see this:
> 
>    GITPERLLIB="$GIT_BUILD_DIR"/perl/blib/lib:"$GIT_BUILD_DIR"/perl/blib/arch/auto/Git
>    export GITPERLLIB
> 
>    If this part wants to split with ';', then the joining needs to
>    be done with ';' to match, no?

No.

It is a lot more complicated than that. As you know, on Linux there is
this implicit assumption that path lists are colon-separated. As a
consequence, Cygwin does the same (because it would be too hard to port
all those Linux/Unix projects to stop assuming colon-separated path lists,
right?)

This is what Cygwin's Bash does (and hence the MSYS2 Bash used by Git for
Windows, too).

Then the MSYS2 Bash calls git.exe, which is *not* an MSYS2 program, hence
the MSYS2 runtime knows that it has to convert the path lists to Windows
paths separated by semicolons.

The next thing happening in our case is that the Perl script is called
from git.exe. Now, the MSYS2 runtime (implicitly spun up by the MSYS2 Perl
interpreter) does *not* convert those path lists back to Unix-like paths
separated by colons.

And that's why the Unix shell script can happily construct the
colon-separated list, and the Perl script will *still* receive the
semicolon-separated version of it.

>  - In addition to t0021, there are similar split with colon in 0202,
>    9000 and 9700, yet I am getting the feeling that you observed the
>    issue only in0021, to which I do not think of a good explanation
>    why.

Here is the good explanation: t0021 relies on a Perl package that is not
yet installed. t0202 relies on Git::I18N, of which there is a version
installed in Git for Windows' SDK. (I do not bother to slow down the test
runs by the Subversion tests, I always skip all of them, that's why t9*
does not matter to me.)

Granted, that is a bug, and an old one at that: the test suite should test
what is in the current revision, not what happens to be installed into the
system locations.

But this late in the game, I am really uncomfortable with the idea to rush
through an equivalent fix to all Perl scripts: who knows what it breaks?

Ciao,
Dscho

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

* Re: [ANNOUNCE] Git v2.16.0-rc1
  2018-01-09 23:55       ` Junio C Hamano
@ 2018-01-10 17:40         ` Johannes Schindelin
  2018-01-10 20:09           ` Junio C Hamano
  0 siblings, 1 reply; 8+ messages in thread
From: Johannes Schindelin @ 2018-01-10 17:40 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, git-packagers, git-for-windows

Hi Junio,

On Tue, 9 Jan 2018, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >
> >> diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
> >> index f1678851de9..470107248eb 100644
> >> --- a/t/t0021/rot13-filter.pl
> >> +++ b/t/t0021/rot13-filter.pl
> >> @@ -31,7 +31,22 @@
> >>  #
> >>  
> >>  use 5.008;
> >> -use lib (split(/:/, $ENV{GITPERLLIB}));
> >> +sub gitperllib {
> >> +...
> >> +	if ($ENV{GITPERLLIB} =~ /;/) {
> >> +		return split(/;/, $ENV{GITPERLLIB});
> >> +	}
> >> +	return split(/:/, $ENV{GITPERLLIB});
> >> +}
> >
> > This cannot be the whole story for a few reasons.
> >
> >  - In t/test-lib.sh we see this:
> >
> >    GITPERLLIB="$GIT_BUILD_DIR"/perl/blib/lib:"$GIT_BUILD_DIR"/perl/blib/arch/auto/Git
> >    export GITPERLLIB
> >
> >    If this part wants to split with ';', then the joining needs to
> >    be done with ';' to match, no?
> >
> >  - In addition to t0021, there are similar split with colon in 0202,
> >    9000 and 9700, yet I am getting the feeling that you observed the
> >    issue only in0021, to which I do not think of a good explanation
> >    why.
> 
> This somehow vaguely rang a bell, and I dug this thing up from the
> archive, [*1*] which ended like so:
> 
>     >> In our C code, we have "#define PATH_SEP ';'", and encourage
>     >> our code to be careful and use it.  Is there something
>     >> similar for Perl scripts, I wonder.
>     >>
>     > We probably should find a better solution to allow this to
>     > work with windows style paths...? I know that python provides
>     > os.pathsep, but I haven't seen an equivalent for perl yet.
>     >
>     > The Env[1] core modules suggests using
>     > $Config::Config{path_sep}[2]..  maybe we should be using this?
> 
>     I was testing this recently on the Perl included with Git for
>     Windows and it returns : for the path separator even though it's
>     on Windows, so I don't think that would work. The Perl in Git
>     for Windows seems to want UNIX-style inputs (something Dscho
>     seemed to allude to in his response earlier.). I'm not sure why
>     it's that way, but he probably knows.
> 
> Your initial response in this thread made it sound as if -rc1 is the
> only thing that changed, but looking at the differences between -rc0
> and -rc1, which does not touch t0021 or any other instances of
> "split(/:/, $ENV{GITPERLLIB})", I am wondering if it is possible
> that perhaps the way Perl is built for GfW has been changed recently
> and we can safely and sanely use $Config::Config{path_sep} (contrary
> to what was found in late Oct in the message quoted above) now?

No, the only thing that changed was the introduction of Git::Packet (and
t0021/*.perl uses that). And that Perl module is not yet installed.

Granted, we tested incorrect versions of those modules, but this late in
the -rc phase, I would prefer to be cautious. People might be relying on
the current bug.

OTOH I might be way too pessimistic and we should essentially replicate
that `sub gitperllib` trick in *all* of our Perl scripts.

What do you think?

Ciao,
Dscho

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

* Re: [git-for-windows] Re: [ANNOUNCE] Git v2.16.0-rc1
  2018-01-10 17:37       ` Johannes Schindelin
@ 2018-01-10 17:54         ` " Johannes Sixt
  0 siblings, 0 replies; 8+ messages in thread
From: Johannes Sixt @ 2018-01-10 17:54 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Junio C Hamano, git, git-packagers, git-for-windows

Am 10.01.2018 um 18:37 schrieb Johannes Schindelin:
> On Tue, 9 Jan 2018, Junio C Hamano wrote:
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>
>>> diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
>>> index f1678851de9..470107248eb 100644
>>> --- a/t/t0021/rot13-filter.pl
>>> +++ b/t/t0021/rot13-filter.pl
>>> @@ -31,7 +31,22 @@
>>>   #
>>>   
>>>   use 5.008;
>>> -use lib (split(/:/, $ENV{GITPERLLIB}));
>>> +sub gitperllib {
>>> +...
>>> +	if ($ENV{GITPERLLIB} =~ /;/) {
>>> +		return split(/;/, $ENV{GITPERLLIB});
>>> +	}
>>> +	return split(/:/, $ENV{GITPERLLIB});
>>> +}
>>
>> This cannot be the whole story for a few reasons.
>>
>>   - In t/test-lib.sh we see this:
>>
>>     GITPERLLIB="$GIT_BUILD_DIR"/perl/blib/lib:"$GIT_BUILD_DIR"/perl/blib/arch/auto/Git
>>     export GITPERLLIB
>>
>>     If this part wants to split with ';', then the joining needs to
>>     be done with ';' to match, no?
> 
> No.
> 
> It is a lot more complicated than that. As you know, on Linux there is
> this implicit assumption that path lists are colon-separated. As a
> consequence, Cygwin does the same (because it would be too hard to port
> all those Linux/Unix projects to stop assuming colon-separated path lists,
> right?)
> 
> This is what Cygwin's Bash does (and hence the MSYS2 Bash used by Git for
> Windows, too).
> 
> Then the MSYS2 Bash calls git.exe, which is *not* an MSYS2 program, hence
> the MSYS2 runtime knows that it has to convert the path lists to Windows
> paths separated by semicolons.
> 
> The next thing happening in our case is that the Perl script is called
> from git.exe. Now, the MSYS2 runtime (implicitly spun up by the MSYS2 Perl
> interpreter) does *not* convert those path lists back to Unix-like paths
> separated by colons.

But this is a bug in MSYS2, isn't it? The MSYS2 runtime should detect 
that it was not invoked by some other MSYS2 process. The MSYS2 startup 
sequence should assume in this case that the environment is 
Windows-style and convert to POSIX before it calls into perl's main().

> And that's why the Unix shell script can happily construct the
> colon-separated list, and the Perl script will *still* receive the
> semicolon-separated version of it.
> 
>>   - In addition to t0021, there are similar split with colon in 0202,
>>     9000 and 9700, yet I am getting the feeling that you observed the
>>     issue only in0021, to which I do not think of a good explanation
>>     why.
> 
> Here is the good explanation: t0021 relies on a Perl package that is not
> yet installed. t0202 relies on Git::I18N, of which there is a version
> installed in Git for Windows' SDK. (I do not bother to slow down the test
> runs by the Subversion tests, I always skip all of them, that's why t9*
> does not matter to me.)

t0202 and the t9* cases are different because perl is invoked by bash 
directly (AFAICS), without a non-MSYS2 process between them. There is no 
difference when the path conversion is omitted in this case by design or 
due to a bug.

-- Hannes

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

* Re: [ANNOUNCE] Git v2.16.0-rc1
  2018-01-10 17:40         ` Johannes Schindelin
@ 2018-01-10 20:09           ` Junio C Hamano
  0 siblings, 0 replies; 8+ messages in thread
From: Junio C Hamano @ 2018-01-10 20:09 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: git, git-packagers, git-for-windows

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> No, the only thing that changed was the introduction of Git::Packet (and
> t0021/*.perl uses that). And that Perl module is not yet installed.

Ahh, that is the difference among other users of split(/:/,
$ENV{GITPERLLIB}).  Scripts other than 0021 may be using installed
ones and using the wrong separator does not affect them.

> Granted, we tested incorrect versions of those modules, but this late in
> the -rc phase, I would prefer to be cautious. People might be relying on
> the current bug.

People meaning those who run "make test"?

In any case, the patch to 0021 as posted sounds like the most
conservative thing to do at this point, even though we would
definitely want to revisit it and clean it up after the release.  As
long as the reason why only 0021 wants the special case while others
are OK is understood, I am OK with the patch ;-)

Thanks.

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

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-05 23:30 [ANNOUNCE] Git v2.16.0-rc1 Junio C Hamano
2018-01-06 12:59 ` Johannes Schindelin
2018-01-06 22:01   ` Johannes Schindelin
2018-01-09 21:42     ` Junio C Hamano
2018-01-09 23:55       ` Junio C Hamano
2018-01-10 17:40         ` Johannes Schindelin
2018-01-10 20:09           ` Junio C Hamano
2018-01-10 17:37       ` Johannes Schindelin
2018-01-10 17:54         ` [git-for-windows] " Johannes Sixt

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

Archives are clonable:
	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

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.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

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