Re: [PATCH v5 7/7] strbuf: remove global variable

From: Junio C Hamano <gitster@pobox.com>
Date: May 11, 2023, 21:42 UTC
Message-ID: <xmqqr0rmtuid.fsf@gitster.g>
In-Reply-To: <20230511194822.1493798-7-calvinwan@google.com>

Calvin Wan <calvinwan@google.com> 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.