[PATCH] fixup! log: add exhaustive tests for pattern style options & config

[PATCH] fixup! log: add exhaustive tests for pattern style options & config

[Johannes Schindelin]

On Windows, `(1|2)` is not a valid file name, and therefore the tag
cannot be created as expected by the new test.

So simply skip this test on Windows.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Published-As: https://github.com/dscho/git/releases/tag/exhaustive-grep-tests-fixup-v1
Fetch-It-Via: git fetch https://github.com/dscho/git exhaustive-grep-tests-fixup-v1

 t/t4202-log.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 36321f19061..6f108e99b7b 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -296,7 +296,7 @@ test_expect_success 'log with grep.patternType configuration and command line' '
 	test_cmp expect actual
 '
 
-test_expect_success 'log with various grep.patternType configurations & command-lines' '
+test_expect_success !MINGW 'log with various grep.patternType configurations & command-lines' '
 	git init pattern-type &&
 	(
 		cd pattern-type &&

base-commit: 3760a479060228867a31eed443334b30124465b9
-- 
2.12.2.windows.2.800.gede8f145e06

Re: [PATCH] fixup! log: add exhaustive tests for pattern style options & config

[Junio C Hamano]

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

> On Windows, `(1|2)` is not a valid file name, and therefore the tag
> cannot be created as expected by the new test.
>
> So simply skip this test on Windows.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---

Will queue as an emergency fix on 'pu' for now, but I'd expect
(read: ask) Ævar to squash this in when the series is rerolled.

Thanks.

> Published-As: https://github.com/dscho/git/releases/tag/exhaustive-grep-tests-fixup-v1
> Fetch-It-Via: git fetch https://github.com/dscho/git exhaustive-grep-tests-fixup-v1
>
>  t/t4202-log.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index 36321f19061..6f108e99b7b 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -296,7 +296,7 @@ test_expect_success 'log with grep.patternType configuration and command line' '
>  	test_cmp expect actual
>  '
>  
> -test_expect_success 'log with various grep.patternType configurations & command-lines' '
> +test_expect_success !MINGW 'log with various grep.patternType configurations & command-lines' '
>  	git init pattern-type &&
>  	(
>  		cd pattern-type &&
>
> base-commit: 3760a479060228867a31eed443334b30124465b9

Re: [PATCH] fixup! log: add exhaustive tests for pattern style options & config

[Jonathan Nieder]

Johannes Schindelin wrote:

> On Windows, `(1|2)` is not a valid file name, and therefore the tag
> cannot be created as expected by the new test.
>
> So simply skip this test on Windows.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

I wouldn't be surprised if there are filesystems used places other
than MINGW that also can't handle this test.  Isn't this what some
tests use a FUNNYNAMES prerequisite for?

In this example, it's the pipe that's not allowed, not the
parenthesis, right?  (At least I have some memories of naming files
with some parentheses.)  Would something like

	test PIPE_IN_FILENAME '
		>"a|b" &&
		test -f "a|b"
	'

work?

Thanks,
Jonathan

Re: [PATCH] fixup! log: add exhaustive tests for pattern style options & config

[Ævar Arnfjörð Bjarmason]

On Fri, May 12, 2017 at 12:50 PM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> On Windows, `(1|2)` is not a valid file name, and therefore the tag
> cannot be created as expected by the new test.
>
> So simply skip this test on Windows.

Thanks for the hotfix. I'll fix this in my v2, but do it differently
in such a way that I can still run these tests on windows.

I.e. the actual test here just needs these odd characters in the
commit message. It's just an unintended implementation detail of
test_commit that a tag is being created.

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> Published-As: https://github.com/dscho/git/releases/tag/exhaustive-grep-tests-fixup-v1
> Fetch-It-Via: git fetch https://github.com/dscho/git exhaustive-grep-tests-fixup-v1
>
>  t/t4202-log.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index 36321f19061..6f108e99b7b 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -296,7 +296,7 @@ test_expect_success 'log with grep.patternType configuration and command line' '
>         test_cmp expect actual
>  '
>
> -test_expect_success 'log with various grep.patternType configurations & command-lines' '
> +test_expect_success !MINGW 'log with various grep.patternType configurations & command-lines' '
>         git init pattern-type &&
>         (
>                 cd pattern-type &&
>
> base-commit: 3760a479060228867a31eed443334b30124465b9
> --
> 2.12.2.windows.2.800.gede8f145e06

Re: [PATCH] fixup! log: add exhaustive tests for pattern style options & config

[Ævar Arnfjörð Bjarmason]

On Sat, May 13, 2017 at 1:44 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Johannes Schindelin wrote:
>
>> On Windows, `(1|2)` is not a valid file name, and therefore the tag
>> cannot be created as expected by the new test.
>>
>> So simply skip this test on Windows.
>>
>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>> ---
>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
>
> I wouldn't be surprised if there are filesystems used places other
> than MINGW that also can't handle this test.  Isn't this what some
> tests use a FUNNYNAMES prerequisite for?
>
> In this example, it's the pipe that's not allowed, not the
> parenthesis, right?  (At least I have some memories of naming files
> with some parentheses.)  Would something like
>
>         test PIPE_IN_FILENAME '
>                 >"a|b" &&
>                 test -f "a|b"
>         '
>
> work?

It would, but as indicated upthread I'll just amend this so the odd
tag/filename won't be needed, since the test doesn't actually use
that.

(B.t.w. I meant "[odd looking] tag or file" in my last E-Mail in this
thread, not just "[odd looking tag]")>

Re: [PATCH] fixup! log: add exhaustive tests for pattern style options & config

[Johannes Schindelin]

Hi Jonathan,

On Fri, 12 May 2017, Jonathan Nieder wrote:

> Johannes Schindelin wrote:
> 
> > On Windows, `(1|2)` is not a valid file name, and therefore the tag
> > cannot be created as expected by the new test.
> >
> > So simply skip this test on Windows.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> 
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
> 
> I wouldn't be surprised if there are filesystems used places other
> than MINGW that also can't handle this test.  Isn't this what some
> tests use a FUNNYNAMES prerequisite for?

Some tests do that. And they all implement that prereq themselves,
differently.

t3600 tries to create a file whose name contains a tab.

t4135 tries to create a file whose name contains a tab, but only if the
MINGW prereq is set.

t9903 tries to create a file whose name contains a newline.

It seems, therefore, that the existing FUNNYNAMES are already in
disagreement, and as you point out below, the file name in question
contains neither a tab nor a newline. So we would introduce *yet another*
disagreeing FUNNYNAMES.

Besides, in some hopefully not so far future, our default refs backend may
not depend on the current platform's file system's idionsyncracies. So
maybe we do not even want the MINGW nor the FUNNYNAMES prereq...

> In this example, it's the pipe that's not allowed, not the
> parenthesis, right?  (At least I have some memories of naming files
> with some parentheses.)

You would be correct in that assumptions. Long names on NTFS can contain
parentheses.

> Would something like
> 
> 	test PIPE_IN_FILENAME '
> 		>"a|b" &&
> 		test -f "a|b"
> 	'
> 
> work?

No.

Remember: on Windows, there is no Unix shell.

(Actually, there is Bash on Ubuntu on Windows, and with the Creators
Update, it became truly awesome, I am a big fan of it. But that is besides
the point here: Git for Windows needs to support Windows versions older
than Windows 10, where Bash on Ubuntu on Windows is simply unavailable.)

So what Git for Windows has to do is quite a burden: we ship with MSYS2,
kind of a light-weight version of Cygwin, to run our shell scripts. (I
would wager a bet that at least a third of the bug reports relate to the
POSIX emulation layer we ship to allow us to run Bash and Perl, but even
that is only half the truth: tracking down those bugs takes easily the
majority of my bug hunting/fixing time, hands down.)

The test suite is no exception. I may have ranted about this about six
dozen times already, so I'll save me the time. Just pretend that you now
read seven long paragraphs describing how much it sucks to run the test
suite on Windows.

Take home message: Unix shell scripting is good for personal use. Don't
use it in applications, not if you want to stay portable. Just don't.

Back to the subject: The MSYS2 emulation layer inherits a neat trick from
Cygwin, where it *can* create file names containing pipe symbols. They
will be quietly mapped into a private UTF-8 page, and when Cygwin or MSYS2
read the file back, the file name maps from this page back to ASCII
transparently.

That strategy is all good and dandy, as long as you stay within the POSIX
emulation layer.

Git for Windows avoids the POSIX emulation layer as much as possible, for
speed, and also for robustness.

Which means that Git does *not* map the file names using said private
UTF-8 code page. And therefore, your test would succeed (because the shell
script would stay within the POSIX emulation layer, which creates that
file using above-mentioned strategy), but Git (being a regular Win32
program) *still* would fail to create said file.

Ciao,
Dscho

Re: [PATCH] fixup! log: add exhaustive tests for pattern style options & config

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Thanks for the hotfix. I'll fix this in my v2, but do it differently
> in such a way that I can still run these tests on windows.
>
> I.e. the actual test here just needs these odd characters in the
> commit message. It's just an unintended implementation detail of
> test_commit that a tag is being created.

My knee-jerk reaction matched Dscho's, but grep is about contents,
and we should be able to test this if we used a sensible tagnames or
didn't use any.  Glad to see somebody can step back and think ;-)

Re: [PATCH] fixup! log: add exhaustive tests for pattern style options & config

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 875 bytes --]

Hi Junio,

On Mon, 15 May 2017, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> 
> > Thanks for the hotfix. I'll fix this in my v2, but do it differently
> > in such a way that I can still run these tests on windows.
> >
> > I.e. the actual test here just needs these odd characters in the
> > commit message. It's just an unintended implementation detail of
> > test_commit that a tag is being created.
> 
> My knee-jerk reaction matched Dscho's, but grep is about contents,
> and we should be able to test this if we used a sensible tagnames or
> didn't use any.  Glad to see somebody can step back and think ;-)

Maybe somebody should step back even further and think even more, as we
could adjust test_commit to mangle the argument into a tag name that is
legal even with a refs backend relying on NTFS.

Ciao,
Dscho

Re: [PATCH] fixup! log: add exhaustive tests for pattern style options & config

[Jonathan Nieder]

Hi,

Johannes Schindelin wrote:
> On Fri, 12 May 2017, Jonathan Nieder wrote:

>> Would something like
>>
>> 	test PIPE_IN_FILENAME '
>> 		>"a|b" &&
>> 		test -f "a|b"
>> 	'
>>
>> work?
[...]
> Back to the subject: The MSYS2 emulation layer inherits a neat trick from
> Cygwin, where it *can* create file names containing pipe symbols. They
> will be quietly mapped into a private UTF-8 page, and when Cygwin or MSYS2
> read the file back, the file name maps from this page back to ASCII
> transparently.
>
> That strategy is all good and dandy, as long as you stay within the POSIX
> emulation layer.
>
> Git for Windows avoids the POSIX emulation layer as much as possible, for
> speed, and also for robustness.
>
> Which means that Git does *not* map the file names using said private
> UTF-8 code page. And therefore, your test would succeed (because the shell
> script would stay within the POSIX emulation layer, which creates that
> file using above-mentioned strategy), but Git (being a regular Win32
> program) *still* would fail to create said file.

Wow.  Thanks for a clear explaination.

I'll be keeping a copy of this message handy for the next time I'm
confused about filename handling in the testsuite on Windows.

Sincerely,
Jonathan

Re: [PATCH] fixup! log: add exhaustive tests for pattern style options & config

[Junio C Hamano]

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

> On Mon, 15 May 2017, Junio C Hamano wrote:
>
>> My knee-jerk reaction matched Dscho's, but grep is about contents,
>> and we should be able to test this if we used a sensible tagnames or
>> didn't use any.  Glad to see somebody can step back and think ;-)
>
> Maybe somebody should step back even further and think even more, as we
> could adjust test_commit to mangle the argument into a tag name that is
> legal even with a refs backend relying on NTFS.

Perhaps, but I am not sure if that is needed.  

The point of the helper is to serve as a simple "we are building a
toy sample history by only adding a one-liner new file" convenience
helper, and I think it is sensible to keep its definition simple.
The callers (like the ones being added in the rerolled patch under
discussion) with special needs can supply tagname when the default
one is not suitable.

In hindsight, perhaps it would have been better if the default for
the helper were _not_ to create any tag (and callers who care about
tags can optionally tell it to add tag, or tag the resulting commit
themselves), but that is lamenting water under the bridge.

Re: [PATCH] fixup! log: add exhaustive tests for pattern style options & config

[Ævar Arnfjörð Bjarmason]

On Tue, May 16, 2017 at 2:46 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> On Mon, 15 May 2017, Junio C Hamano wrote:
>>
>>> My knee-jerk reaction matched Dscho's, but grep is about contents,
>>> and we should be able to test this if we used a sensible tagnames or
>>> didn't use any.  Glad to see somebody can step back and think ;-)
>>
>> Maybe somebody should step back even further and think even more, as we
>> could adjust test_commit to mangle the argument into a tag name that is
>> legal even with a refs backend relying on NTFS.
>
> Perhaps, but I am not sure if that is needed.
>
> The point of the helper is to serve as a simple "we are building a
> toy sample history by only adding a one-liner new file" convenience
> helper, and I think it is sensible to keep its definition simple.
> The callers (like the ones being added in the rerolled patch under
> discussion) with special needs can supply tagname when the default
> one is not suitable.
>
> In hindsight, perhaps it would have been better if the default for
> the helper were _not_ to create any tag (and callers who care about
> tags can optionally tell it to add tag, or tag the resulting commit
> themselves), but that is lamenting water under the bridge.

This works, but I wonder if it's worth it to solve this one-off issue:

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 5ee124332a..4cab67c410 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -195,7 +195,15 @@ test_commit () {
                test_tick
        fi &&
        git ${indir:+ -C "$indir"} commit $signoff -m "$1" &&
-       git ${indir:+ -C "$indir"} tag "${4:-$1}"
+       if test -n "$4"
+       then
+               git ${indir:+ -C "$indir"} tag "$4"
+       elif test -n "$(echo $1 | tr -d A-Za-z0-9/~_.#-)"
+       then
+               error "Implicitly created tag '$1' looks unusual,
probably fails outside *nix"
+       else
+               git ${indir:+ -C "$indir"} tag "$1"
+       fi
 }

 # Call test_merge with the arguments "<message> <commit>", where <commit>