[PATCH] grep: Don't pass a TODO test if REG_STARTEND is supported

[PATCH] grep: Don't pass a TODO test if REG_STARTEND is supported

[Ævar Arnfjörð Bjarmason]

Change the git grep test that utilizes the REG_STARTEND flag so that
it doesn't TODO pass on platforms where REG_STARTEND is supported.

Git's own harness doesn't care, but a TAP harness will report all TODO
tests that pass. Having t7008-grep-binary.sh be the only test (aside
from the test-lib.sh test) that passes a TODO test is distracting.

Before prove(1)'s test summary looked like this:

    All tests successful.

    Test Summary Report
    -------------------
    ./t0000-basic.sh                        (Wstat: 0 Tests: 46 Failed: 0)
      TODO passed:   5
    ./t7008-grep-binary.sh                  (Wstat: 0 Tests: 18 Failed: 0)
      TODO passed:   11
    Files=476, Tests=6071, [...]
    Result: PASS

And now it'll give:

    All tests successful.

    Test Summary Report
    -------------------
    ./t0000-basic.sh                        (Wstat: 0 Tests: 46 Failed: 0)
      TODO passed:   5
    Files=476, Tests=6071,

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t7008-grep-binary.sh |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/t/t7008-grep-binary.sh b/t/t7008-grep-binary.sh
index eb8ca88..6fd2b40 100755
--- a/t/t7008-grep-binary.sh
+++ b/t/t7008-grep-binary.sh
@@ -59,11 +59,16 @@ test_expect_success 'git grep -Fi iLE a' '
 	git grep -Fi iLE a
 '
 
-# This test actually passes on platforms where regexec() supports the
-# flag REG_STARTEND.
-test_expect_failure 'git grep ile a' '
-	git grep ile a
-'
+if git grep ile a
+then
+	# This only passes on platforms where regexec() supports the
+	# REG_STARTEND flag.
+	test_expect_success 'git grep ile a' 'git grep ile a'
+else
+	# On platforms where REG_STARTEND isn't supported we mark the
+	# failure as a TODO.
+	test_expect_failure 'git grep ile a' 'git grep ile a'
+fi
 
 test_expect_failure 'git grep .fi a' '
 	git grep .fi a
-- 
1.7.0.4

Re: [PATCH] grep: Don't pass a TODO test if REG_STARTEND is supported

[Junio C Hamano]

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

> +if git grep ile a
> +then
> +	test_expect_success 'git grep ile a' 'git grep ile a'
> +else
> +	test_expect_failure 'git grep ile a' 'git grep ile a'
> +fi

So if command "X" is known to succeed, we run it inside expect_success
and if not we run it inside expect_failure?

What kind of idiocy is that, I have to wonder...

Re: [PATCH] grep: Don't pass a TODO test if REG_STARTEND is supported

[Ævar Arnfjörð Bjarmason]

On Thu, Jul 8, 2010 at 19:40, Junio C Hamano <gitster@pobox.com> wrote:
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> +if git grep ile a
>> +then
>> +     test_expect_success 'git grep ile a' 'git grep ile a'
>> +else
>> +     test_expect_failure 'git grep ile a' 'git grep ile a'
>> +fi
>
> So if command "X" is known to succeed, we run it inside expect_success
> and if not we run it inside expect_failure?
>
> What kind of idiocy is that, I have to wonder...

Well, the point is to normalize the test suite so that we never have
passing TODO tests if everything's OK.

Re: [PATCH] grep: Don't pass a TODO test if REG_STARTEND is supported

[René Scharfe]

Am 08.07.2010 22:09, schrieb Ævar Arnfjörð Bjarmason:
> On Thu, Jul 8, 2010 at 19:40, Junio C Hamano <gitster@pobox.com> wrote:
>> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>>
>>> +if git grep ile a
>>> +then
>>> +     test_expect_success 'git grep ile a' 'git grep ile a'
>>> +else
>>> +     test_expect_failure 'git grep ile a' 'git grep ile a'
>>> +fi
>>
>> So if command "X" is known to succeed, we run it inside expect_success
>> and if not we run it inside expect_failure?
>>
>> What kind of idiocy is that, I have to wonder...
> 
> Well, the point is to normalize the test suite so that we never have
> passing TODO tests if everything's OK.

In this particular case the tested functionality is not provided accross
all platforms supported by git.  It would be better to fix the actual
issue, namely that the fallback regex lib we provide doesn't support
REG_STARTEND.

René

Re: [PATCH] grep: Don't pass a TODO test if REG_STARTEND is supported

[Ævar Arnfjörð Bjarmason]

On Thu, Jul 8, 2010 at 21:58, René Scharfe <rene.scharfe@lsrfire.ath.cx> wrote:

> In this particular case the tested functionality is not provided accross
> all platforms supported by git.  It would be better to fix the actual
> issue, namely that the fallback regex lib we provide doesn't support
> REG_STARTEND.

We're currently providing the GNU regex library, it seems Mac OS X and
FreeBSD ship with a regex library that has REG_STARTEND (according to
`git log --reverse compat/regex` the current library was added because
of incompatibilities on OSX, but those were since fixed in
310e0216c8).

If the FreeBSD and Mac OS X implementations are good enough we could
just swap the GNU one for the FreeBSD one
(http://www.freebsd.org/cgi/cvsweb.cgi/src/lib/libc/regex/). Is there
any reason not to do this, or would a different regex library be a
better fit?

Re: [PATCH] grep: Don't pass a TODO test if REG_STARTEND is supported

[Junio C Hamano]

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

> On Thu, Jul 8, 2010 at 19:40, Junio C Hamano <gitster@pobox.com> wrote:
>> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>>
>>> +if git grep ile a
>>> +then
>>> +     test_expect_success 'git grep ile a' 'git grep ile a'
>>> +else
>>> +     test_expect_failure 'git grep ile a' 'git grep ile a'
>>> +fi
>>
>> So if command "X" is known to succeed, we run it inside expect_success
>> and if not we run it inside expect_failure?
>>
>> What kind of idiocy is that, I have to wonder...
>
> Well, the point is to normalize the test suite so that we never have
> passing TODO tests if everything's OK.

I do not consider a test that passes under some condition but doesn't
under some other condition "everything is OK".  Marking the test as
"expect failure" as René originally did makes a lot of sense to me.

The quoted patch is even worse as it will _actively_ prevent you from
catching a new error you just introduced while futzing "git grep" on a
platform that used to work.  Your "if" statement will say "ah, grep is
broken", and you will use expect-failure, not because your platform does
not support REG_STARTEND, but because you broke "git grep".

The point of having tests is to help you catch your bugs while you
develop.  A test that turns itself off when the feature it is testing is
broken helps nobody.

So forget about "passing TODO tests", whatever a "TODO test" is.  The
change in question is actively _wrong_.

Re: [PATCH] grep: Don't pass a TODO test if REG_STARTEND is supported

[Ævar Arnfjörð Bjarmason]

On Thu, Jul 15, 2010 at 17:47, Junio C Hamano <gitster@pobox.com> wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> On Thu, Jul 8, 2010 at 19:40, Junio C Hamano <gitster@pobox.com> wrote:
>>> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>>>
>>>> +if git grep ile a
>>>> +then
>>>> +     test_expect_success 'git grep ile a' 'git grep ile a'
>>>> +else
>>>> +     test_expect_failure 'git grep ile a' 'git grep ile a'
>>>> +fi
>>>
>>> So if command "X" is known to succeed, we run it inside expect_success
>>> and if not we run it inside expect_failure?
>>>
>>> What kind of idiocy is that, I have to wonder...
>>
>> Well, the point is to normalize the test suite so that we never have
>> passing TODO tests if everything's OK.
>
> I do not consider a test that passes under some condition but doesn't
> under some other condition "everything is OK".  Marking the test as
> "expect failure" as René originally did makes a lot of sense to me.
>
> The quoted patch is even worse as it will _actively_ prevent you from
> catching a new error you just introduced while futzing "git grep" on a
> platform that used to work.  Your "if" statement will say "ah, grep is
> broken", and you will use expect-failure, not because your platform does
> not support REG_STARTEND, but because you broke "git grep".
>
> The point of having tests is to help you catch your bugs while you
> develop.  A test that turns itself off when the feature it is testing is
> broken helps nobody.
>
> So forget about "passing TODO tests", whatever a "TODO test" is.  The
> change in question is actively _wrong_.

I was under the impression that REG_STARTEND was considered purely
icing on the cake, i.e. that the tests should be passing whether or
not it was present.

I guess my reasoning at the time was that if that wasn't the case,
reporting an unexpected pass by default, as opposed to a failing TODO
on platforms without REG_STARTEND. Since only TAP will report this, I
thought that was just an omission.

Anyway, since REG_STARTEND *isn't* obviously considered icing you're
of course right, but the test is still broken as-is. Now it reports an
abnormal condition if REG_STARTEND is present (passing TODO test), it
should instead have a failing TODO test where REG_STARTEND isn't
present. I'll come up with a patch to fix that.

We should also just upgrade the GNU regex library in compat/regex to
the version that supports REG_STARTEND. Unfortunately that seems
easier said than done, since the library is now part of glibc, and has
aquired a lot of glibc specific macros and other constructs that would
need to be #defined away or otherwise worked around.

Thanks for the review.

Re: [RFC/PATCH] Update compat/regex

[Ævar Arnfjörð Bjarmason]

On Thu, Jul 15, 2010 at 22:00, Jonathan Nieder <jrnieder@gmail.com> wrote:
> The old glibc regular expression library git uses does not support
> REG_STARTEND. Borrow the latest version from the GNU C library
> (version 2.12, license is LGPL 2.1 or later).
>
> Changes from glibc version: update the FSF address in the license
> header, use _LIBC_REGEX macro to allow building outside of glibc,
> add MAX, bool, true, and false macros to allow building with a C89
> compiler.
>
> Reintroduces warnings that were fixed in git before.  They can be
> defeated again separately.  Nevertheless this should be an
> improvement.

Those should be funneled upstream anyway, not just fixed in our tree.

> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> Ęvar Arnfjörš Bjarmason wrote:
>
>> Unfortunately that seems
>> easier said than done, since the library is now part of glibc, and has
>> aquired a lot of glibc specific macros and other constructs
>
> Oh, come on. :)
>
> Completely untested.

This patch has all the glibc-specific stuff that makes it break hard
if you don't have the GNU C library. Writing macros/definitions to fix all that
stuff up was the "easier said than done" part I was referring to.

It's probably not that hard (although I wouldn't put it past GNU to
have e.g. GCC-specific stuff in the code, but I haven't checked), just
tedious.

