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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 19, 2020, 16:08 UTC
Message-ID: <xmqqeeo2vcsl.fsf@gitster.c.googlers.com>
In-Reply-To: <xmqqimdevd4l.fsf@gitster.c.googlers.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 16 quoted lines

> Hariom verma <hariom18599@gmail.com> writes:
>
>>> 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.

I was undecided between mentioning and not mentioning the variable name "space" here. On one hand, one _could_ argue that "space" is used to remember we saw "space and the like" and if it were named "seen_non_title_char", then such a confusion to treat LF so specially might not have occurred. But on the other hand, "space" is what the variable exactly keeps track of; it is just the need for space on the output side, i.e. we remember that "space needed before the next output" with that variable.

I am inclined not to suggest renaming "space" at all, but it won't be the end of the world if it were renamed to "need_space" (before the next output), or "seen_nontitle". If we were to actually rename, I have moderately strong preference to the "need_space" over "seen_nontitle", as it won't have to be renamed again when the logic to require a space before the next output has to be updated to include cases other than just "we saw a nontitle character".

Thanks.