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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: May 10, 2023, 08:12 UTC
Message-ID: <8c8afa4a-ae2d-5a13-f9fa-5a5e11a0c810@gmail.com>
In-Reply-To: <20230508165908.526247-7-calvinwan@google.com>

Hi Calvin

On 08/05/2023 17:59, Calvin Wan wrote:

> As a library that only interacts with other primitives, strbuf should
> not utilize the comment_line_char global variable within its
> functions. Therefore, add an additional parameter for functions that use
> comment_line_char and refactor callers to pass it in instead.

I find the revised subject and commit message much easier to understand.

Show 18 quoted lines

> diff --git a/strbuf.c b/strbuf.c
> index d5978fee4e..eba65ca421 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -1,6 +1,5 @@
>   #include "git-compat-util.h"
>   #include "alloc.h"
> -#include "environment.h"
>   #include "gettext.h"
>   #include "hex.h"
>   #include "strbuf.h"
> @@ -362,7 +361,8 @@ static void add_lines(struct strbuf *out,
>   	strbuf_complete_line(out);
>   }
>   
> -void strbuf_add_commented_lines(struct strbuf *out, const char *buf, size_t size)
> +void strbuf_add_commented_lines(struct strbuf *out, const char *buf,
> +				size_t size, char comment_line_char)

I don't really object to this change as I can understand why you are making it, but it does make this function more cumbersome to use within git itself where we now have to pass the global comment_line_char explicitly.

Show 7 quoted lines

> @@ -1054,7 +1055,8 @@ static size_t cleanup(char *line, size_t len)
>    * Enable skip_comments to skip every line starting with comment
>    * character.
>    */
> -void strbuf_stripspace(struct strbuf *sb, int skip_comments)
> +void strbuf_stripspace(struct strbuf *sb, int skip_comments,
> +		       char comment_line_char)

Rather than adding a new parameter here could we change the signature to

	void strbuf_stripspace(struct strbuf *sb, char comment_char)

and not strip comments if comment_char == '\0'? There doesn't seem much point in forcing callers to pass comment_line_char when they don't want to strip comments.

Best Wishes

Phillip