on Solaris:

    In file included from compat/regex/regex.c:73:
    compat/regex/regex_internal.c:40: error: syntax error before
"re_string_allocate"
    compat/regex/regex_internal.c:42: warning: return type defaults to `int'
    compat/regex/regex_internal.c:68: error: syntax error before
"re_string_construct"
    compat/regex/regex_internal.c:70: warning: return type defaults to `int'
    compat/regex/regex_internal.c:131: error: syntax error before
"re_string_realloc_buffers"
    compat/regex/regex_internal.c:132: warning: return type defaults to `int'
    compat/regex/regex_internal.c:132: error: conflicting types for
're_string_realloc_buffers'
    compat/regex/regex_internal.h:392: error: previous declaration of
're_string_realloc_buffers' was here
    compat/regex/regex_internal.c:132: error: conflicting types for
're_string_realloc_buffers'
    compat/regex/regex_internal.h:392: error: previous declaration of
're_string_realloc_buffers' was here
    compat/regex/regex_internal.c:572: error: syntax error before
"re_string_reconstruct"
    compat/regex/regex_internal.c:573: warning: return type defaults to `int'
    compat/regex/regex_internal.c: In function `re_string_reconstruct':
    compat/regex/regex_internal.c:687: warning: unused variable `prev_valid_len'
    compat/regex/regex_internal.c: At top level:
    compat/regex/regex_internal.c:962: error: syntax error before
"re_node_set_alloc"
    compat/regex/regex_internal.c:963: warning: return type defaults to `int'
    compat/regex/regex_internal.c:974: error: syntax error before
"re_node_set_init_1"
    compat/regex/regex_internal.c:975: warning: return type defaults to `int'
    compat/regex/regex_internal.c:990: error: syntax error before
"re_node_set_init_2"
    compat/regex/regex_internal.c:991: warning: return type defaults to `int'
    compat/regex/regex_internal.c:1020: error: syntax error before
"re_node_set_init_copy"
    compat/regex/regex_internal.c:1021: warning: return type defaults to `int'
    compat/regex/regex_internal.c:1045: error: syntax error before
"re_node_set_add_intersect"
    compat/regex/regex_internal.c:1047: warning: return type defaults to `int'
    compat/regex/regex_internal.c:1136: error: syntax error before
"re_node_set_init_union"
    compat/regex/regex_internal.c:1138: warning: return type defaults to `int'
    compat/regex/regex_internal.c:1189: error: syntax error before
"re_node_set_merge"
    compat/regex/regex_internal.c:1190: warning: return type defaults to `int'
    compat/regex/regex_internal.c:1272: error: syntax error before
"re_node_set_insert"
    compat/regex/regex_internal.c:1273: warning: return type defaults to `int'
    compat/regex/regex_internal.c:1329: error: syntax error before
"re_node_set_insert_last"
    compat/regex/regex_internal.c:1330: warning: return type defaults to `int'
    compat/regex/regex_internal.c: In function `re_dfa_add_node':
    compat/regex/regex_internal.c:1406: warning: unused variable `type'
    compat/regex/regex_internal.c: At top level:
    compat/regex/regex_internal.c:1472: error: syntax error before
"re_acquire_state"
    compat/regex/regex_internal.c:1474: warning: return type defaults to `int'
    compat/regex/regex_internal.c: In function `re_acquire_state':
    compat/regex/regex_internal.c:1493: warning: return makes integer
from pointer without a cast
    compat/regex/regex_internal.c:1501: warning: return makes integer
from pointer without a cast
    compat/regex/regex_internal.c: At top level:
    compat/regex/regex_internal.c:1516: error: syntax error before
"re_acquire_state_context"
    compat/regex/regex_internal.c:1518: warning: return type defaults to `int'
    compat/regex/regex_internal.c: In function `re_acquire_state_context':
    compat/regex/regex_internal.c:1537: warning: return makes integer
from pointer without a cast
    compat/regex/regex_internal.c:1544: warning: return makes integer
from pointer without a cast
    compat/regex/regex_internal.c: At top level:
    compat/regex/regex_internal.c:1553: error: syntax error before
"register_state"
    compat/regex/regex_internal.c:1555: warning: return type defaults to `int'
    compat/regex/regex_internal.c:1608: error: syntax error before
"create_ci_newstate"
    compat/regex/regex_internal.c:1610: warning: return type defaults to `int'
    compat/regex/regex_internal.c:1610: error: conflicting types for
'create_ci_newstate'
    compat/regex/regex_internal.c:27: error: previous declaration of
'create_ci_newstate' was here
    compat/regex/regex_internal.c:1610: error: conflicting types for
'create_ci_newstate'
    compat/regex/regex_internal.c:27: error: previous declaration of
'create_ci_newstate' was here
    compat/regex/regex_internal.c: In function `create_ci_newstate':
    compat/regex/regex_internal.c:1650: warning: return makes integer
from pointer without a cast
    compat/regex/regex_internal.c: At top level:
    compat/regex/regex_internal.c:1658: error: syntax error before
"create_cd_newstate"
    compat/regex/regex_internal.c:1660: warning: return type defaults to `int'
    compat/regex/regex_internal.c:1660: error: conflicting types for
'create_cd_newstate'
    compat/regex/regex_internal.c:31: error: previous declaration of
'create_cd_newstate' was here
    compat/regex/regex_internal.c:1660: error: conflicting types for
'create_cd_newstate'
    compat/regex/regex_internal.c:31: error: previous declaration of
'create_cd_newstate' was here
    compat/regex/regex_internal.c: In function `create_cd_newstate':
    compat/regex/regex_internal.c:1726: warning: return makes integer
from pointer without a cast
    In file included from compat/regex/regex.c:74:
    compat/regex/regcomp.c: In function `create_initial_state':
    compat/regex/regcomp.c:1013: warning: assignment makes pointer
from integer without a cast
    compat/regex/regcomp.c:1020: warning: assignment makes pointer
from integer without a cast
    compat/regex/regcomp.c:1022: warning: assignment makes pointer
from integer without a cast
    compat/regex/regcomp.c:1026: warning: assignment makes pointer
from integer without a cast
    In file included from compat/regex/regex.c:75:
    compat/regex/regexec.c: In function `regexec':
    compat/regex/regexec.c:230: warning: unused variable `dfa'
    compat/regex/regexec.c: In function `re_search_stub':
    compat/regex/regexec.c:418: warning: unused variable `dfa'
    compat/regex/regexec.c: At top level:
    compat/regex/regexec.c:629: error: syntax error before "re_search_internal"
    compat/regex/regexec.c:631: warning: return type defaults to `int'
    compat/regex/regexec.c:631: error: conflicting types for
're_search_internal'
    compat/regex/regexec.c:43: error: previous declaration of
're_search_internal' was here
    compat/regex/regexec.c:631: error: conflicting types for
're_search_internal'
    compat/regex/regexec.c:43: error: previous declaration of
're_search_internal' was here
    compat/regex/regexec.c:956: error: syntax error before
"prune_impossible_nodes"
    compat/regex/regexec.c:957: warning: return type defaults to `int'
    compat/regex/regexec.c:957: error: conflicting types for
'prune_impossible_nodes'
    compat/regex/regexec.c:56: error: previous declaration of
'prune_impossible_nodes' was here
    compat/regex/regexec.c:957: error: conflicting types for
'prune_impossible_nodes'
    compat/regex/regexec.c:56: error: previous declaration of
'prune_impossible_nodes' was here
    compat/regex/regexec.c: In function `acquire_init_state_context':
    compat/regex/regexec.c:1074: warning: return makes pointer from
integer without a cast
    compat/regex/regexec.c: At top level:
    compat/regex/regexec.c:1095: error: syntax error before "check_matching"
    compat/regex/regexec.c:1097: warning: return type defaults to `int'
    compat/regex/regexec.c:1097: error: conflicting types for 'check_matching'
    compat/regex/regexec.c:58: error: previous declaration of
'check_matching' was here
    compat/regex/regexec.c:1097: error: conflicting types for 'check_matching'
    compat/regex/regexec.c:58: error: previous declaration of
'check_matching' was here
    compat/regex/regexec.c:1368: error: syntax error before "push_fail_stack"
    compat/regex/regexec.c:1370: warning: return type defaults to `int'
    compat/regex/regexec.c:1370: error: conflicting types for 'push_fail_stack'
    compat/regex/regexec.c:69: error: previous declaration of
'push_fail_stack' was here
    compat/regex/regexec.c:1370: error: conflicting types for 'push_fail_stack'
    compat/regex/regexec.c:69: error: previous declaration of
'push_fail_stack' was here
    compat/regex/regexec.c:1415: error: syntax error before "set_regs"
    compat/regex/regexec.c:1417: warning: return type defaults to `int'
    compat/regex/regexec.c:1417: error: conflicting types for 'set_regs'
    compat/regex/regexec.c:73: error: previous declaration of
'set_regs' was here
    compat/regex/regexec.c:1417: error: conflicting types for 'set_regs'
    compat/regex/regexec.c:73: error: previous declaration of
'set_regs' was here
    compat/regex/regexec.c:1669: error: syntax error before
"build_sifted_states"
    compat/regex/regexec.c:1671: warning: return type defaults to `int'
    compat/regex/regexec.c:1671: error: conflicting types for
'build_sifted_states'
    compat/regex/regexec.c:89: error: previous declaration of
'build_sifted_states' was here
    compat/regex/regexec.c:1671: error: conflicting types for
'build_sifted_states'
    compat/regex/regexec.c:89: error: previous declaration of
'build_sifted_states' was here
    compat/regex/regexec.c: In function `merge_state_array':
    compat/regex/regexec.c:1772: warning: assignment makes pointer
from integer without a cast
    compat/regex/regexec.c: In function `update_cur_sifted_state':
    compat/regex/regexec.c:1815: warning: assignment makes pointer
from integer without a cast
    compat/regex/regexec.c: At top level:
    compat/regex/regexec.c:1831: error: syntax error before
"add_epsilon_src_nodes"
    compat/regex/regexec.c:1833: warning: return type defaults to `int'
    compat/regex/regexec.c:1833: error: conflicting types for
'add_epsilon_src_nodes'
    compat/regex/regexec.c:98: error: previous declaration of
'add_epsilon_src_nodes' was here
    compat/regex/regexec.c:1833: error: conflicting types for
'add_epsilon_src_nodes'
    compat/regex/regexec.c:98: error: previous declaration of
'add_epsilon_src_nodes' was here
    compat/regex/regexec.c: In function `add_epsilon_src_nodes':
    compat/regex/regexec.c:1837: warning: initialization makes pointer
