Re: [ANNOUNCE] Git v2.16.0-rc1

[Junio C Hamano]

Johannes Schindelin  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.

Re: [git-for-windows] Re: [ANNOUNCE] Git v2.16.0-rc1

[Johannes Sixt]

Am 10.01.2018 um 18:37 schrieb Johannes Schindelin:

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

Johannes Schindelin  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

Re: [ANNOUNCE] Git v2.16.0-rc1

[Johannes Schindelin]

Hi Junio,

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

> Junio C Hamano  writes:
> 
> > Johannes Schindelin  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

Re: [ANNOUNCE] Git v2.16.0-rc1

[Johannes Schindelin]

Hi Junio,

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

> Johannes Schindelin  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

Re: [ANNOUNCE] Git v2.16.0-rc1

[Junio C Hamano]

Junio C Hamano  writes:

> Johannes Schindelin  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-ejkahgwkn9tro4mfvba9odbwcza9jh0pk6ze6fosk...@mail.gmail.com/

Re: [ANNOUNCE] Git v2.16.0-rc1

[Junio C Hamano]

Johannes Schindelin  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.

Re: [ANNOUNCE] Git v2.16.0-rc1

[Johannes Schindelin]

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

Re: [ANNOUNCE] Git v2.16.0-rc1

[Johannes Schindelin]

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

[ANNOUNCE] Git v2.16.0-rc1

[Junio C Hamano]

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.

The tarballs are found at:

https://www.kernel.org/pub/software/scm/git/testing/

The following public repositories all have a copy of the
'v2.16.0-rc1' tag and the 'master' branch that the tag points at:

  url = https://kernel.googlesource.com/pub/scm/git/git
  url = git://repo.or.cz/alt-git.git
  url = https://github.com/gitster/git

New contributors whose contributions weren't in v2.15.0 are as follows.
Welcome to the Git development community!

  Albert Astals Cid, Antoine Beaupré, Damien Marié, Daniel
  Bensoussan, Florian Klink, Gennady Kupava, Guillaume Castagnino,
  Haaris Mehmood, Hans Jerry Illikainen, Ingo Ruhnke, Jakub
  Bereżański, Jean Carlo Machado, Julien Dusser, J Wyman,
  Kevin, Łukasz Stelmach, Marius Paliga, Olga Telezhnaya,
  Rafael Ascensão, Robert Abel, Robert P. J. Day, Shuyu Wei,
  and Wei Shuyu.

Returning contributors who helped this release are as follows.
Thanks for your continued support.

  Adam Dinwoodie, Ævar Arnfjörð Bjarmason, Alex Vandiver,
  Anders Kaseorg, Andrey Okoshkin, Ann T Ropea, Beat Bolli,
  Ben Peart, Brandon Williams, brian m. carlson, Carlos Martín
  Nieto, Charles Bailey, Christian Couder, Dave Borowitz, Dennis
  Kaarsemaker, Derrick Stolee, Elijah Newren, Emily Xie, Eric
  Sunshine, Eric Wong, Heiko Voigt, Jacob Keller, Jameson Miller,
  Jean-Noel Avila, Jeff Hostetler, Jeff King, Johannes Schindelin,
  Jonathan Nieder, Jonathan Tan, Junio C Hamano, Kaartic Sivaraam,
  Kevin Daudt, Lars Schneider, Liam Beguin, Luke Diamand, Martin
  Ågren, Michael Haggerty, Nicolas Morey-Chaisemartin, Phil
  Hord, Phillip Wood, Pranit Bauva, Prathamesh Chavan, Ramsay
  Jones, Randall S. Becker, Rasmus Villemoes, René Scharfe,
  Simon Ruderich, Stefan Beller, Steffen Prohaska, Stephan Beyer,
  SZEDER Gábor, Thomas Braun, Thomas Gummerer, Todd Zullinger,
  Torsten Bögershausen, and W. Trevor King.



Git 2.16 Release Notes (draft)
==

Backward compatibility notes and other notable changes.

 * Use of an empty string as a pathspec element that is used for
   'everything matches' is now an error.


Updates since v2.15
---

UI, Workflows & Features

 * An empty string as a pathspec element that means "everything"
   i.e. 'git add ""', is now illegal.  We started this by first
   deprecating and warning a pathspec that has such an element in
   2.11 (Nov 2016).

 * A hook script that is set unexecutable is simply ignored.  Git
   notifies when such a file is ignored, unless the message is
   squelched via advice.ignoredHook configuration.

 * "git pull" has been taught to accept "--[no-]signoff" option and
   pass it down to "git merge".

 * The "--push-option=" option to "git push" now defaults to a
   list of strings configured via push.pushOption variable.

 * "gitweb" checks if a directory is searchable with Perl's "-x"
   operator, which can be enhanced by using "filetest 'access'"
   pragma, which now we do.

 * "git stash save" has been deprecated in favour of "git stash push".

 * The set of paths output from "git status --ignored" was tied
   closely with its "--untracked=" option, but now it can be
   controlled more flexibly.  Most notably, a directory that is
   ignored because it is listed to be ignored in the ignore/exclude
   mechanism can be handled differently from a directory that ends up
   to be ignored only because all files in it are ignored.

 * The remote-helper for talking to MediaWiki has been updated to
   truncate an overlong pagename so that ".mw" suffix can still be
   added.

 * The remote-helper for talking to MediaWiki has been updated to
   work with mediawiki namespaces.

 * The "--format=..." option "git for-each-ref" takes learned to show
   the name of the 'remote' repository and the ref at the remote side
   that is affected for 'upstream' and 'push' via "%(push:remotename)"
   and friends.

 * Doc and message updates to teach users "bisect view" is a synonym
   for "bisect visualize".

 * "git bisect run" that did not specify any command to run used to go
   ahead and treated all commits to be tested as 'good'.  This has
   been corrected by making the command error out.

 * The SubmittingPatches document has been converted to produce an
   HTML version via AsciiDoc/Asciidoctor.

 * We learned to talk to watchman to speed up "git status" and other
   operations that need to see which paths have been modified.

 * The "diff" family of commands learned to ignore differences in
   carriage return at the end of line.

 * Places that know about "sendemail.to", like documentation and shell
   completion (in contrib/) have been taught about "sendemail.tocmd",
   too.

 * "git add --renormalize ." is a new and safer way to record the fact