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