Re: [PATCH v3 2/5] strbuf: refactor strbuf_trim_trailing_ch()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Dec 7, 2024, 06:35 UTC
- Message-ID
- <xmqqzfl8t2yl.fsf@gitster.g>
- In-Reply-To
- <20241206124248.160494-3-christian.couder@gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
Show 10 quoted lines
> We often have to split strings at some specified terminator character. > The strbuf_split*() functions, that we can use for this purpose, > return substrings that include the terminator character, so we often > need to remove that character. > > When it is a whitespace, newline or directory separator, the > terminator character can easily be removed using an existing triming > function like strbuf_rtrim(), strbuf_trim_trailing_newline() or > strbuf_trim_trailing_dir_sep(). There is no function to remove that > character when it's not one of those characters though.
Heh, totally uninteresting (alternative being open coding this one). If we pass, instead of a single character 'c', an array of characters to be stripped from the right (like strspn() allows you to skip from the left), I may have been a bit more receptive, though ;-)
Show 6 quoted lines
> +void strbuf_trim_trailing_ch(struct strbuf *sb, int c)
> +{
> + while (sb->len > 0 && sb->buf[sb->len - 1] == c)
> + sb->len--;
> + sb->buf[sb->len] = '\0';
> +}So, trim_trailing will leave "foo" when "foo,,," is fed with c set to ','.
Show 13 quoted lines
> diff --git a/trace2/tr2_cfg.c b/trace2/tr2_cfg.c
> index 22a99a0682..9da1f8466c 100644
> --- a/trace2/tr2_cfg.c
> +++ b/trace2/tr2_cfg.c
> @@ -35,10 +35,7 @@ static int tr2_cfg_load_patterns(void)
>
> tr2_cfg_patterns = strbuf_split_buf(envvar, strlen(envvar), ',', -1);
> for (s = tr2_cfg_patterns; *s; s++) {
> - struct strbuf *buf = *s;
> -
> - if (buf->len && buf->buf[buf->len - 1] == ',')
> - strbuf_setlen(buf, buf->len - 1);
> + strbuf_trim_trailing_ch(*s, ',');And the only thing that prevents this rewrite from being buggy is the use of misdesigned strbuf_split_buf() function (which by now we should have deprecated!). Because it splits at ',', we won't have more than one ',' trailing, but we still split that one trailing comma because the misdesigned strbuf_split_buf() leaves the separator at the end of each element.
This does not look like a very convincing example to demonstrate why the new helper function is useful, at least to me.
If somebody would touch this area of code, I think a lot nicer clean-up would be to rewrite the thing into a helper function that is called from here, and the other one in the next hunk in a single patch, and then clean up the refactored helper function not to use the strbuf_split_buf(). Looking at the way tr2_cfg_patterns and tr2_cfg_env_vars are used, they have *NO* valid reason why they have to be a strbuf. Once populated, they are only used for a constant string pointed at by their .buf member. A string_list constructed by appending (i.e. not sorted) would be a lot more suitable data structure.
Show 15 quoted lines
> strbuf_trim_trailing_newline(*s);
> strbuf_trim(*s);
> }
> @@ -74,10 +71,7 @@ static int tr2_load_env_vars(void)
>
> tr2_cfg_env_vars = strbuf_split_buf(varlist, strlen(varlist), ',', -1);
> for (s = tr2_cfg_env_vars; *s; s++) {
> - struct strbuf *buf = *s;
> -
> - if (buf->len && buf->buf[buf->len - 1] == ',')
> - strbuf_setlen(buf, buf->len - 1);
> + strbuf_trim_trailing_ch(*s, ',');
> strbuf_trim_trailing_newline(*s);
> strbuf_trim(*s);
> }