from integer without a cast
    compat/regex/regexec.c: At top level:
    compat/regex/regexec.c:2144: error: syntax error before "sift_states_bkref"
    compat/regex/regexec.c:2146: warning: return type defaults to `int'
    compat/regex/regexec.c:2146: error: conflicting types for
'sift_states_bkref'
    compat/regex/regexec.c:120: error: previous declaration of
'sift_states_bkref' was here
    compat/regex/regexec.c:2146: error: conflicting types for
'sift_states_bkref'
    compat/regex/regexec.c:120: error: previous declaration of
'sift_states_bkref' was here
    compat/regex/regexec.c:2274: error: syntax error before "transit_state"
    compat/regex/regexec.c:2276: warning: return type defaults to `int'
    compat/regex/regexec.c:2276: error: conflicting types for 'transit_state'
    compat/regex/regexec.c:129: error: previous declaration of
'transit_state' was here
    compat/regex/regexec.c:2276: error: conflicting types for 'transit_state'
    compat/regex/regexec.c:129: error: previous declaration of
'transit_state' was here
    compat/regex/regexec.c: In function `transit_state':
    compat/regex/regexec.c:2303: warning: return makes integer from
pointer without a cast
    compat/regex/regexec.c:2314: warning: return makes integer from
pointer without a cast
    compat/regex/regexec.c:2316: warning: return makes integer from
pointer without a cast
    compat/regex/regexec.c: In function `merge_state_with_log':
    compat/regex/regexec.c:2375: warning: assignment makes pointer
from integer without a cast
    compat/regex/regexec.c: In function `transit_state_bkref':
    compat/regex/regexec.c:2648: warning: assignment makes pointer
from integer without a cast
    compat/regex/regexec.c:2665: warning: assignment makes pointer
from integer without a cast
    compat/regex/regexec.c: At top level:
    compat/regex/regexec.c:2699: error: syntax error before "get_subexp"
    compat/regex/regexec.c:2700: warning: return type defaults to `int'
    compat/regex/regexec.c:2700: error: conflicting types for 'get_subexp'
    compat/regex/regexec.c:153: error: previous declaration of
'get_subexp' was here
    compat/regex/regexec.c:2700: error: conflicting types for 'get_subexp'
    compat/regex/regexec.c:153: error: previous declaration of
'get_subexp' was here
    compat/regex/regexec.c:2899: error: syntax error before "check_arrival"
    compat/regex/regexec.c:2901: warning: return type defaults to `int'
    compat/regex/regexec.c:2901: error: conflicting types for 'check_arrival'
    compat/regex/regexec.c:164: error: previous declaration of
'check_arrival' was here
    compat/regex/regexec.c:2901: error: conflicting types for 'check_arrival'
    compat/regex/regexec.c:164: error: previous declaration of
'check_arrival' was here
    compat/regex/regexec.c: In function `check_arrival':
    compat/regex/regexec.c:2974: warning: assignment makes pointer
from integer without a cast
    compat/regex/regexec.c:3025: warning: assignment makes pointer
from integer without a cast
    compat/regex/regexec.c: At top level:
    compat/regex/regexec.c:3060: error: syntax error before
"check_arrival_add_next_nodes"
    compat/regex/regexec.c:3062: warning: return type defaults to `int'
    compat/regex/regexec.c:3062: error: conflicting types for
'check_arrival_add_next_nodes'
    compat/regex/regexec.c:169: error: previous declaration of
'check_arrival_add_next_nodes' was here
    compat/regex/regexec.c:3062: error: conflicting types for
'check_arrival_add_next_nodes'
    compat/regex/regexec.c:169: error: previous declaration of
'check_arrival_add_next_nodes' was here
    compat/regex/regexec.c: In function `check_arrival_add_next_nodes':
    compat/regex/regexec.c:3066: warning: unused variable `err'
    compat/regex/regexec.c: At top level:
    compat/regex/regexec.c:3192: error: syntax error before
"check_arrival_expand_ecl_sub"
    compat/regex/regexec.c:3194: warning: return type defaults to `int'
    compat/regex/regexec.c:3194: error: conflicting types for
'check_arrival_expand_ecl_sub'
    compat/regex/regexec.c:177: error: previous declaration of
'check_arrival_expand_ecl_sub' was here
    compat/regex/regexec.c:3194: error: conflicting types for
'check_arrival_expand_ecl_sub'
    compat/regex/regexec.c:177: error: previous declaration of
'check_arrival_expand_ecl_sub' was here
    compat/regex/regexec.c:3236: error: syntax error before "expand_bkref_cache"
    compat/regex/regexec.c:3238: warning: return type defaults to `int'
    compat/regex/regexec.c:3238: error: conflicting types for
'expand_bkref_cache'
    compat/regex/regexec.c:181: error: previous declaration of
'expand_bkref_cache' was here
    compat/regex/regexec.c:3238: error: conflicting types for
'expand_bkref_cache'
    compat/regex/regexec.c:181: error: previous declaration of
'expand_bkref_cache' was here
    compat/regex/regexec.c: In function `expand_bkref_cache':
    compat/regex/regexec.c:3309: warning: assignment makes pointer
from integer without a cast
    compat/regex/regexec.c: In function `build_trtable':
    compat/regex/regexec.c:3434: warning: assignment makes pointer
from integer without a cast
    compat/regex/regexec.c:3442: warning: assignment makes pointer
from integer without a cast
    compat/regex/regexec.c:3450: warning: assignment makes pointer
from integer without a cast
    compat/regex/regexec.c: At top level:
    compat/regex/regexec.c:4102: error: syntax error before "extend_buffers"
    compat/regex/regexec.c:4103: warning: return type defaults to `int'
    compat/regex/regexec.c:4103: error: conflicting types for 'extend_buffers'
    compat/regex/regexec.c:202: error: previous declaration of
'extend_buffers' was here
    compat/regex/regexec.c:4103: error: conflicting types for 'extend_buffers'
    compat/regex/regexec.c:202: error: previous declaration of
'extend_buffers' was here
    compat/regex/regexec.c:4165: error: syntax error before "match_ctx_init"
    compat/regex/regexec.c:4166: warning: return type defaults to `int'
    compat/regex/regexec.c:4166: error: conflicting types for 'match_ctx_init'
    compat/regex/regexec.c:22: error: previous declaration of
'match_ctx_init' was here
    compat/regex/regexec.c:4166: error: conflicting types for 'match_ctx_init'
    compat/regex/regexec.c:22: error: previous declaration of
'match_ctx_init' was here
    compat/regex/regexec.c:4238: error: syntax error before
"match_ctx_add_entry"
    compat/regex/regexec.c:4240: warning: return type defaults to `int'
    compat/regex/regexec.c:4240: error: conflicting types for
'match_ctx_add_entry'
    compat/regex/regexec.c:27: error: previous declaration of
'match_ctx_add_entry' was here
    compat/regex/regexec.c:4240: error: conflicting types for
'match_ctx_add_entry'
    compat/regex/regexec.c:27: error: previous declaration of
'match_ctx_add_entry' was here
    compat/regex/regexec.c:4310: error: syntax error before
"match_ctx_add_subtop"
    compat/regex/regexec.c:4311: warning: return type defaults to `int'
    compat/regex/regexec.c:4311: error: conflicting types for
'match_ctx_add_subtop'
    compat/regex/regexec.c:31: error: previous declaration of
'match_ctx_add_subtop' was here
    compat/regex/regexec.c:4311: error: conflicting types for
'match_ctx_add_subtop'
    compat/regex/regexec.c:31: error: previous declaration of
'match_ctx_add_subtop' was here
    compat/regex/regex_internal.h:392: warning:
're_string_realloc_buffers' declared `static' but never defined
    compat/regex/regex_internal.c:27: warning: 'create_ci_newstate'
declared `static' but never defined
    compat/regex/regex_internal.c:31: warning: 'create_cd_newstate'
declared `static' but never defined
    compat/regex/regexec.c:22: warning: 'match_ctx_init' declared
`static' but never defined
    compat/regex/regexec.c:27: warning: 'match_ctx_add_entry' declared
`static' but never defined
    compat/regex/regexec.c:31: warning: 'match_ctx_add_subtop'
declared `static' but never defined
    compat/regex/regexec.c:43: warning: 're_search_internal' declared
`static' but never defined
    compat/regex/regexec.c:56: warning: 'prune_impossible_nodes'
declared `static' but never defined
    compat/regex/regexec.c:58: warning: 'check_matching' declared
`static' but never defined
    compat/regex/regexec.c:69: warning: 'push_fail_stack' declared
`static' but never defined
    compat/regex/regexec.c:73: warning: 'set_regs' declared `static'
but never defined
    compat/regex/regexec.c:89: warning: 'build_sifted_states' declared
`static' but never defined
    compat/regex/regexec.c:98: warning: 'add_epsilon_src_nodes'
declared `static' but never defined
    compat/regex/regexec.c:120: warning: 'sift_states_bkref' declared
`static' but never defined
    compat/regex/regexec.c:129: warning: 'transit_state' declared
`static' but never defined
    compat/regex/regexec.c:153: warning: 'get_subexp' declared
`static' but never defined
    compat/regex/regexec.c:164: warning: 'check_arrival' declared
`static' but never defined
    compat/regex/regexec.c:169: warning:
'check_arrival_add_next_nodes' declared `static' but never defined
    compat/regex/regexec.c:177: warning:
'check_arrival_expand_ecl_sub' declared `static' but never defined
    compat/regex/regexec.c:181: warning: 'expand_bkref_cache' declared
`static' but never defined
    compat/regex/regexec.c:202: warning: 'extend_buffers' declared
`static' but never defined
    gmake: *** [compat/regex/regex.o] Error 1

And on FreeBSD:

    In file included from git-compat-util.h:93,
                     from cache.h:4,
                     from thread-utils.c:1:
    compat/regex/regex.h:367: warning: declaration does not declare anything
        CC compat/memmem.o
    In file included from compat/../git-compat-util.h:93,
                     from compat/memmem.c:1:
    compat/regex/regex.h:367: warning: declaration does not declare anything
        CC compat/regex/regex.o
    In file included from compat/regex/regex.c:63:
    compat/regex/regex_internal.h:421:20: error: alloca.h: No such
file or directory
    In file included from compat/regex/regex.c:73:
    compat/regex/regex_internal.c:40: error: expected '=', ',', ';',
'asm' or '__attribute__' before 're_string_allocate'
    compat/regex/regex_internal.c:68: error: expected '=', ',', ';',
'asm' or '__attribute__' before 're_string_construct'
    compat/regex/regex_internal.c:131: error: expected '=', ',', ';',
