Hi Junio,
On Tue, Aug 18, 2020 at 12:59 AM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> "Hariom Verma via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
> > -static void format_sanitized_subject(struct strbuf *sb, const char *msg)
> > +static void format_sanitized_subject(struct strbuf *sb, const char *msg, size_t len)
> > {
> > + char *r = xmemdupz(msg, len);
> > size_t trimlen;
> > size_t start_len = sb->len;
> > int space = 2;
> > + int i;
> >
> > - for (; *msg && *msg != '\n'; msg++) {
> > - if (istitlechar(*msg)) {
> > + for (i = 0; i < len; i++) {
> > + if (r[i] == '\n')
> > + r[i] = ' ';
>
> Copying the whole string only for this one looks very wasteful.
> Can't you do
>
> for (i = 0; i < len; i++) {
> char r = msg[i];
> if (isspace(r))
> r = ' ';
> if (istitlechar(r)) {
> ...
> }
>
> or something like that instead?
Ok, that sounds better. Noted for the next version.
> > + if (istitlechar(r[i])) {
> > if (space == 1)
> > strbuf_addch(sb, '-');
> > space = 0;
> > - strbuf_addch(sb, *msg);
> > - if (*msg == '.')
> > - while (*(msg+1) == '.')
> > - msg++;
> > + strbuf_addch(sb, r[i]);
> > + if (r[i] == '.')
> > + while (r[i+1] == '.')
> > + i++;
> > } else
> > space |= 1;
> > }
> > + free(r);
>
> Also, because neither LF or SP is a titlechar(), wouldn't the "if
> r[i] is LF, replace it with SP" a no-op wrt what will be in sb at
> the end?
Maybe its better to directly replace LF with hyphen? [Instead of first
replacing LF with SP and then replacing SP with '-'.]
> > case 'f': /* sanitized subject */
> > - format_sanitized_subject(sb, msg + c->subject_off);
> > + eol = strchrnul(msg + c->subject_off, '\n');
> > + format_sanitized_subject(sb, msg + c->subject_off, eol - (msg + c->subject_off));
>
> This original caller expected the helper to stop reading at the end
> of the first line, but the updated helper needs to be told where to
> stop, so we do so with some extra computation. Makes sense.
Yeah.
Thanks,
Hariom
