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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 19, 2020, 16:01 UTC
Message-ID: <xmqqimdevd4l.fsf@gitster.c.googlers.com>
In-Reply-To: <CA+CkUQ9tkwXmrHq_ZV+RCgwoFHZ0M4dEhBkjUd97Xi+3shB-WQ@mail.gmail.com>

Hariom verma <hariom18599@gmail.com> writes:

Show 6 quoted lines

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

Why do you think LF is so special?

Everything other than titlechar() including HT, '#', '*', SP is treated in the same way as the body of that loop. It does not directly contribute to the final contents of sb, but just leaves the marker in the variable "space" the fact that when adding the next titlechar() to the resulting sb, we need a SP to wordbreak.

LF now happens to be in the set due to the way you extended the function (it wasn't fed to this function by its sole caller), but other than that, it is no more special than HT, SP or '*'. And they are not replaced with SP or replaced with '-'.

So it would be the most sensible to just drop 'if LF, replace it with SP before doing anything else' you added. The existing 'if titlechar, add it to sb but if we saw non-title, add a SP before doing so to wordbreak, and if not titlechar, just remember the fact that we saw one' should work fine as-is without special casing LF at all.

Or am I missing something subtle?