'asm' or '__attribute__' before 're_string_realloc_buffers'
    compat/regex/regex_internal.c:572: error: expected '=', ',', ';',
'asm' or '__attribute__' before 're_string_reconstruct'
    compat/regex/regex_internal.c:962: error: expected '=', ',', ';',
'asm' or '__attribute__' before 're_node_set_alloc'
    compat/regex/regex_internal.c:974: error: expected '=', ',', ';',
'asm' or '__attribute__' before 're_node_set_init_1'
    compat/regex/regex_internal.c:990: error: expected '=', ',', ';',
'asm' or '__attribute__' before 're_node_set_init_2'
    compat/regex/regex_internal.c:1020: error: expected '=', ',', ';',
'asm' or '__attribute__' before 're_node_set_init_copy'
    compat/regex/regex_internal.c:1045: error: expected '=', ',', ';',
'asm' or '__attribute__' before 're_node_set_add_intersect'
    compat/regex/regex_internal.c:1136: error: expected '=', ',', ';',
'asm' or '__attribute__' before 're_node_set_init_union'
    compat/regex/regex_internal.c:1189: error: expected '=', ',', ';',
'asm' or '__attribute__' before 're_node_set_merge'
    compat/regex/regex_internal.c:1272: error: expected '=', ',', ';',
'asm' or '__attribute__' before 're_node_set_insert'
    compat/regex/regex_internal.c:1329: error: expected '=', ',', ';',
'asm' or '__attribute__' before 're_node_set_insert_last'
    compat/regex/regex_internal.c: In function 're_dfa_add_node':
    compat/regex/regex_internal.c:1406: warning: unused variable 'type'
    compat/regex/regex_internal.c: At top level:
    compat/regex/regex_internal.c:1472: error: expected '=', ',', ';',
'asm' or '__attribute__' before 're_acquire_state'
    compat/regex/regex_internal.c:1516: error: expected '=', ',', ';',
'asm' or '__attribute__' before 're_acquire_state_context'
    compat/regex/regex_internal.c:1553: error: expected '=', ',', ';',
'asm' or '__attribute__' before 'register_state'
    compat/regex/regex_internal.c:1608: error: expected '=', ',', ';',
'asm' or '__attribute__' before 'create_ci_newstate'
    compat/regex/regex_internal.c:1658: error: expected '=', ',', ';',
'asm' or '__attribute__' before 'create_cd_newstate'
    In file included from compat/regex/regex.c:74:
    compat/regex/regcomp.c: In function 're_compile_internal':
    compat/regex/regcomp.c:778: warning: implicit declaration of
function 're_string_construct'
    compat/regex/regcomp.c: In function 'create_initial_state':
    compat/regex/regcomp.c:969: warning: implicit declaration of
function 're_node_set_init_copy'
    compat/regex/regcomp.c:1002: warning: implicit declaration of
function 're_node_set_merge'
    compat/regex/regcomp.c:1013: warning: implicit declaration of
function 're_acquire_state_context'
    compat/regex/regcomp.c:1013: warning: assignment makes pointer
from integer without a cast
    compat/regex/regcomp.c:1020: warning: assignment makes pointer
from integer without a cast
    compat/regex/regcomp.c:1022: warning: assignment makes pointer
from integer without a cast
    compat/regex/regcomp.c:1026: warning: assignment makes pointer
from integer without a cast
    compat/regex/regcomp.c: In function 'link_nfa_nodes':
    compat/regex/regcomp.c:1408: warning: implicit declaration of
function 're_node_set_init_2'
    compat/regex/regcomp.c:1415: warning: implicit declaration of
function 're_node_set_init_1'
    compat/regex/regcomp.c: In function 'duplicate_node_closure':
    compat/regex/regcomp.c:1459: warning: implicit declaration of
function 're_node_set_insert'
    compat/regex/regcomp.c: In function 'calc_inveclosure':
    compat/regex/regcomp.c:1590: warning: implicit declaration of
function 're_node_set_insert_last'
    compat/regex/regcomp.c: In function 'calc_eclosure_iter':
    compat/regex/regcomp.c:1653: warning: implicit declaration of
function 're_node_set_alloc'
    In file included from compat/regex/regex.c:75:
    compat/regex/regexec.c: In function 'regexec':
    compat/regex/regexec.c:230: warning: unused variable 'dfa'
    compat/regex/regexec.c: In function 're_search_stub':
    compat/regex/regexec.c:418: warning: unused variable 'dfa'
    compat/regex/regexec.c: At top level:
    compat/regex/regexec.c:629: error: expected '=', ',', ';', 'asm'
or '__attribute__' before 're_search_internal'
    compat/regex/regexec.c:636: error: expected identifier or '('
before '{' token
    compat/regex/regexec.c:956: error: expected '=', ',', ';', 'asm'
or '__attribute__' before 'prune_impossible_nodes'
    compat/regex/regexec.c:958: error: expected identifier or '('
before '{' token
    compat/regex/regexec.c: In function 'acquire_init_state_context':
    compat/regex/regexec.c:1074: warning: return makes pointer from
integer without a cast
    compat/regex/regexec.c: At top level:
    compat/regex/regexec.c:1095: error: expected '=', ',', ';', 'asm'
or '__attribute__' before 'check_matching'
    compat/regex/regexec.c:1368: error: expected '=', ',', ';', 'asm'
or '__attribute__' before 'push_fail_stack'
    compat/regex/regexec.c:1415: error: expected '=', ',', ';', 'asm'
or '__attribute__' before 'set_regs'
    compat/regex/regexec.c:1669: error: expected '=', ',', ';', 'asm'
or '__attribute__' before 'build_sifted_states'
    compat/regex/regexec.c: In function 'merge_state_array':
    compat/regex/regexec.c:1768: warning: implicit declaration of
function 're_node_set_init_union'
    compat/regex/regexec.c:1772: warning: implicit declaration of
function 're_acquire_state'
    compat/regex/regexec.c:1772: warning: assignment makes pointer
from integer without a cast
    compat/regex/regexec.c: In function 'update_cur_sifted_state':
    compat/regex/regexec.c:1815: warning: assignment makes pointer
from integer without a cast
    compat/regex/regexec.c: At top level:
    compat/regex/regexec.c:1831: error: expected '=', ',', ';', 'asm'
or '__attribute__' before 'add_epsilon_src_nodes'
    compat/regex/regexec.c: In function 'sub_epsilon_src_nodes':
    compat/regex/regexec.c:1884: warning: implicit declaration of
function 're_node_set_add_intersect'
    compat/regex/regexec.c: At top level:
    compat/regex/regexec.c:2144: error: expected '=', ',', ';', 'asm'
or '__attribute__' before 'sift_states_bkref'
    compat/regex/regexec.c:2274: error: expected '=', ',', ';', 'asm'
or '__attribute__' before 'transit_state'
    compat/regex/regexec.c: In function 'merge_state_with_log':
    compat/regex/regexec.c:2375: warning: assignment makes pointer
from integer without a cast
    compat/regex/regexec.c: In function 'transit_state_bkref':
    compat/regex/regexec.c:2648: warning: assignment makes pointer
from integer without a cast
    compat/regex/regexec.c:2665: warning: assignment makes pointer
from integer without a cast
    compat/regex/regexec.c: At top level:
    compat/regex/regexec.c:2699: error: expected '=', ',', ';', 'asm'
or '__attribute__' before 'get_subexp'
    compat/regex/regexec.c:2899: error: expected '=', ',', ';', 'asm'
or '__attribute__' before 'check_arrival'
    compat/regex/regexec.c:3060: error: expected '=', ',', ';', 'asm'
or '__attribute__' before 'check_arrival_add_next_nodes'
    compat/regex/regexec.c:3192: error: expected '=', ',', ';', 'asm'
or '__attribute__' before 'check_arrival_expand_ecl_sub'
    compat/regex/regexec.c:3236: error: expected '=', ',', ';', 'asm'
or '__attribute__' before 'expand_bkref_cache'
    compat/regex/regexec.c: In function 'build_trtable':
    compat/regex/regexec.c:3434: warning: assignment makes pointer
from integer without a cast
    compat/regex/regexec.c:3442: warning: assignment makes pointer
from integer without a cast
    compat/regex/regexec.c:3450: warning: assignment makes pointer
from integer without a cast
    compat/regex/regexec.c: At top level:
    compat/regex/regexec.c:4102: error: expected '=', ',', ';', 'asm'
or '__attribute__' before 'extend_buffers'
    compat/regex/regexec.c:4165: error: expected '=', ',', ';', 'asm'
or '__attribute__' before 'match_ctx_init'
    compat/regex/regexec.c:4238: error: expected '=', ',', ';', 'asm'
or '__attribute__' before 'match_ctx_add_entry'
    compat/regex/regexec.c:4310: error: expected '=', ',', ';', 'asm'
or '__attribute__' before 'match_ctx_add_subtop'
    gmake: *** [compat/regex/regex.o] Error 1

Re: [RFC/PATCH] Update compat/regex

[Andreas Schwab]

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

> This patch has all the glibc-specific stuff that makes it break hard
> if you don't have the GNU C library. Writing macros/definitions to fix all that
> stuff up was the "easier said than done" part I was referring to.

You might want to try out the gnulib version instead.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [RFC/PATCH] Update compat/regex

[Ævar Arnfjörð Bjarmason]

On Fri, Jul 16, 2010 at 14:17, Andreas Schwab <schwab@linux-m68k.org> wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> This patch has all the glibc-specific stuff that makes it break hard
>> if you don't have the GNU C library. Writing macros/definitions to fix all that
>> stuff up was the "easier said than done" part I was referring to.
>
> You might want to try out the gnulib version instead.

I fiddled a bit with gnulib for both the regex engine and libintl, but
I can't get it to do what I want.

The assumption with gnulib seems to be that you're including the
libraries in a GNU program that only uses the autotools, it seems to
be about as easy to just copy/paste things from glibc if you're adding
libraries to a program like Git that uses its own build system.

Re: [RFC/PATCH] Update compat/regex

[Paolo Bonzini]

On 08/15/2010 01:08 PM, Ævar Arnfjörð Bjarmason wrote:
> On Fri, Jul 16, 2010 at 14:17, Andreas Schwab<schwab@linux-m68k.org>  wrote:
>> Ævar Arnfjörð Bjarmason<avarab@gmail.com>  writes:
>>
>>> This patch has all the glibc-specific stuff that makes it break hard
>>> if you don't have the GNU C library. Writing macros/definitions to fix all that
>>> stuff up was the "easier said than done" part I was referring to.
>>
>> You might want to try out the gnulib version instead.
>
> I fiddled a bit with gnulib for both the regex engine and libintl, but
> I can't get it to do what I want.
>
> The assumption with gnulib seems to be that you're including the
> libraries in a GNU program that only uses the autotools, it seems to
> be about as easy to just copy/paste things from glibc if you're adding
> libraries to a program like Git that uses its own build system.

