threads / patch / 24326

patchgrep: Don't pass a TODO test if REG_STARTEND is supported

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

## tl;dr

36 messages between Jul 8, 2010 and Aug 18, 2010. Diffs are folded; open one to read it.

replies: 35people: 6as markdown or json

Ævar Arnfjörð Bjarmason· Jul 8, 2010, 00:42 UTC · lore

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(-)
Show changes to t/t7008-grep-binary.sh +10 −5
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
Junio C Hamano· Jul 8, 2010, 19:40 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
Show 6 quoted lines
> +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...
Ævar Arnfjörð Bjarmason· Jul 8, 2010, 20:09 UTC · re: Junio C Hamano · lore

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

On Thu, Jul 8, 2010 at 19:40, Junio C Hamano <gitster@pobox.com> wrote:
Show 13 quoted lines
> Æ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.

René Scharfe· Jul 8, 2010, 21:58 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Am 08.07.2010 22:09, schrieb Ævar Arnfjörð Bjarmason:
Show 17 quoted lines
> 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é
Ævar Arnfjörð Bjarmason· Jul 15, 2010, 15:32 UTC · re: René Scharfe · lore

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

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?

Junio C Hamano· Jul 15, 2010, 17:47 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 17 quoted lines
> 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_.

Ævar Arnfjörð Bjarmason· Jul 15, 2010, 18:44 UTC · re: Junio C Hamano · lore

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

On Thu, Jul 15, 2010 at 17:47, Junio C Hamano <gitster@pobox.com> wrote:
Show 36 quoted lines
> Æ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.
Ævar Arnfjörð Bjarmason· Jul 16, 2010, 14:33 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

On Thu, Jul 15, 2010 at 18:44, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
Show 52 quoted lines
> 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

Show 5 quoted lines
> 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.

Jonathan Nieder· Jul 16, 2010, 19:50 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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

Ævar Arnfjörð Bjarmason· Jul 16, 2010, 20:51 UTC · re: Jonathan Nieder · lore

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

On Fri, Jul 16, 2010 at 19:50, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 21 quoted lines
> Æ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).

Show 9 quoted lines
>> 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
Jonathan Nieder· Jul 16, 2010, 21:06 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Ævar Arnfjörð Bjarmason wrote:
> The failure is totally predicated on whether or not REG_STARTEND is
> available on the system
[...]
Show 5 quoted lines
> 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

Ævar Arnfjörð Bjarmason· Jul 16, 2010, 21:19 UTC · re: Jonathan Nieder · lore

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

On Fri, Jul 16, 2010 at 21:06, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 27 quoted lines
> Æ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 :)

Show 7 quoted lines
>> 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.
Show 7 quoted lines
>> 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 :)

Ævar Arnfjörð Bjarmason· Jul 16, 2010, 13:58 UTC · lore

Re: [RFC/PATCH] Update compat/regex

On Thu, Jul 15, 2010 at 22:00, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 12 quoted lines
> 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.
Show 11 quoted lines
> 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
Andreas Schwab· Jul 16, 2010, 14:17 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [RFC/PATCH] Update compat/regex

Æ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."
Ævar Arnfjörð Bjarmason· Aug 15, 2010, 11:08 UTC · re: Andreas Schwab · lore

Re: [RFC/PATCH] Update compat/regex

On Fri, Jul 16, 2010 at 14:17, Andreas Schwab <schwab@linux-m68k.org> wrote:
Show 7 quoted lines
> Æ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.

Paolo Bonzini· Aug 16, 2010, 12:26 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [RFC/PATCH] Update compat/regex

On 08/15/2010 01:08 PM, Ævar Arnfjörð Bjarmason wrote:
Show 16 quoted lines
> 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
Ævar Arnfjörð Bjarmason· Aug 17, 2010, 03:25 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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
Ævar Arnfjörð Bjarmason· Aug 17, 2010, 03:25 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to 2 files +9 −2

Makefile, compat/regex/regex.h

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
Jonathan Nieder· Aug 17, 2010, 03:35 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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.
Show 14 quoted lines
> --- 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.
Ævar Arnfjörð Bjarmason· Aug 17, 2010, 03:25 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to t/t7008-grep-binary.sh +1 −1
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
Ævar Arnfjörð Bjarmason· Aug 17, 2010, 05:17 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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
Jonathan Nieder· Aug 17, 2010, 08:03 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Æ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(-)
Show changes to 2 files +22 −0

config.mak.in, configure.ac

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
Ævar Arnfjörð Bjarmason· Aug 17, 2010, 09:24 UTC · re: Jonathan Nieder · lore

[PATCH 0/5] Update compat/regex

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
Paolo Bonzini· Aug 17, 2010, 11:46 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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
Jonathan Nieder· Aug 17, 2010, 23:50 UTC · re: Junio C Hamano · lore

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

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
Ævar Arnfjörð Bjarmason· Aug 18, 2010, 10:41 UTC · re: Jonathan Nieder · lore

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

2010/8/17 Jonathan Nieder <jrnieder@gmail.com>:
Show 10 quoted lines
> 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.

Ævar Arnfjörð Bjarmason· Aug 17, 2010, 09:24 UTC · re: Jonathan Nieder · lore

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

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(-)
Show changes to 2 files +7 −0

Makefile, compat/regex/regex.h

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
Ævar Arnfjörð Bjarmason· Aug 17, 2010, 09:24 UTC · re: Jonathan Nieder · lore

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

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(-)
Show changes to compat/regex/regcomp.c +2 −5
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
Ævar Arnfjörð Bjarmason· Aug 17, 2010, 09:24 UTC · re: Jonathan Nieder · lore

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

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(-)
Show changes to t/t7008-grep-binary.sh +1 −1
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
Ævar Arnfjörð Bjarmason· Aug 17, 2010, 09:24 UTC · re: Jonathan Nieder · lore

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

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(-)
Show changes to 2 files +22 −0

config.mak.in, configure.ac

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
Ævar Arnfjörð Bjarmason· Aug 17, 2010, 05:17 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to 2 files +7 −0

Makefile, compat/regex/regex.h

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
Ævar Arnfjörð Bjarmason· Aug 17, 2010, 05:17 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to t/t7008-grep-binary.sh +1 −1
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
Jonathan Nieder· Aug 17, 2010, 03:37 UTC · lore

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

Ævar Arnfjörð Bjarmason wrote:
Show 16 quoted lines
> +++ 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.)

Ævar Arnfjörð Bjarmason· Aug 17, 2010, 03:50 UTC · re: Jonathan Nieder · lore

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

On Tue, Aug 17, 2010 at 03:37, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 21 quoted lines
> Æ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.

Jonathan Nieder· Aug 17, 2010, 04:08 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Ævar Arnfjörð Bjarmason wrote:
> On Tue, Aug 17, 2010 at 03:37, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> Ævar Arnfjörð Bjarmason wrote:
Show 22 quoted lines
>>> +++ 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.

← back to recent threads