This update drops 1/12, which is an unnecessary change, and changes a
couple of echo/printf to test_write_lines. One of those echo uses
backlashes and causes problems with Debian dash.
Interdiff therefore is not really interesting
This is in preparation to turn test-regex into some generic regex
testing command.
Helped-by: Eric Sunshine [off-list ref]
Helped-by: Ramsay Jones [off-list ref]
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
t/t0070-fundamental.sh | 2 +-
test-regex.c | 12 ++++++++++--
2 files changed, 11 insertions(+), 3 deletions(-)
@@ -31,7 +31,7 @@ test_expect_success 'git_mkstemps_mode does not fail if fd 0 is not open' ' test_expect_success'check for a bug in the regex routines''# if this test fails, re-build git with NO_REGEX=1-test-regex+test-regex--bug' test_done
@@ -16,5 +16,13 @@ int main(int argc, char **argv)if(m[0].rm_so==3)/* matches '\n' when it should not */die("regex bug confirmed: re-build git with NO_REGEX=1");-exit(0);+return0;+}++intmain(intargc,char**argv)+{+if(argc==2&&!strcmp(argv[1],"--bug"))+returntest_regex_bug();+else+usage("test-regex --bug");}
Similar to the previous commit, we can't use kws on icase search
outside ascii range. But we can't simply pass the pattern to
regcomp/pcre like the previous commit because it may contain regex
special characters, so we need to quote the regex first.
To avoid misquote traps that could lead to undefined behavior, we
always stick to basic regex engine in this case. We don't need fancy
features for grepping a literal string anyway.
basic_regex_quote_buf() assumes that if the pattern is in a multibyte
encoding, ascii chars must be unambiguously encoded as single
bytes. This is true at least for UTF-8. For others, let's wait until
people yell up. Chances are nobody uses multibyte, non utf-8 charsets
anymore.
Noticed-by: Plamen Totev [off-list ref]
Helped-by: René Scharfe [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
grep.c | 25 ++++++++++++++++++++++++-
quote.c | 37 +++++++++++++++++++++++++++++++++++++
quote.h | 1 +
t/t7812-grep-icase-non-ascii.sh | 26 ++++++++++++++++++++++++++
4 files changed, 88 insertions(+), 1 deletion(-)
@@ -440,3 +440,40 @@ void tcl_quote_buf(struct strbuf *sb, const char *src)}strbuf_addch(sb,'"');}++voidbasic_regex_quote_buf(structstrbuf*sb,constchar*src)+{+charc;++if(*src=='^'){+/* only beginning '^' is special and needs quoting */+strbuf_addch(sb,'\\');+strbuf_addch(sb,*src++);+}+if(*src=='*')+/* beginning '*' is not special, no quoting */+strbuf_addch(sb,*src++);++while((c=*src++)){+switch(c){+case'[':+case'.':+case'\\':+case'*':+strbuf_addch(sb,'\\');+strbuf_addch(sb,c);+break;++case'$':+/* only the end '$' is special and needs quoting */+if(*src=='\0')+strbuf_addch(sb,'\\');+strbuf_addch(sb,c);+break;++default:+strbuf_addch(sb,c);+break;+}+}+}
There's another regcomp code block coming in this function. By moving
the error handling code out of this block, we don't have to add the
same error handling code in the new block.
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
diffcore-pickaxe.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
@@ -204,20 +204,13 @@ void diffcore_pickaxe(struct diff_options *o)intopts=o->pickaxe_opts;regex_tregex,*regexp=NULL;kwset_tkws=NULL;+interr=0;if(opts&(DIFF_PICKAXE_REGEX|DIFF_PICKAXE_KIND_G)){-interr;intcflags=REG_EXTENDED|REG_NEWLINE;if(DIFF_OPT_TST(o,PICKAXE_IGNORE_CASE))cflags|=REG_ICASE;err=regcomp(®ex,needle,cflags);-if(err){-/* The POSIX.2 people are surely sick */-charerrbuf[1024];-regerror(err,®ex,errbuf,1024);-regfree(®ex);-die("invalid regex: %s",errbuf);-}regexp=®ex;}else{kws=kwsalloc(DIFF_OPT_TST(o,PICKAXE_IGNORE_CASE)
@@ -225,6 +218,13 @@ void diffcore_pickaxe(struct diff_options *o)kwsincr(kws,needle,strlen(needle));kwsprep(kws);}+if(err){+/* The POSIX.2 people are surely sick */+charerrbuf[1024];+regerror(err,®ex,errbuf,1024);+regfree(®ex);+die("invalid regex: %s",errbuf);+}/* Might want to warn when both S and G are on; I don't care... */pickaxe(&diff_queued_diff,o,regexp,kws,
In the previous change in this function, we add locale support for
single-byte encodings only. It looks like pcre only supports utf-* as
multibyte encodings, the others are left in the cold (which is
fine).
We need to enable PCRE_UTF8 so pcre can find character boundary
correctly. It's needed for case folding (when --ignore-case is used)
or '*', '+' or similar syntax is used.
The "has_non_ascii()" check is to be on the conservative side. If
there's non-ascii in the pattern, the searched content could still be
in utf-8, but we can treat it just like a byte stream and everything
should work. If we force utf-8 based on locale only and pcre validates
utf-8 and the file content is in non-utf8 encoding, things break.
Noticed-by: Plamen Totev [off-list ref]
Helped-by: Plamen Totev [off-list ref]
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
grep.c | 2 ++
t/t7812-grep-icase-non-ascii.sh | 15 +++++++++++++++
2 files changed, 17 insertions(+)
Similar to the "grep -F -i" case, we can't use kws on icase search
outside ascii range, so we quote the string and pass it to regcomp as
a basic regexp and let regex engine deal with case sensitivity.
The new test is put in t7812 instead of t4209-log-pickaxe because
lib-gettext.sh might cause problems elsewhere, probably.
Noticed-by: Plamen Totev [off-list ref]
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
diffcore-pickaxe.c | 11 +++++++++++
t/t7812-grep-icase-non-ascii.sh | 7 +++++++
2 files changed, 18 insertions(+)
This function returns true if git is running under an UTF-8
locale. pcre in the next patch will need this.
is_encoding_utf8() is used instead of strcmp() to catch both "utf-8"
and "utf8" suffixes.
When built with no gettext support, we peek in several env variables
to detect UTF-8. pcre library might support utf-8 even if libc is
built without locale support.. The peeking code is a copy from
compat/regex/regcomp.c
Helped-by: Ævar Arnfjörð Bjarmason [off-list ref]
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
gettext.c | 24 ++++++++++++++++++++++--
gettext.h | 1 +
2 files changed, 23 insertions(+), 2 deletions(-)
When we detect the pattern is just a literal string, we avoid heavy
regex engine and use fast substring search implemented in kwsset.c.
But kws uses git-ctype which is locale-independent so it does not know
how to fold case properly outside ascii range. Let regcomp or pcre
take care of this case instead. Slower, but accurate.
Noticed-by: Plamen Totev [off-list ref]
Helped-by: René Scharfe [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
grep.c | 7 ++++++-
t/t7812-grep-icase-non-ascii.sh (new +x) | 23 +++++++++++++++++++++++
2 files changed, 29 insertions(+), 1 deletion(-)
create mode 100755 t/t7812-grep-icase-non-ascii.sh
The default tables are usually built with C locale and only suitable
for LANG=C or similar. This should make case insensitive search work
correctly for all single-byte charsets.
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
grep.c | 8 ++++++--
grep.h | 1 +
t/t7813-grep-icase-iso.sh (new +x) | 19 +++++++++++++++++++
3 files changed, 26 insertions(+), 2 deletions(-)
create mode 100755 t/t7813-grep-icase-iso.sh
@@ -204,20 +204,13 @@ void diffcore_pickaxe(struct diff_options *o)intopts=o->pickaxe_opts;regex_tregex,*regexp=NULL;kwset_tkws=NULL;+interr=0;if(opts&(DIFF_PICKAXE_REGEX|DIFF_PICKAXE_KIND_G)){-interr;intcflags=REG_EXTENDED|REG_NEWLINE;if(DIFF_OPT_TST(o,PICKAXE_IGNORE_CASE))cflags|=REG_ICASE;err=regcomp(®ex,needle,cflags);-if(err){-/* The POSIX.2 people are surely sick */-charerrbuf[1024];-regerror(err,®ex,errbuf,1024);-regfree(®ex);-die("invalid regex: %s",errbuf);-}regexp=®ex;}else{kws=kwsalloc(DIFF_OPT_TST(o,PICKAXE_IGNORE_CASE)
@@ -225,6 +218,13 @@ void diffcore_pickaxe(struct diff_options *o)kwsincr(kws,needle,strlen(needle));kwsprep(kws);}+if(err){+/* The POSIX.2 people are surely sick */+charerrbuf[1024];+regerror(err,®ex,errbuf,1024);+regfree(®ex);+die("invalid regex: %s",errbuf);+}
Hrm. I wondered what happens if we see an error in the kwset code block,
which did not put anything useful in "regex" at all.
It's OK right now, because "err" is newly promoted to the top of the
function, and so we know that kwset cannot call it. But it seems like
an accident waiting to happen. Calling it "regex_err" or something might
help.
But I also wonder if a function wouldn't be better. You could even roll
it up with regcomp, like:
static void regcomp_or_die(regex_t *regex, const char *pattern, int flags)
{
int err = regcomp(regex, pattern, flags);
if (err) {
char buf[1024];
regerror(err, ®ex, buf, sizeof(buf));
regfree(®ex);
die("invalid regex: %s", buf);
}
}
I think you could also skip the regfree(), since we are about to die. I
also think the error message would probably be better if it mentioned
the text of "pattern" itself (since it might be coming from config, or
you might have provided several patterns, or you might have thought
something was supposed to be a non-regex).
-Peff
v2 fixes Junio's and Jeff's comments (both good). The sharing "!icase
|| ascii_only" is made a separate commit (6/12) because I think it
takes some seconds to realize that the conversion is correct and it's
technically not needed in 5/12 (and it's sort of the opposite of 1/12)
Interdiff
@@ -229,13 +240,6 @@ void diffcore_pickaxe(struct diff_options *o)kwsincr(kws,needle,strlen(needle));kwsprep(kws);}-if(err){-/* The POSIX.2 people are surely sick */-charerrbuf[1024];-regerror(err,®ex,errbuf,1024);-regfree(®ex);-die("invalid regex: %s",errbuf);-}/* Might want to warn when both S and G are on; I don't care... */pickaxe(&diff_queued_diff,o,regexp,kws,
When we detect the pattern is just a literal string, we avoid heavy
regex engine and use fast substring search implemented in kwsset.c.
But kws uses git-ctype which is locale-independent so it does not know
how to fold case properly outside ascii range. Let regcomp or pcre
take care of this case instead. Slower, but accurate.
Noticed-by: Plamen Totev [off-list ref]
Helped-by: René Scharfe [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
grep.c | 7 ++++++-
t/t7812-grep-icase-non-ascii.sh (new +x) | 23 +++++++++++++++++++++++
2 files changed, 29 insertions(+), 1 deletion(-)
create mode 100755 t/t7812-grep-icase-non-ascii.sh
The default tables are usually built with C locale and only suitable
for LANG=C or similar. This should make case insensitive search work
correctly for all single-byte charsets.
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
grep.c | 8 ++++++--
grep.h | 1 +
t/t7813-grep-icase-iso.sh (new +x) | 19 +++++++++++++++++++
3 files changed, 26 insertions(+), 2 deletions(-)
create mode 100755 t/t7813-grep-icase-iso.sh
"!icase || ascii_only" is repeated twice in this if/else chain as this
series evolves. Rewrite it (and basically revert the first if
condition back to before the "grep: break down an "if" stmt..." commit).
Helped-by: Junio C Hamano [off-list ref]
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
grep.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
There's another regcomp code block coming in this function that needs
the same error handling. This function can help avoid duplicating
error handling code.
Helped-by: Jeff King [off-list ref]
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
diffcore-pickaxe.c | 22 +++++++++++++---------
1 file changed, 13 insertions(+), 9 deletions(-)
In the previous change in this function, we add locale support for
single-byte encodings only. It looks like pcre only supports utf-* as
multibyte encodings, the others are left in the cold (which is
fine).
We need to enable PCRE_UTF8 so pcre can find character boundary
correctly. It's needed for case folding (when --ignore-case is used)
or '*', '+' or similar syntax is used.
The "has_non_ascii()" check is to be on the conservative side. If
there's non-ascii in the pattern, the searched content could still be
in utf-8, but we can treat it just like a byte stream and everything
should work. If we force utf-8 based on locale only and pcre validates
utf-8 and the file content is in non-utf8 encoding, things break.
Noticed-by: Plamen Totev [off-list ref]
Helped-by: Plamen Totev [off-list ref]
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
grep.c | 2 ++
t/t7812-grep-icase-non-ascii.sh | 15 +++++++++++++++
2 files changed, 17 insertions(+)
Similar to the previous commit, we can't use kws on icase search
outside ascii range. But we can't simply pass the pattern to
regcomp/pcre like the previous commit because it may contain regex
special characters, so we need to quote the regex first.
To avoid misquote traps that could lead to undefined behavior, we
always stick to basic regex engine in this case. We don't need fancy
features for grepping a literal string anyway.
basic_regex_quote_buf() assumes that if the pattern is in a multibyte
encoding, ascii chars must be unambiguously encoded as single
bytes. This is true at least for UTF-8. For others, let's wait until
people yell up. Chances are nobody uses multibyte, non utf-8 charsets
anymore.
Noticed-by: Plamen Totev [off-list ref]
Helped-by: René Scharfe [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
grep.c | 24 +++++++++++++++++++++++-
quote.c | 37 +++++++++++++++++++++++++++++++++++++
quote.h | 1 +
t/t7812-grep-icase-non-ascii.sh | 26 ++++++++++++++++++++++++++
4 files changed, 87 insertions(+), 1 deletion(-)
@@ -440,3 +440,40 @@ void tcl_quote_buf(struct strbuf *sb, const char *src)}strbuf_addch(sb,'"');}++voidbasic_regex_quote_buf(structstrbuf*sb,constchar*src)+{+charc;++if(*src=='^'){+/* only beginning '^' is special and needs quoting */+strbuf_addch(sb,'\\');+strbuf_addch(sb,*src++);+}+if(*src=='*')+/* beginning '*' is not special, no quoting */+strbuf_addch(sb,*src++);++while((c=*src++)){+switch(c){+case'[':+case'.':+case'\\':+case'*':+strbuf_addch(sb,'\\');+strbuf_addch(sb,c);+break;++case'$':+/* only the end '$' is special and needs quoting */+if(*src=='\0')+strbuf_addch(sb,'\\');+strbuf_addch(sb,c);+break;++default:+strbuf_addch(sb,c);+break;+}+}+}
This function returns true if git is running under an UTF-8
locale. pcre in the next patch will need this.
is_encoding_utf8() is used instead of strcmp() to catch both "utf-8"
and "utf8" suffixes.
When built with no gettext support, we peek in several env variables
to detect UTF-8. pcre library might support utf-8 even if libc is
built without locale support.. The peeking code is a copy from
compat/regex/regcomp.c
Helped-by: Ævar Arnfjörð Bjarmason [off-list ref]
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
gettext.c | 24 ++++++++++++++++++++++--
gettext.h | 1 +
2 files changed, 23 insertions(+), 2 deletions(-)
Similar to the "grep -F -i" case, we can't use kws on icase search
outside ascii range, so we quote the string and pass it to regcomp as
a basic regexp and let regex engine deal with case sensitivity.
The new test is put in t7812 instead of t4209-log-pickaxe because
lib-gettext.sh might cause problems elsewhere, probably.
Noticed-by: Plamen Totev [off-list ref]
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
diffcore-pickaxe.c | 11 +++++++++++
t/t7812-grep-icase-non-ascii.sh | 7 +++++++
2 files changed, 18 insertions(+)
This is in preparation to turn test-regex into some generic regex
testing command.
Helped-by: Eric Sunshine [off-list ref]
Helped-by: Ramsay Jones [off-list ref]
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
Signed-off-by: Junio C Hamano <redacted>
---
t/t0070-fundamental.sh | 2 +-
test-regex.c | 12 ++++++++++--
2 files changed, 11 insertions(+), 3 deletions(-)
@@ -31,7 +31,7 @@ test_expect_success 'git_mkstemps_mode does not fail if fd 0 is not open' ' test_expect_success'check for a bug in the regex routines''# if this test fails, re-build git with NO_REGEX=1-test-regex+test-regex--bug' test_done
@@ -16,5 +16,13 @@ int main(int argc, char **argv)if(m[0].rm_so==3)/* matches '\n' when it should not */die("regex bug confirmed: re-build git with NO_REGEX=1");-exit(0);+return0;+}++intmain(intargc,char**argv)+{+if(argc==2&&!strcmp(argv[1],"--bug"))+returntest_regex_bug();+else+usage("test-regex --bug");}