Andreas is right, the glibc code is not meant to be portable.

It is really simpler if you start from the version in gnulib, which is 
the one that is included in most GNU packages nowadays.  You should 
download GNU grep 2.6.x and (starting from lib/reg*) add headers from 
its lib/ directory until it compiles.

Alternatively try out gawk, as it does not use gnulib but has the same 
set of sanitizations.

Paolo

[PATCH/RFC 0/3] Update compat/regex

[Ævar Arnfjörð Bjarmason]

On Mon, Aug 16, 2010 at 12:26, Paolo Bonzini <bonzini@gnu.org> wrote:

> Alternatively try out gawk, as it does not use gnulib but has the same set
> of sanitizations.

Why didn't you say that earlier? :)

Here's a RFC patch series that uses the gawk regex engine from the
gawk-devel branch of gawk CVS.

It compiles on Linux/FreeBSD and Solaris, with only a single warning
on FreeBSD due to an unused variable (upstream bug).

Ævar Arnfjörð Bjarmason (3):
  compat/regex: use the regex engine from gawk for compat
  compat/regex: hacks to get the gawk regex engine to compile within
    git
  t/t7008-grep-binary.sh: un-TODO a test that needs REG_STARTEND

 Makefile                      |    4 +
 compat/regex/COPYING          |  674 ++++++
 compat/regex/mbsupport.h      |   59 +
 compat/regex/regcomp.c        | 3892 ++++++++++++++++++++++++++++++++
 compat/regex/regex.c          | 5003 +----------------------------------------
 compat/regex/regex.h          |  462 +++--
 compat/regex/regex_internal.c | 1744 ++++++++++++++
 compat/regex/regex_internal.h |  810 +++++++
 compat/regex/regexec.c        | 4377 +++++++++++++++++++++++++++++++++++
 t/t7008-grep-binary.sh        |    2 +-
 10 files changed, 11921 insertions(+), 5106 deletions(-)
 create mode 100644 compat/regex/COPYING
 create mode 100644 compat/regex/mbsupport.h
 create mode 100644 compat/regex/regcomp.c
 create mode 100644 compat/regex/regex_internal.c
 create mode 100644 compat/regex/regex_internal.h
 create mode 100644 compat/regex/regexec.c

-- 
1.7.2.1.389.gc3d0b

[PATCH/RFC 2/3] compat/regex: hacks to get the gawk regex engine to compile within git

[Ævar Arnfjörð Bjarmason]

The gawk regex engine didn't include stdio.h, and only include
stddef.h if HAVE_STDDEF_H is set.

Adding -DHAVE_STDDEF_H caused some internal errors in by /usr/include
headers, so change the regex.h code to include it unconditionally.

We also need to define -DGAWK so that e.g. "bool", "MAX" and other
similar things used inside gawk get defined.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile             |    4 ++++
 compat/regex/regex.h |    7 +++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index b4745a5..6704780 100644
--- a/Makefile
+++ b/Makefile
@@ -1443,6 +1443,10 @@ ifdef UNRELIABLE_FSTAT
 	BASIC_CFLAGS += -DUNRELIABLE_FSTAT
 endif
 ifdef NO_REGEX
+	# TODO: How do I compile just regex.o with this flag, not the
+	# whole of Git?
+	BASIC_CFLAGS += -DGAWK
+
 	COMPAT_CFLAGS += -Icompat/regex
 	COMPAT_OBJS += compat/regex/regex.o
 endif
diff --git a/compat/regex/regex.h b/compat/regex/regex.h
index de93327..508bc80 100644
--- a/compat/regex/regex.h
+++ b/compat/regex/regex.h
@@ -22,9 +22,12 @@
 #ifndef _REGEX_H
 #define _REGEX_H 1
 
-#ifdef HAVE_STDDEF_H
+#include <stdio.h>
+/*
+  Git: Was in `#ifdef HAVE_STDDEF_H` in gawk, adding -DHAVE_STDDEF_H makes a
+  *lot* of other things break
+*/
 #include <stddef.h>
-#endif
 
 #ifdef HAVE_SYS_TYPES_H
 #include <sys/types.h>
-- 
1.7.2.1.389.gc3d0b

Re: [PATCH/RFC 2/3] compat/regex: hacks to get the gawk regex engine to compile within git

[Jonathan Nieder]

Hi!

Ævar Arnfjörð Bjarmason wrote:

> +	# TODO: How do I compile just regex.o with this flag, not the
> +	# whole of Git?
> +	BASIC_CFLAGS += -DGAWK

 gawk.o: EXTRA_CPPFLAGS = -DGAWK

See v1.7.1-rc0~60 and v1.7.0-rc0~90^2~2.

> --- a/compat/regex/regex.h
> +++ b/compat/regex/regex.h
> @@ -22,9 +22,12 @@
>  #ifndef _REGEX_H
>  #define _REGEX_H 1
>  
> -#ifdef HAVE_STDDEF_H
> +#include <stdio.h>
> +/*
> +  Git: Was in `#ifdef HAVE_STDDEF_H` in gawk, adding -DHAVE_STDDEF_H makes a
> +  *lot* of other things break
> +*/
>  #include <stddef.h>
> -#endif

Maybe

 #if 1
 #include "git-compat-util.h"
 #else
 ...

would be simpler.

[PATCH/RFC 3/3] t/t7008-grep-binary.sh: un-TODO a test that needs REG_STARTEND

[Ævar Arnfjörð Bjarmason]

Now that we have a regex engine that supports REG_STARTEND this test
should fail if "git grep" can't grep NULL characters. Platforms that
don't have a POSIX regex engine that supports REG_STARTEND should
always define NO_REGEX=YesPlease when compiling.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t7008-grep-binary.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/t7008-grep-binary.sh b/t/t7008-grep-binary.sh
index eb8ca88..c0f9f3f 100755
--- a/t/t7008-grep-binary.sh
+++ b/t/t7008-grep-binary.sh
@@ -61,7 +61,7 @@ test_expect_success 'git grep -Fi iLE a' '
 
 # This test actually passes on platforms where regexec() supports the
 # flag REG_STARTEND.
-test_expect_failure 'git grep ile a' '
+test_expect_success 'git grep ile a' '
 	git grep ile a
 '
 
-- 
1.7.2.1.389.gc3d0b

Re: [PATCH/RFC 1/3] compat/regex: use the regex engine from gawk for compat

[Jonathan Nieder]

Ævar Arnfjörð Bjarmason wrote:

