Re: [PATCH 0/6] Create Git/Packet.pm

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Oct 27, 2017, 15:05 UTC
Message-ID: <alpine.DEB.2.21.1.1710271626410.6482@virtualbox>
In-Reply-To: <xmqq4lqmfoy7.fsf@gitster.mtv.corp.google.com>

Hi Junio,

On Thu, 26 Oct 2017, Junio C Hamano wrote:

Show 19 quoted lines

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Note that the correct blib path starts with `C:\BuildAgent\_work` and
> > the line
> >
> > 	use lib (split(/:/, $ENV{GITPERLLIB}));
> >
> > splits off the drive letter from the rest of the path. Obviously, this
> > fails to Do The Right Thing, and simply points to Yet Another Portability
> > Problem with Git's reliance on Unix scripting.
> 
> 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.
> 
> I notice that t/{t0202,t9000,t9700}/test.pl share the same
> split(/:/, $ENV{GITPERLLIB}); forcing this specific variable to use
> the non-platform convention to accomodate the use of split(/:/)
> certainly is a workaround, but it does feel dirty.

It is not only dirty, it *still* does not work with that workaround: Note that GITPERLLIB is *also* set in the bin-wrappers/*...

And even then, it does not work: somewhere along the way, the path is converted to a Windows path with backslashes, and for some reason MSYS2 Perl fails to handle that.

The best workaround I found in Git's source code (yes, it took me 2h to investigate this littly problem, but at least I got an in-depth understanding of the issue) was to pass BLIB_DIR instead, and not perform a split but generate the two paths to include in Perl instead. Of course, that would break out-of-tree usage of GITPERLLIB...

That's why I settled on the out-of-tree workaround that Windows contributors will have to somehow figure out (as if it was not hard enough already to contribute to Git for Windows...).

> It is hard to imagine that we were the first people who wants to
> split the value of a variable into a list, where the value is a list
> of paths, concatenated into a single string with a delimiter that
> may be platform specific.

No, the test suite has plenty of use cases for that. It usually works.

The problem is that t0021 contains very complex code that goes back and forth between the C layer and the scripted layer. At one stage, the pseudo-Unix paths are converted to Windows paths, with drive prefix and backslashes, separated by semicolons. And somewhere along the lines, this cannot be converted back.

I *think* that it happens when the bin-wrapper for git.exe is executed from inside Git itself, or some such.

Show 5 quoted lines

> I wonder if we are going against a best practice established in the Perl
> world, simply because we don't know about it (i.e. basically, it would
> say "don't split at a colon because not all world is Unix; use
> $this_module instead", similar to "don't split at a slash, use
> File::Spec instead to extract path components").

We go against best practices by having crucial parts of Git implemented as Perl scripts. But you knew that ;-)

Ciao, Dscho