Re: [PATCH] wt-status: Don't find scissors line beyond buf len

From: Florian Schmidt <flosch@nutanix.com>
Date: Mar 8, 2024, 09:08 UTC
Message-ID: <1ff36e64-b993-4cbb-ba0a-01aca5396ef6@nutanix.com>
In-Reply-To: <xmqq34t1n91w.fsf@gitster.g>

That's a quick review cycle, thanks!

On 07/03/2024 19:20, Junio C Hamano wrote:

> You do not have to say "Currently, if"; just "If" is sufficient.
> Cf. Documentation/SubmittingPatches[[present-tense]]

ack

Show 24 quoted lines

> Looks correct, but we probably can make the fix a lot more isolated
> into a single block, like the attached patch.  How does this look?
> 
>   wt-status.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git c/wt-status.c w/wt-status.c
> index b5a29083df..511f37cfe0 100644
> --- c/wt-status.c
> +++ w/wt-status.c
> @@ -1093,8 +1093,11 @@ size_t wt_status_locate_end(const char *s, size_t len)
>   	strbuf_addf(&pattern, "\n%c %s", comment_line_char, cut_line);
>   	if (starts_with(s, pattern.buf + 1))
>   		len = 0;
> -	else if ((p = strstr(s, pattern.buf)))
> -		len = p - s + 1;
> +	else if ((p = strstr(s, pattern.buf))) {
> +		int newlen = p - s + 1;
> +		if (newlen < len)
> +			len = newlen;
> +	}
>   	strbuf_release(&pattern);
>   	return len;
>   }

That looks good to me, thanks! For context, I had sent in the slightly larger patch because at first look, I got confused by the fact that wt_status_locate_end takes len as a parameter, but then seemingly doesn't read it at all and only writes to it. (Of course, if neither the if nor the if-else branch are hit, len is in fact read when it is returned unchanged.) So I made the patch a bit larger in the hope that the next cursory reader might not get confused. But this option is a much more minimal patch, and functionally the same. So it comes down to style, and you have a much better feeling for that in this code base, so I'm happy to go with this.

Do you want me to send this version as a v2 to you + the list as per the documentation?

Cheers, flosch