> +++ b/compat/regex/mbsupport.h
> @@ -0,0 +1,59 @@
> +/*
> + * mbsupport.h --- Localize determination of whether we have multibyte stuff.
> + */
> +
> +/* 
> + * Copyright (C) 2004, 2005 the Free Software Foundation, Inc.
> + * 
> + * This file is part of GAWK, the GNU implementation of the
> + * AWK Programming Language.
> + * 
> + * GAWK is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 3 of the License, or
> + * (at your option) any later version.

Worrisome.  (Most of git is still GPL-2 only, which would not be
compatible.)

Re: [PATCH/RFC 1/3] compat/regex: use the regex engine from gawk for compat

[Ævar Arnfjörð Bjarmason]

On Tue, Aug 17, 2010 at 03:37, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Ævar Arnfjörð Bjarmason wrote:
>
>> +++ b/compat/regex/mbsupport.h
>> @@ -0,0 +1,59 @@
>> +/*
>> + * mbsupport.h --- Localize determination of whether we have multibyte stuff.
>> + */
>> +
>> +/*
>> + * Copyright (C) 2004, 2005 the Free Software Foundation, Inc.
>> + *
>> + * This file is part of GAWK, the GNU implementation of the
>> + * AWK Programming Language.
>> + *
>> + * GAWK is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 3 of the License, or
>> + * (at your option) any later version.
>
> Worrisome.  (Most of git is still GPL-2 only, which would not be
> compatible.)

Actually it's LGPL-2.1 (all the copyright headers in the *.[ch] files
agree), I just copied the wrong license.

Re: [PATCH/RFC 1/3] compat/regex: use the regex engine from gawk for compat

[Jonathan Nieder]

Ævar Arnfjörð Bjarmason wrote:
> On Tue, Aug 17, 2010 at 03:37, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> Ævar Arnfjörð Bjarmason wrote:

>>> +++ b/compat/regex/mbsupport.h
>>> @@ -0,0 +1,59 @@
>>> +/*
>>> + * mbsupport.h --- Localize determination of whether we have multibyte stuff.
>>> + */
>>> +
>>> +/*
>>> + * Copyright (C) 2004, 2005 the Free Software Foundation, Inc.
>>> + *
>>> + * This file is part of GAWK, the GNU implementation of the
>>> + * AWK Programming Language.
>>> + *
>>> + * GAWK is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License as published by
>>> + * the Free Software Foundation; either version 3 of the License, or
>>> + * (at your option) any later version.
>>
>> Worrisome.  (Most of git is still GPL-2 only, which would not be
>> compatible.)
>
> Actually it's LGPL-2.1 (all the copyright headers in the *.[ch] files
> agree), I just copied the wrong license.

Thanks for checking.  That’s good to hear.

[PATCH/RFC v2 0/3] Update compat/regex

[Ævar Arnfjörð Bjarmason]

This has the following changes:

 * Supply the custom regex.o flags only to regex.o as suggested by
   Jonathan Nieder:
        
        +ifdef NO_REGEX
        +compat/regex/regex.o: EXTRA_CPPFLAGS = -DGAWK -DNO_MBSUPPORT
        +endif

 * The code is LGPL-2.1, not GPL-3

 * Don't include mbsupport.h, we don't need it, and it can be
   un-included with a flag.

 * Simplify our modifications to regex.h, just include two headers at
   the very top, don't modify any gawk code.

 * Update commit messages
        
Ævar Arnfjörð Bjarmason (3):
  compat/regex: use the regex engine from gawk for compat
  compat/regex: get the gawk regex engine to compile within git
  t/t7008-grep-binary.sh: un-TODO a test that needs REG_STARTEND

 Makefile                      |    4 +
 compat/regex/regcomp.c        | 3892 ++++++++++++++++++++++++++++++++
 compat/regex/regex.c          | 5003 +----------------------------------------
 compat/regex/regex.h          |  462 +++--
 compat/regex/regex_internal.c | 1744 ++++++++++++++
 compat/regex/regex_internal.h |  810 +++++++
 compat/regex/regexec.c        | 4377 +++++++++++++++++++++++++++++++++++
 t/t7008-grep-binary.sh        |    2 +-
 8 files changed, 11188 insertions(+), 5106 deletions(-)
 create mode 100644 compat/regex/regcomp.c
 create mode 100644 compat/regex/regex_internal.c
 create mode 100644 compat/regex/regex_internal.h
 create mode 100644 compat/regex/regexec.c

-- 
1.7.2.1.389.gc3d0b

Re: [PATCH/RFC v2 0/3] Update compat/regex

[Jonathan Nieder]

Ævar Arnfjörð Bjarmason wrote:

> Ævar Arnfjörð Bjarmason (3):
>   compat/regex: use the regex engine from gawk for compat
>   compat/regex: get the gawk regex engine to compile within git
>   t/t7008-grep-binary.sh: un-TODO a test that needs REG_STARTEND

For what it’s worth:

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

Thanks.  Here’s a patch to go on top.  I am not sure what platforms
support REG_STARTEND, but we can always update the makefile as reports
come in.

-- 8< --
Subject: autoconf: don't use platform regex if it lacks REG_STARTEND

If the platform regex cannot match null bytes, we might as well
use the glibc version instead.

Cc: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Cc: René Scharfe <rene.scharfe@lsrfire.ath.cx>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 config.mak.in |    1 +
 configure.ac  |   21 +++++++++++++++++++++
 2 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/config.mak.in b/config.mak.in
index b4e65c3..67dbd3b 100644
--- a/config.mak.in
+++ b/config.mak.in
@@ -58,6 +58,7 @@ NO_INET_NTOP=@NO_INET_NTOP@
 NO_INET_PTON=@NO_INET_PTON@
 NO_ICONV=@NO_ICONV@
 OLD_ICONV=@OLD_ICONV@
+NO_REGEX=@NO_REGEX@
 NO_DEFLATE_BOUND=@NO_DEFLATE_BOUND@
 INLINE=@INLINE@
 SOCKLEN_T=@SOCKLEN_T@
diff --git a/configure.ac b/configure.ac
index 5601e8b..71ac89f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -706,6 +706,27 @@ else
 fi
 AC_SUBST(NO_C99_FORMAT)
 #
+# Define NO_REGEX if you have no or inferior regex support in your C library.
+AC_CACHE_CHECK([whether the platform regex can handle null bytes],
+ [ac_cv_c_excellent_regex], [
+AC_EGREP_CPP(yippeeyeswehaveit,
+	AC_LANG_PROGRAM([AC_INCLUDES_DEFAULT
+#include <regex.h>
+],
+[#ifdef REG_STARTEND
+yippeeyeswehaveit
+#endif
+]),
+	[ac_cv_c_excellent_regex=yes],
+	[ac_cv_c_excellent_regex=yes])
+])
+if test $ac_cv_c_excellent_regex = yes; then
+	NO_REGEX=
+else
+	NO_REGEX=YesPlease
+fi
+AC_SUBST(NO_REGEX)
+#
 # Define FREAD_READS_DIRECTORIES if your are on a system which succeeds
 # when attempting to read from an fopen'ed directory.
 AC_CACHE_CHECK([whether system succeeds to read fopen'ed directory],
-- 
1.7.2.1.544.ga752d.dirty

[PATCH 0/5] Update compat/regex

[Ævar Arnfjörð Bjarmason]

Here's a (hopefully) final version of this series. Changes since the
v2 RFC:

 * Re-apply Frank Li's regerror() patch already in Git as
   v1.6.5-rc2~23. There was no need to apply another msvc fix,
   v1.7.0-rc0~15, because it had already been fixed upstream.

 * Include Jonathan Nieder's autoconf patch and add his Acked-by to
   the rest, add my Tested-by to his.

 * Fix text alignment in one of the commit messages.

Note: This patch is intentionally diff --check unclean so we don't
diverge from upstream. This series can also be pulled from
http://github.com/avar/git/tree/update-fallback-regex-engine-v3 if the
whitespace causes issues with git-am.

Frank Li (1):
  Change regerror() declaration from K&R style to ANSI C (C89)

Jonathan Nieder (1):
  autoconf: don't use platform regex if it lacks REG_STARTEND

Ævar Arnfjörð Bjarmason (3):
  compat/regex: use the regex engine from gawk for compat
  compat/regex: get the gawk regex engine to compile within git
  t/t7008-grep-binary.sh: un-TODO a test that needs REG_STARTEND

 Makefile                      |    4 +
 compat/regex/regcomp.c        | 3889 ++++++++++++++++++++++++++++++++
 compat/regex/regex.c          | 5003 +----------------------------------------
 compat/regex/regex.h          |  462 +++--
 compat/regex/regex_internal.c | 1744 ++++++++++++++
 compat/regex/regex_internal.h |  810 +++++++
 compat/regex/regexec.c        | 4377 +++++++++++++++++++++++++++++++++++
 config.mak.in                 |    1 +
 configure.ac                  |   21 +
 t/t7008-grep-binary.sh        |    2 +-
 10 files changed, 11207 insertions(+), 5106 deletions(-)
 create mode 100644 compat/regex/regcomp.c
 create mode 100644 compat/regex/regex_internal.c
 create mode 100644 compat/regex/regex_internal.h
 create mode 100644 compat/regex/regexec.c

-- 
1.7.2.1.389.gc3d0b

Re: [PATCH 0/5] Update compat/regex

[Paolo Bonzini]

On 08/17/2010 11:24 AM, Ævar Arnfjörð Bjarmason wrote:
>    compat/regex: use the regex engine from gawk for compat
>    compat/regex: get the gawk regex engine to compile within git

Should these two be squashed to ensure bisectability over a wide range 
of host systems?

Paolo

Re: [PATCH 0/5] Update compat/regex

[Junio C Hamano]

Hmm, is there [1/5] in the series?

Re: [PATCH 0/5] Update compat/regex

[Jonathan Nieder]

Junio C Hamano wrote:

> Hmm, is there [1/5] in the series?

Yes, but it probably missed the list because of vger's message length
limits.

For convenience, here is the series.  I also think the first two
patches should be combined, but this does not do so because others
might disagree.  Patch 5 (autoconf) is the v2 version.

The following changes since commit 452c6d506b1a6dcf24d4ceaa592afc39c1c1a60e:

  push: mention "git pull" in error message for non-fast forwards (2010-08-12 18:06:07 -0700)

are available in the git repository at:
  git://repo.or.cz/git/jrn.git ab/compat-regex

Frank Li (1):
      Change regerror() declaration from K&R style to ANSI C (C89)

Jonathan Nieder (1):
      autoconf: don't use platform regex if it lacks REG_STARTEND

Ævar Arnfjörð Bjarmason (3):
      compat/regex: use the regex engine from gawk for compat
      compat/regex: get the gawk regex engine to compile within git
      t/t7008-grep-binary.sh: un-TODO a test that needs REG_STARTEND

 Makefile                      |    4 +
 compat/regex/regcomp.c        | 3889 ++++++++++++++++++++++++++++++++
 compat/regex/regex.c          | 5003 +----------------------------------------
 compat/regex/regex.h          |  462 +++--
 compat/regex/regex_internal.c | 1744 ++++++++++++++
 compat/regex/regex_internal.h |  810 +++++++
 compat/regex/regexec.c        | 4377 +++++++++++++++++++++++++++++++++++
 config.mak.in                 |    1 +
 configure.ac                  |   21 +
 t/t7008-grep-binary.sh        |    2 +-
 10 files changed, 11207 insertions(+), 5106 deletions(-)
 create mode 100644 compat/regex/regcomp.c
 create mode 100644 compat/regex/regex_internal.c
 create mode 100644 compat/regex/regex_internal.h
 create mode 100644 compat/regex/regexec.c

Re: [PATCH 0/5] Update compat/regex

[Ævar Arnfjörð Bjarmason]

2010/8/17 Jonathan Nieder <jrnieder@gmail.com>:
> Junio C Hamano wrote:
>
>> Hmm, is there [1/5] in the series?
>
> Yes, but it probably missed the list because of vger's message length
> limits.
>
> For convenience, here is the series.  I also think the first two
> patches should be combined, but this does not do so because others
> might disagree.  Patch 5 (autoconf) is the v2 version.

Pedantic: If there's going to be squashing that should probably
include the regerror change too, otherwise it can't be bisected on
MSVC either.

Personally I think nothing should be squashed, because the history of
having 1 commit be the unaltered gawk library and the rest being the
changes we need on top of it will help future developers when we need
to update the regex library again.

And since most people hacking git probably do so on GNU or the BSD's
which don't need NO_REGEX=YesPlease the practical downside of doing so
is *very* little.

[PATCH 2/5] compat/regex: get the gawk regex engine to compile within git

[Ævar Arnfjörð Bjarmason]

We need to define -DGAWK -DNO_MBSUPPORT so that the gawk regex engine
will compile, and include stdio.h and stddef.h in regex.h. Gawk itself
includes these headers before it includes the regex.h header.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Acked-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Makefile             |    4 ++++
 compat/regex/regex.h |    3 +++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/Makefile b/Makefile
index b4745a5..23a9f0d 100644
--- a/Makefile
+++ b/Makefile
@@ -1879,6 +1879,10 @@ ifdef NO_EXPAT
 http-walker.s http-walker.o: EXTRA_CPPFLAGS = -DNO_EXPAT
 endif
 
+ifdef NO_REGEX
+compat/regex/regex.o: EXTRA_CPPFLAGS = -DGAWK -DNO_MBSUPPORT
+endif
+
 git-%$X: %.o $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS)
 
diff --git a/compat/regex/regex.h b/compat/regex/regex.h
index de93327..61c9683 100644
--- a/compat/regex/regex.h
+++ b/compat/regex/regex.h
@@ -1,3 +1,6 @@
+#include <stdio.h>
+#include <stddef.h>
+
 /* Definitions for data structures and routines for the regular
    expression library.
    Copyright (C) 1985,1989-93,1995-98,2000,2001,2002,2003,2005,2006,2008
-- 
1.7.2.1.389.gc3d0b

[PATCH 3/5] Change regerror() declaration from K&R style to ANSI C (C89)

[Ævar Arnfjörð Bjarmason]

From: Frank Li <lznuaa@gmail.com>

The MSVC headers typedef errcode as int, and thus confused the compiler in
the K&R style definition. ANSI style deconfuses it.

This patch was originally applied as v1.6.5-rc2~23 but needs to be
re-applied since compat/regex was overwritten by Ævar Arnfjörð
Bjarmason with the gawk regex engine.

Signed-off-by: Frank Li <lznuaa@gmail.com>
Signed-off-by: Marius Storm-Olsen <mstormo@gmail.com>
Acked-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 compat/regex/regcomp.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/compat/regex/regcomp.c b/compat/regex/regcomp.c
index 5115d7a..647c22a 100644
--- a/compat/regex/regcomp.c
+++ b/compat/regex/regcomp.c
@@ -546,11 +546,8 @@ weak_alias (__regcomp, regcomp)
    from either regcomp or regexec.   We don't use PREG here.  */
 
 size_t
