Re: pgindent (was Re: [COMMITTERS] pgsql: Preventive maintenance in advance of pgindent run.)

On 2017-05-21 03:00, Tom Lane wrote:
> I wrote:
>> Also, I found two places where an overlength comment line is simply busted
>> altogether --- notice that a character is missing at the split point:
>
> I found the cause of that: you need to apply this patch:
>
> --- freebsd_indent/pr_comment.c~ 2017-05-17 14:59:31.548442801 -0400
> +++ freebsd_indent/pr_comment.c 2017-05-20 20:51:16.447332977 -0400
> @@ -344,8 +353,8 @@ pr_comment(void)
> {
> int len = strlen(t_ptr);
>
> - CHECK_SIZE_COM(len);
> - memmove(e_com, t_ptr, len);
> + CHECK_SIZE_COM(len + 1);
> + memmove(e_com, t_ptr, len + 1);
> last_bl = strpbrk(e_com, " \t");
> e_com += len;
> }
>
> As the code stands, the strpbrk call is being applied to a
> not-null-terminated string and therefore is sometimes producing an
> insane value of last_bl, messing up decisions later in the comment.
> Having the memmove include the trailing \0 resolves that.

I have been analyzing this and came to different conclusions. Foremost,
a strpbrk() call like that finds the first occurrence of either space or
a tab, but last_bl means "last blank" - it's used for marking where to
wrap a comment line if it turns out to be too long. The previous coding
moved the character sequence byte after byte, updating last_bl every
time it was copying one of the two characters. I've rewritten that part as:
CHECK_SIZE_COM(len);
memmove(e_com, t_ptr, len);
- last_bl = strpbrk(e_com, " \t");
e_com += len;
+ last_bl = NULL;
+ for (t_ptr = e_com - 1; t_ptr > e_com - len; t_ptr--)
+ if (*t_ptr == ' ' || *t_ptr == '\t') {
+ last_bl = t_ptr;
+ break;
+ }
}

But then I also started to wonder if there is any case when there's more
than one character to copy and I haven't found one yet. It looks like
} while (!memchr("*\n\r\b\t", *buf_ptr, 6) &&
(now_col <= adj_max_col || !last_bl));
guarantees that if we're past adj_max_col, it'll only be one non-space
character. But I'm not sure yet.