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

[Hariom verma <hariom18599@gmail.com>]

Hi,

On Wed, Aug 19, 2020 at 9:38 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > 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".

Yeah, if it was named "seen_non_title_char", I might not get confused.
But now as you have already explained its working pretty well, "space"
makes more sense to me.
Well, I'm okay with both "space" and "need_space".

I wonder what others have to say on this? "space" or "need_space"?

Thanks,
Hariom