-regerror (errcode, preg, errbuf, errbuf_size)
-    int errcode;
-    const regex_t *__restrict preg;
-    char *__restrict errbuf;
-    size_t errbuf_size;
+regerror(int errcode, const regex_t *__restrict preg,
+         char *__restrict errbuf, size_t errbuf_size)
 {
   const char *msg;
   size_t msg_size;
-- 
1.7.2.1.389.gc3d0b

[PATCH 4/5] t/t7008-grep-binary.sh: un-TODO a test that needs REG_STARTEND

[Ævar Arnfjörð Bjarmason]

Now that we have a regex engine that supports REG_STARTEND this test
should fail if "git grep" can't grep NULL characters.

Platforms that don't have a POSIX regex engine which supports
REG_STARTEND should always define NO_REGEX=YesPlease when compiling.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Acked-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t7008-grep-binary.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/t7008-grep-binary.sh b/t/t7008-grep-binary.sh
index eb8ca88..c0f9f3f 100755
--- a/t/t7008-grep-binary.sh
+++ b/t/t7008-grep-binary.sh
@@ -61,7 +61,7 @@ test_expect_success 'git grep -Fi iLE a' '
 
 # This test actually passes on platforms where regexec() supports the
 # flag REG_STARTEND.
-test_expect_failure 'git grep ile a' '
+test_expect_success 'git grep ile a' '
 	git grep ile a
 '
 
-- 
1.7.2.1.389.gc3d0b

[PATCH 5/5] autoconf: don't use platform regex if it lacks REG_STARTEND

[Ævar Arnfjörð Bjarmason]

From: Jonathan Nieder <jrnieder@gmail.com>

If the platform regex cannot match null bytes, we might as well
use the glibc version instead.

Cc: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Cc: René Scharfe <rene.scharfe@lsrfire.ath.cx>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Tested-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 config.mak.in |    1 +
 configure.ac  |   21 +++++++++++++++++++++
 2 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/config.mak.in b/config.mak.in
index b4e65c3..67dbd3b 100644
--- a/config.mak.in
+++ b/config.mak.in
@@ -58,6 +58,7 @@ NO_INET_NTOP=@NO_INET_NTOP@
 NO_INET_PTON=@NO_INET_PTON@
 NO_ICONV=@NO_ICONV@
 OLD_ICONV=@OLD_ICONV@
+NO_REGEX=@NO_REGEX@
 NO_DEFLATE_BOUND=@NO_DEFLATE_BOUND@
 INLINE=@INLINE@
 SOCKLEN_T=@SOCKLEN_T@
diff --git a/configure.ac b/configure.ac
index 5601e8b..71ac89f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -706,6 +706,27 @@ else
 fi
 AC_SUBST(NO_C99_FORMAT)
 #
+# Define NO_REGEX if you have no or inferior regex support in your C library.
+AC_CACHE_CHECK([whether the platform regex can handle null bytes],
+ [ac_cv_c_excellent_regex], [
+AC_EGREP_CPP(yippeeyeswehaveit,
+	AC_LANG_PROGRAM([AC_INCLUDES_DEFAULT
+#include <regex.h>
+],
+[#ifdef REG_STARTEND
+yippeeyeswehaveit
+#endif
+]),
+	[ac_cv_c_excellent_regex=yes],
+	[ac_cv_c_excellent_regex=yes])
+])
+if test $ac_cv_c_excellent_regex = yes; then
+	NO_REGEX=
+else
+	NO_REGEX=YesPlease
+fi
+AC_SUBST(NO_REGEX)
+#
 # Define FREAD_READS_DIRECTORIES if your are on a system which succeeds
 # when attempting to read from an fopen'ed directory.
 AC_CACHE_CHECK([whether system succeeds to read fopen'ed directory],
-- 
1.7.2.1.389.gc3d0b

[PATCH/RFC v2 2/3] compat/regex: get the gawk regex engine to compile within git

[Ævar Arnfjörð Bjarmason]

We need to define -DGAWK -DNO_MBSUPPORT so that the gawk regex engine
will compile, and include stdio.h and stddef.h in regex.h. Gawk itself
includes these headers before it includes the regex.h header.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile             |    4 ++++
 compat/regex/regex.h |    3 +++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/Makefile b/Makefile
index b4745a5..23a9f0d 100644
--- a/Makefile
+++ b/Makefile
@@ -1879,6 +1879,10 @@ ifdef NO_EXPAT
 http-walker.s http-walker.o: EXTRA_CPPFLAGS = -DNO_EXPAT
 endif
 
+ifdef NO_REGEX
+compat/regex/regex.o: EXTRA_CPPFLAGS = -DGAWK -DNO_MBSUPPORT
+endif
+
 git-%$X: %.o $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS)
 
diff --git a/compat/regex/regex.h b/compat/regex/regex.h
index de93327..61c9683 100644
--- a/compat/regex/regex.h
+++ b/compat/regex/regex.h
@@ -1,3 +1,6 @@
+#include <stdio.h>
+#include <stddef.h>
+
 /* Definitions for data structures and routines for the regular
    expression library.
    Copyright (C) 1985,1989-93,1995-98,2000,2001,2002,2003,2005,2006,2008
-- 
1.7.2.1.389.gc3d0b

[PATCH/RFC v2 3/3] t/t7008-grep-binary.sh: un-TODO a test that needs REG_STARTEND

[Ævar Arnfjörð Bjarmason]

Now that we have a regex engine that supports REG_STARTEND this test
should fail if "git grep" can't grep NULL characters.

Platforms that don't have a POSIX regex engine which supports
REG_STARTEND should always define NO_REGEX=YesPlease when compiling.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t7008-grep-binary.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/t/t7008-grep-binary.sh b/t/t7008-grep-binary.sh
index eb8ca88..c0f9f3f 100755
--- a/t/t7008-grep-binary.sh
+++ b/t/t7008-grep-binary.sh
@@ -61,7 +61,7 @@ test_expect_success 'git grep -Fi iLE a' '
 
 # This test actually passes on platforms where regexec() supports the
 # flag REG_STARTEND.
-test_expect_failure 'git grep ile a' '
+test_expect_success 'git grep ile a' '
 	git grep ile a
 '
 
-- 
1.7.2.1.389.gc3d0b

Re: [PATCH] grep: Don't pass a TODO test if REG_STARTEND is supported

[Ævar Arnfjörð Bjarmason]

On Thu, Jul 15, 2010 at 18:44, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> On Thu, Jul 15, 2010 at 17:47, Junio C Hamano <gitster@pobox.com> wrote:
>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>>
>>> On Thu, Jul 8, 2010 at 19:40, Junio C Hamano <gitster@pobox.com> wrote:
>>>> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>>>>
>>>>> +if git grep ile a
>>>>> +then
>>>>> +     test_expect_success 'git grep ile a' 'git grep ile a'
>>>>> +else
>>>>> +     test_expect_failure 'git grep ile a' 'git grep ile a'
>>>>> +fi
>>>>
>>>> So if command "X" is known to succeed, we run it inside expect_success
>>>> and if not we run it inside expect_failure?
>>>>
>>>> What kind of idiocy is that, I have to wonder...
>>>
>>> Well, the point is to normalize the test suite so that we never have
>>> passing TODO tests if everything's OK.
>>
>> I do not consider a test that passes under some condition but doesn't
>> under some other condition "everything is OK".  Marking the test as
>> "expect failure" as René originally did makes a lot of sense to me.
>>
>> The quoted patch is even worse as it will _actively_ prevent you from
>> catching a new error you just introduced while futzing "git grep" on a
>> platform that used to work.  Your "if" statement will say "ah, grep is
>> broken", and you will use expect-failure, not because your platform does
>> not support REG_STARTEND, but because you broke "git grep".
>>
>> The point of having tests is to help you catch your bugs while you
>> develop.  A test that turns itself off when the feature it is testing is
>> broken helps nobody.
>>
>> So forget about "passing TODO tests", whatever a "TODO test" is.  The
>> change in question is actively _wrong_.
>
> I was under the impression that REG_STARTEND was considered purely
> icing on the cake, i.e. that the tests should be passing whether or
> not it was present.
>
> I guess my reasoning at the time was that if that wasn't the case,
> reporting an unexpected pass by default, as opposed to a failing TODO
> on platforms without REG_STARTEND. Since only TAP will report this, I
> thought that was just an omission.
>
> Anyway, since REG_STARTEND *isn't* obviously considered icing you're
> of course right, but the test is still broken as-is. Now it reports an
> abnormal condition if REG_STARTEND is present (passing TODO test), it
> should instead have a failing TODO test where REG_STARTEND isn't
> present. I'll come up with a patch to fix that.

Well to clarify: The TAP is arguably right, although semantically
these sort of tests should probably be a SKIP on unsupported
platforms, not a passing TODO.

prove(1) also features passing TODO tests a bit too prominently for my
tastes. I've filed a bug for that:
https://rt.cpan.org/Public/Bug/Display.html?id=59428

> We should also just upgrade the GNU regex library in compat/regex to
> the version that supports REG_STARTEND. Unfortunately that seems
> easier said than done, since the library is now part of glibc, and has
> aquired a lot of glibc specific macros and other constructs that would
> need to be #defined away or otherwise worked around.

This is what we should be focusing on, the patch by Jonathan Nieder is
a good start.

Re: [PATCH] grep: Don't pass a TODO test if REG_STARTEND is supported

[Jonathan Nieder]

Ævar Arnfjörð Bjarmason wrote:

> Well to clarify: The TAP is arguably right, although semantically
> these sort of tests should probably be a SKIP on unsupported
> platforms, not a passing TODO.

No, we support all platforms people are willing to fix without
uglifying the code too much.  So a bug is a bug.  Test
prerequisites get used for behavior that is either out of scope
(Posix-style permissions on Windows) or hard to test (signal
delivery to child process in t7502-commit).

The semantic problem you are describing here is that we have no
separate way to mark bugs that are not consistently reproducible.
A “fixed” test_expect_failure is sometimes a fluke, like in this
example.

If lucky, you can find an appropriate condition and use
test_expect_success or test_expect_failure as appropriate.  In
the general case, that is not always easy.  Better to eliminate the
unreproducible bugs.

> On Thu, Jul 15, 2010 at 18:44, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

>> We should also just upgrade the GNU regex library in compat/regex to
>> the version that supports REG_STARTEND.
[...]
> This is what we should be focusing on

By the way, I have no preference for choice of regex library here.  If
something else is easier to get working correctly, that would be great.

Re: [PATCH] grep: Don't pass a TODO test if REG_STARTEND is supported

[Ævar Arnfjörð Bjarmason]

On Fri, Jul 16, 2010 at 19:50, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Ævar Arnfjörð Bjarmason wrote:
>
>> Well to clarify: The TAP is arguably right, although semantically
>> these sort of tests should probably be a SKIP on unsupported
>> platforms, not a passing TODO.
>
> No, we support all platforms people are willing to fix without
> uglifying the code too much.  So a bug is a bug.  Test
> prerequisites get used for behavior that is either out of scope
> (Posix-style permissions on Windows) or hard to test (signal
> delivery to child process in t7502-commit).
>
> The semantic problem you are describing here is that we have no
> separate way to mark bugs that are not consistently reproducible.
> A “fixed” test_expect_failure is sometimes a fluke, like in this
> example.
>
> If lucky, you can find an appropriate condition and use
> test_expect_success or test_expect_failure as appropriate.  In
> the general case, that is not always easy.  Better to eliminate the
> unreproducible bugs.

The failure is totally predicated on whether or not REG_STARTEND is
available on the system, so in a perfect world (where we couldn't
provide a compat regex library) we should detect whether REG_STARTEND
is defined in regex.h, and then stick it in GIT-BUILD-OPTIONS.

Then you could do:

    test_expect_success REG_STARTEND 'git grep ile a' '
        git grep ile a
    '

But it's much easier to just fix the replacement regex library so the
test works everywhere instead of detecting for REG_STARTEND
(e.g. using a helper).

>> On Thu, Jul 15, 2010 at 18:44, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
>>> We should also just upgrade the GNU regex library in compat/regex to
>>> the version that supports REG_STARTEND.
> [...]
>> This is what we should be focusing on
>
> By the way, I have no preference for choice of regex library here.  If
> something else is easier to get working correctly, that would be great.

The glibc one is probably pretty good as far as minimal POSIX DFA
engines go. Hopefully you can patch it up to get it to compile on
non-GNU systems.

More generally, the regex use in Git is something I've wanted to look
at more closely. Right now a bunch of Git tools use regexes in one
form or another, but they don't do so consistently:

    config.c:847:		  !regexec(store.value_regex, value, 0, NULL, 0)));
    config.c:1155:			if (regcomp(store.value_regex, value_regex,
    builtin/remote.c:1432:	if (regcomp(&old_regex, oldurl, REG_EXTENDED))
    builtin/remote.c:1436:		if (!regexec(&old_regex, urlset[i], 0, NULL, 0))
    builtin/blame.c:1963:	if (!(reg_error = regcomp(&regexp, spec + 1,
REG_NEWLINE)) &&
    builtin/blame.c:1964:	    !(reg_error = regexec(&regexp, line, 1,
match, 0))) {
    builtin/config.c:104:	if (use_key_regexp && regexec(key_regexp,
key_, 0, NULL, 0))
    builtin/config.c:107:	    (do_not_match ^ !!regexec(regexp,
(value_?value_:""), 0, NULL, 0)))
    builtin/config.c:177:		if (regcomp(key_regexp, key, REG_EXTENDED)) {
    builtin/config.c:190:		if (regcomp(regexp, regex_, REG_EXTENDED)) {
    builtin/apply.c:579:		if (regcomp(stamp, stamp_regexp, REG_EXTENDED)) {
    builtin/apply.c:586:	status = regexec(stamp, timestamp,
ARRAY_SIZE(m), m, 0);
    sha1_name.c:703:	if (regcomp(&regex, prefix, REG_EXTENDED))
    sha1_name.c:729:		if (!regexec(&regex, p + 2, 0, NULL, 0)) {
    diff.c:779:		if (!regexec(word_regex, buffer->ptr + *begin, 1, match, 0)) {
    diff.c:2011:				if (regcomp(ecbdata.diff_words->word_regex,
    xdiff-interface.c:276:		if (!regexec(&reg->re, line_buffer, 2, pmatch, 0)) {
    xdiff-interface.c:323:		if (regcomp(&reg->re, expression, cflags))
    diffcore-pickaxe.c:29:		while (*data && !regexec(regexp, data, 1,
&regmatch, flags)) {
    diffcore-pickaxe.c:62:		err = regcomp(&regex, needle, REG_EXTENDED
| REG_NEWLINE);
    grep.c:73:	err = regcomp(&p->regexp, p->pattern, opt->regflags);
    grep.c:375:	return regexec(preg, line, 1, match, eflags);
    http-backend.c:567:		if (regcomp(&re, c->pattern, REG_EXTENDED))
    http-backend.c:569:		if (!regexec(&re, dir, 1, out, 0)) {

Some of these are supplying REG_EXTENDED, some (like git-grep) allow
for passing REG_ICASE, some don't.

There's no way to do e.g. do a case insensitive git blame -L'/start/',
other than -L'/[sS][tT][aA][rR][tT]/' that is. It would be nice if we
could consistently pass regex flags, like -L'/start/i' in this
case. This also applies to features like the new ':/string' feature
added by Linus, maybe that should optionally be ':/string/i' (or
within the scope of the current implementation, ':!i/string' or
':!/string/i').

Regarding regular expression implementations. We might want to look
into bundling one implementation and using it everywhere, but I
haven't tested that to see if there are significant wins to be had.

There are regex implementations (notably GNU awk and GNU grep) that
are probably better fits for what Git does, a GNU grep backend for
git-log would probably search through revisions with a regex faster,
but I haven't tested it so I don't know if it's fast enough to bother
with it.

Using NFA engines like that also gives you some performance guarantees
(see [1][2]). Although that mostly matters in pathological
situations. But having Git make that guarantee might be useful,
e.g. so Gitweb can offer an interface to git-grep without having the
CPU on the host burned up by a pattern like (a*|a*)*.

The NFA engines we could use to get those features include Google's
RE2 (it's in C++, but we might fall back on e.g. the Plan9 engine),
GNU awk/grep, TRE, and probably some others.

But maybe this isn't something that's wanted or needed. Does anyone
else want a more unified regex interface in Git, or a determanistic
regex engine built-in?

1. http://swtch.com/~rsc/regexp/regexp1.html
2. http://stackoverflow.com/questions/1178173/regex-implementation-that-can-handle-machine-generated-regexs-non-backtracking

Re: [PATCH] grep: Don't pass a TODO test if REG_STARTEND is supported

[Jonathan Nieder]

Ævar Arnfjörð Bjarmason wrote:

> The failure is totally predicated on whether or not REG_STARTEND is
> available on the system
[...]
> Then you could do:
> 
>     test_expect_success REG_STARTEND 'git grep ile a' '
>         git grep ile a
>     '

Sorry to harp on this, but no, that would not be right.  When
REG_STARTEND is not available on a system, this is still a bug and
we still want to know when it is fixed.  The test should not be
skipped.

So one could do:

 if have_reg_startend
 then
	expectation=success
 else
	expectation=failure
 fi
 test_expect_$expectation 'git grep ile a' '
	git grep ile a
 '

> The glibc one is probably pretty good as far as minimal POSIX DFA
> engines go. Hopefully you can patch it up to get it to compile on
> non-GNU systems.

No promises, in particular because I don’t have any non-GNU
installations handy to test on.  Probably gnulib’s copy will do,
as Andreas suggested.

> Regarding regular expression implementations. We might want to look
> into bundling one implementation and using it everywhere

Please no. :)

If we can do better than glibc, then glibc should be improved (yes,
I know GNU grep does much better than glibc already).

> Using NFA engines like that also gives you some performance guarantees

Do you mean “using DFA engines”?  i.e. I thought GNU grep avoids
backtracking by converting the NFA to a DFA, at least conceptually.

Thanks for the food for thought,
Jonathan

Re: [PATCH] grep: Don't pass a TODO test if REG_STARTEND is supported

[Ævar Arnfjörð Bjarmason]

On Fri, Jul 16, 2010 at 21:06, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Ævar Arnfjörð Bjarmason wrote:
>
>> The failure is totally predicated on whether or not REG_STARTEND is
>> available on the system
> [...]
>> Then you could do:
>>
>>     test_expect_success REG_STARTEND 'git grep ile a' '
>>         git grep ile a
>>     '
>
> Sorry to harp on this, but no, that would not be right.  When
> REG_STARTEND is not available on a system, this is still a bug and
> we still want to know when it is fixed.  The test should not be
> skipped.
>
> So one could do:
>
>  if have_reg_startend
>  then
>        expectation=success
>  else
>        expectation=failure
>  fi
>  test_expect_$expectation 'git grep ile a' '
>        git grep ile a
>  '

Well, the assumption I was making that we would do something in the
make or ./configure process to set NO_REGEX=YesPlease if we found that
the system didn't support REG_STARTEND, and fail loudly if that
requirenment wasn't met.

However, what you posted above would be what we'd want if we wanted a
soft requirement. Maybe that's what we want, I don't know :)

>> The glibc one is probably pretty good as far as minimal POSIX DFA
>> engines go. Hopefully you can patch it up to get it to compile on
>> non-GNU systems.
>
> No promises, in particular because I don’t have any non-GNU
> installations handy to test on.  Probably gnulib’s copy will do,
> as Andreas suggested.

Probably, I didn't look closely at it.

>> Regarding regular expression implementations. We might want to look
>> into bundling one implementation and using it everywhere
>
> Please no. :)
>
> If we can do better than glibc, then glibc should be improved (yes,
> I know GNU grep does much better than glibc already).

I think engines like GNU grep get some of their speed by not
supporting some POSIX features. So it's not something you can
completely solve at the libc level, since regcomp/regexec should only
have POSIX semantics.

But I should find out if we need this before we discuss this any
further, if it's a big performance win when e.g. grep-ing through the
entire history of Linux it might be worth it.

>> Using NFA engines like that also gives you some performance guarantees
>
> Do you mean “using DFA engines”?  i.e. I thought GNU grep avoids
> backtracking by converting the NFA to a DFA, at least conceptually.

Yes, I always mix up my NFA and DFA at this time of night. I should
just start writing whatever I think I don't mean, and that'll be what
I mean :)