Re: [PATCH] t/Makefile: make sure that file names are truly platform-independent

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 16, 2016, 09:53 UTC
Message-ID: <xmqqmvkddoqi.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <alpine.DEB.2.20.1608161035320.4924@virtualbox>

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

Show 6 quoted lines

>> > > By the way, doesn't ls-files take pathspec glob, saving one extra
>> > > process to run grep?
>> 
>> I specifically did not do that, sorry for omitting the rationale from the
>> commit message. The reason why I have that grep is so that the backslash
>> can also catch non-ASCII characters, such as "Hellö-Jüniö".

That's clever and I did miss that. It deserves an in-code comment next to the use of separate grep to prevent others from making the same mistake in the future. Having an explanation in the log message would help but not sufficient for a subtle construct like this.

The trick to catch non-ascii depends on core.quotepath set to true (which is the default). You would need to run ls-files with an explicit "-c core.quotepath=false" to be safe.

It also means that the addition of HT to the grep pattern I suggested is not necessary, because ls-files will always use "a\tb" form for a pathname with HT, and you will catch it by looking for the backslash. You can lose '"' from the grep pattern for the same reason.

Two other tricky things are

 (1) the test may be run inside a tarball extract and "git ls-files"
     may not report the pathnames in t/.

 (2) the user may not have a working Git yet in her path.

The invocation of "git -c core.quotepath=false ls-files" needs to at least have 2>/dev/null to squelch an unnecessary error message. In such a scenario, we may miss a new offending pathname, but we will not have false positive and I think that is the best we can do.

Thanks.