Re: [PATCH v3 3/3] Use the newly-introduced regexec_buf() function

[Junio C Hamano]

Johannes Schindelin  writes:

>> Also I agree with Peff that a test with an embedded NUL would be a
>> good thing.
>
> This is something I will leave to somebody else, as it was not my
> intention to fix this and I *really* have more pressing things to do right
> now... Sorry!

As I said a few minutes ago, I think we can stop _before_ worrying
about an embedded NUL, which is something we haven't handled before
anyway so it is a new feature that can be built later outside the
scope of this series.

Re: [PATCH v3 3/3] Use the newly-introduced regexec_buf() function

[Johannes Schindelin]

Hi Peff,

On Fri, 9 Sep 2016, Jeff King wrote:

> On Fri, Sep 09, 2016 at 11:52:50AM +0200, Johannes Schindelin wrote:
> 
> > > Also I agree with Peff that a test with an embedded NUL would be a
> > > good thing.
> > 
> > This is something I will leave to somebody else, as it was not my
> > intention to fix this and I *really* have more pressing things to do right
> > now... Sorry!
> 
> I think it is literally just squashing this into your final patch:
> 
> diff --git a/t/t4061-diff-pickaxe.sh b/t/t4061-diff-pickaxe.sh
> index f0bf50b..37b8dde 100755
> --- a/t/t4061-diff-pickaxe.sh
> +++ b/t/t4061-diff-pickaxe.sh
> @@ -19,4 +19,13 @@ test_expect_success '-G matches' '
>   test 4096-zeroes.txt = "$(cat out)"
>  '
>  
> +test_expect_success '-G matches after embedded NUL' '
> + printf "one\0two" >file &&
> + git add file &&
> + git commit -m embedded &&
> + echo embedded >expect &&
> + git log -Gtwo --format=%s >actual &&
> + test_cmp expect actual
> +'
> +
>  test_done

Thank you for providing me with the patch.

However, the whole idea of supporting regular expressions on buffers with
embedded NULs *is* different from the purpose of this patch series.

And in my quick web search, I got the impression that the presence of
REG_STARTEND really does not guarantee that regexec() won't stop at the
first NUL when rm_eo points after it.

So yeah, the patch would be easy to squash in, but the entire "rat's tail"
of making sure that this works everywhere, *in addition* to making sure
that the crash on mmap()ed buffers no longer occurs, would just delay this
patch series.

And unfortunately I do not have time for that right now.

Ciao,
Dscho

Re: [PATCH v3 3/3] Use the newly-introduced regexec_buf() function

[Jeff King]

On Fri, Sep 09, 2016 at 11:52:50AM +0200, Johannes Schindelin wrote:

> > Also I agree with Peff that a test with an embedded NUL would be a
> > good thing.
> 
> This is something I will leave to somebody else, as it was not my
> intention to fix this and I *really* have more pressing things to do right
> now... Sorry!

I think it is literally just squashing this into your final patch:

diff --git a/t/t4061-diff-pickaxe.sh b/t/t4061-diff-pickaxe.sh
index f0bf50b..37b8dde 100755
--- a/t/t4061-diff-pickaxe.sh
+++ b/t/t4061-diff-pickaxe.sh
@@ -19,4 +19,13 @@ test_expect_success '-G matches' '
test 4096-zeroes.txt = "$(cat out)"
 '
 
+test_expect_success '-G matches after embedded NUL' '
+   printf "one\0two" >file &&
+   git add file &&
+   git commit -m embedded &&
+   echo embedded >expect &&
+   git log -Gtwo --format=%s >actual &&
+   test_cmp expect actual
+'
+
 test_done

-Peff

Re: [PATCH v3 3/3] Use the newly-introduced regexec_buf() function

[Johannes Schindelin]

Hi Junio,

On Thu, 8 Sep 2016, Junio C Hamano wrote:

> Please give these three patches a common prefix, e.g.
> 
>   regex: -G feeds a non NUL-terminated string to regexec() and 
> fails
> regex: add regexec_buf() that can work on a non NUL-terminated string
>   regex: use regexec_buf()
> 
> or something like that.

Done.

> Also I agree with Peff that a test with an embedded NUL would be a
> good thing.

This is something I will leave to somebody else, as it was not my
intention to fix this and I *really* have more pressing things to do right
now... Sorry!

Ciao,
Dscho

Re: [PATCH v3 3/3] Use the newly-introduced regexec_buf() function

[Junio C Hamano]

Johannes Schindelin  writes:

> @@ -33,11 +32,8 @@ static void diffgrep_consume(void *priv, char *line, 
> unsigned long len)
>* caller early.
>*/
>   return;
> - /* Yuck -- line ought to be "const char *"! */
> - hold = line[len];
> - line[len] = '\0';
> - data->hit = !regexec(data->regexp, line + 1, 1, , 0);
> - line[len] = hold;
> + data->hit = !regexec_buf(data->regexp, line + 1, len - 1, 1,
> +  , 0);

This is an unexpected happy surprise.  It really feels good to see
that "Yuck" line go.

> @@ -228,18 +227,16 @@ static long ff_regexp(const char *line, long len,
>   len--;
>   }
>  
> - line_buffer = xstrndup(line, len); /* make NUL terminated */
> -
>   for (i = 0; i < regs->nr; i++) {
>   struct ff_reg *reg = regs->array + i;
> - if (!regexec(>re, line_buffer, 2, pmatch, 0)) {
> + if (!regexec_buf(>re, line, len, 2, pmatch, 0)) {

So is this hunk.  Removing unnecessary copying is a very good thing.

Please give these three patches a common prefix, e.g.

regex: -G feeds a non NUL-terminated string to regexec() and 
fails
regex: add regexec_buf() that can work on a non NUL-terminated string
regex: use regexec_buf()

or something like that.

Also I agree with Peff that a test with an embedded NUL would be a
good thing.

This round is so close to perfect.