On Thu, Jul 31, 2025 at 08:47:10PM -0700, Junio C Hamano wrote:
> shejialuo <shejialuo@xxxxxxxxx> writes:
>
> > On Thu, Jul 31, 2025 at 03:46:03PM -0700, Junio C Hamano wrote:
> >> static int split_string(struct string_list *list, const char *string, const char *delim,
> >> - int maxsplit, int in_place)
> >> + int maxsplit, int in_place, unsigned flags)
> >> {
> >> int count = 0;
> >> const char *p = string;
> >> @@ -320,12 +327,18 @@ static int split_string(struct string_list *list, const char *string, const char
> >> for (;;) {
> >> char *end;
> >>
> >> + if (flags & STRING_LIST_SPLIT_TRIM) {
> >> + /* ltrim */
> >> + while (*p && isspace(*p))
> >> + p++;
> >> + }
> >> +
> >> if (0 <= maxsplit && maxsplit <= count)
> >> end = NULL;
> >> else
> >> end = strpbrk(p, delim);
> >>
> >
> > In `append_one`, we would tell whether `end` is NULL. I somehow feel
> > strange why we need to do that in `append_one`. Should we just set `end`
> > to be `p + strlen(p)` when `end` is NULL. And then we could do rtrim
> > inside this function instead of `append_one` to avoid passing "flags" to
> > `append_one`.
>
> Sorry, but I do not see why such an alternative design is a better
> idea. The helper function's purpose is to stuff the substring at
> [p..end), possibly after rtrimming, to the list. You could compute
> rtrim in the caller, but that would make the logic here more complex
> (at least, you'd need to introduce yet another variable similar to
> "end" that points at the real tail of the string, and you cannot
> reuse "end" for it, because of the exit condition you see below).
>
I agree with you that we would introduce another variable. However, the
thing I quite dislike is that we do ltrim inside the current function
and we do rtrim inside `append_one`.
My thinking is that we should handle the [p, end) string in the same
place. We could either decide to drop the string or change the string in
the same place. However, at now, the logic happens at two different
places, which is my concern.
