Re: [PATCH v2 16/22] pickaxe -S: support content with NULs under --pickaxe-regex

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 30, 2021, 23:54 UTC
Message-ID: <xmqqim589ozn.fsf@gitster.g>
In-Reply-To: <20210216115801.4773-17-avarab@gmail.com>

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

Show 33 quoted lines

> Fix a bug in the matching routine powering -S<rx> --pickaxe-regex so
> that we won't abort early on content that has NULs in it.
>
> We've had a hard requirement on REG_STARTEND since 2f8952250a8 (regex:
> add regexec_buf() that can work on a non NUL-terminated string,
> 2016-09-21), but this sanity check dates back to d01d8c67828 (Support
> for pickaxe matching regular expressions, 2006-03-29).
>
> It wasn't needed anymore, and as the now-passing test shows, actively
> getting in our way.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  diffcore-pickaxe.c     | 4 ++--
>  t/t4209-log-pickaxe.sh | 8 ++++++++
>  2 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
> index dd1b5c72332..0bf50a2f595 100644
> --- a/diffcore-pickaxe.c
> +++ b/diffcore-pickaxe.c
> @@ -78,12 +78,12 @@ static unsigned int contains(mmfile_t *mf, regex_t *regexp, kwset_t kws)
>  		regmatch_t regmatch;
>  		int flags = 0;
>  
> -		while (sz && *data &&
> +		while (sz &&
>  		       !regexec_buf(regexp, data, sz, 1, &regmatch, flags)) {
>  			flags |= REG_NOTBOL;
>  			data += regmatch.rm_eo;
>  			sz -= regmatch.rm_eo;
> -			if (sz && *data && regmatch.rm_so == regmatch.rm_eo) {
> +			if (sz && regmatch.rm_so == regmatch.rm_eo) {

OK. As we always require start-end support, we do not need to stop at NULs, and we shouldn't if we are dealing with a haystack with NUL in it. The needle may be behind that NUL.

Makes sense.

Show 16 quoted lines

>  				data++;
>  				sz--;
>  			}
> diff --git a/t/t4209-log-pickaxe.sh b/t/t4209-log-pickaxe.sh
> index 4b65b89e7a5..6ea1f02d142 100755
> --- a/t/t4209-log-pickaxe.sh
> +++ b/t/t4209-log-pickaxe.sh
> @@ -201,4 +201,12 @@ test_expect_success 'log -S looks into binary files' '
>  	test_cmp log full-log
>  '
>  
> +test_expect_success 'log -S --pickaxe-regex looks into binary files' '
> +	git -C GS-bin-txt log --pickaxe-regex -Sa >log &&
> +	test_cmp log full-log &&
> +
> +	git -C GS-bin-txt log --pickaxe-regex -S[a] >log &&

Please quote this so that readers do not have to look around to see if there _could_ have been a file with such a name that matches the [glob] to cause the needle not passed literally to us.

> +	test_cmp log full-log
> +'
> +
>  test_done