Re: [PATCH v3 7/9] pretty: refactor `format_sanitized_subject()`

[Hariom verma <hariom18599@gmail.com>]

Hi Junio,

On Tue, Aug 18, 2020 at 12:59 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Hariom Verma via GitGitGadget" <gitgitgadget@gmail.com> 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