Florian Schmidt <flosch@xxxxxxxxxxx> writes:
> Currently, if
> (a) There is a "---" divider in a commit message,
> (b) At some point beyond that divider, there is a cut-line (that is,
> "# ------------------------ >8 ------------------------") in the
> commit message,
> (c) the user does not explicitly set the "no-divider" option,
> then "git interpret-trailers" will hang indefinitively.
You do not have to say "Currently, if"; just "If" is sufficient.
Cf. Documentation/SubmittingPatches[[present-tense]]
> This is because when (a) is true, find_end_of_log_message() will invoke
> ignored_log_message_bytes() with a len that is intended to make it
> ignore the part of the commit message beyond the divider. However,
> ignored_log_message_bytes() calls wt_status_locate_end(), and that
> function ignores the length restriction when it tries to locate the cut
> line. If it manages to find one, the returned cutoff value is greater
> than len. At this point, ignored_log_message_bytes() goes into an
> infinite loop, because it won't advance the string parsing beyond len,
> but the exit condition expects to reach cutoff.
Good finding.
> It seems sensible to expect that wt_status_locate_end() should honour
> the length parameter passed in, and doing so fixes this issue.
Thanks. This is an ancient bug, not a retression from recent
changes to the trailer library [linusa CC'ed to save him from
wasting his time wondering if he broke anything].
> diff --git a/wt-status.c b/wt-status.c
> index b5a29083df..51a84575ed 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1089,14 +1089,19 @@ size_t wt_status_locate_end(const char *s, size_t len)
> {
> const char *p;
> struct strbuf pattern = STRBUF_INIT;
> + size_t result = 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;
> + result = 0;
> + else if ((p = strstr(s, pattern.buf))) {
> + result = p - s + 1;
> + if (result > len) {
> + result = len;
> + }
> + }
> strbuf_release(&pattern);
> - return len;
> + return result;
> }
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;
}
