Calvin Wan <calvinwan@xxxxxxxxxx> writes:
> strbuf_stripspace() removes the skip_comments boolean and checks if
> comment_line_char is a non-NULL character to determine whether to skip
> comments or not.
Hmph, that is certainly cute. Taking it as granted that NUL can
never serve any useful purpose in a text file (where stripspace
makes any sense), passing NUL as the comment_line_char can certainly
be used as a sign that no stripping is desired.
And those existing callers that do not want comment stripping are
already passing 0 at the position because the original parameter is
an integer (used as a Boolean); they now ought to be passing '\0'
instead, but passing 0 is acceptable.
It is not trivial to catch existing callers that pass 1 to mean
"please strip" as they are not supposed to pass comment_line_char
(usually "#", but there is a global variable for that visible to
them). If left unconverted, they end up asking that lines that
begin with SOH ('\001') to be considered comments and get stripped.
In order to catch such a mistake, however, you cannot say "ah, the
skip_comments parameter is '1'; is that because the end user really
wanted SOH as the comment leader? Let's warn if the value of the
comment_line_char global is not 1" for obvious reasons X-<.
So this step, while it makes sense in a vacuum, is a cute idea, and
is nicer in the longer term (because we certainly do not want to
have to pass an extra parameter to the function), raises the risk of
semantic mismerge higher for topics in flight that do want to use
stripspace to remove lines that are commented out. My quick "git
log -S" seems to tell me there is no such topic I happened to have
picked up in 'seen' right now, though, so it may be OK.
Thanks.
