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

[Junio C Hamano <gitster@pobox.com>]

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

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