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

"Hariom Verma via GitGitGadget" [off-list ref] 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?  

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

 	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.

Hi Junio,

On Tue, Aug 18, 2020 at 12:59 AM Junio C Hamano [off-list ref] wrote:

"Hariom Verma via GitGitGadget" [off-list ref] writes:

quoted

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

quoted

+             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 '-'.]

quoted

      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