I wonder if we have developers for whom the non-ASCII here is an issue.
I do have an issue myself ;-) but I can survive. I do not know
about others.
This sort of code is much more pleseant to read and work with if you use
strbuf_split_buf(). This isn't performance sensitive, so a few more
allocations is fine.
Please do not encourage use of strbuf_split_buf(). It is a
misdesigned API as it rarely is justifyable to have an array, each
element of which can be independently tweaked by being strbuf. We
are not implementing a text editor after all ;-)
A helper function that takes a string and returns a strvec would be
a good fit, though.
I see this is partially a pre-existing thing in the file, but we have an
__APPLE__ already in cache.h. Perhaps define a iLAUNCHCTL_AVAILABLE
there. See e.g. 62e5ee81a39 (read-cache.c: remove #ifdef NO_PTHREADS,
2018-11-03).
From: Jeff King <hidden> Date: 2021-07-13 00:15:57
On Tue, Jul 06, 2021 at 01:52:12PM -0700, Junio C Hamano wrote:
quoted
This sort of code is much more pleseant to read and work with if you use
strbuf_split_buf(). This isn't performance sensitive, so a few more
allocations is fine.
Please do not encourage use of strbuf_split_buf(). It is a
misdesigned API as it rarely is justifyable to have an array, each
element of which can be independently tweaked by being strbuf. We
are not implementing a text editor after all ;-)
Very much agreed on avoiding strbuf_split_buf(). My usual go-to is
string_list_split(), which I think would work here for splitting on ":".
A helper function that takes a string and returns a strvec would be
a good fit, though.
I was going to second that, but I see we already have one. :) Dscho
introduced it in c5aa6db64f (argv_array: offer to split a string by
whitespace, 2018-04-25), and then it later became strvec_split().
And indeed, Lénaïc's patches use it elsewhere. I think it doesn't work
in this instance because it can't take an arbitrary delimiter. But I
wouldn't at all mind seeing it grow that feature (and I suspect it could
even share some code with string_list_split(), but didn't look).
-Peff
From: Eric Sunshine <hidden> Date: 2021-07-13 02:22:36
On Mon, Jul 12, 2021 at 8:16 PM Jeff King [off-list ref] wrote:
On Tue, Jul 06, 2021 at 01:52:12PM -0700, Junio C Hamano wrote:
quoted
A helper function that takes a string and returns a strvec would be
a good fit, though.
I was going to second that, but I see we already have one. :) Dscho
introduced it in c5aa6db64f (argv_array: offer to split a string by
whitespace, 2018-04-25), and then it later became strvec_split().
And indeed, Lénaïc's patches use it elsewhere. I think it doesn't work
in this instance because it can't take an arbitrary delimiter. But I
wouldn't at all mind seeing it grow that feature (and I suspect it could
even share some code with string_list_split(), but didn't look).
Since Lénaïc is a relative newcomer to the project, can we, as
reviewers, be clear that we don't expect him to perform the task of
generalizing strvec_split() just to get this series -- which is
already at v7 -- landed? I gave the previous round a pretty thorough
going-over and -- aside from one minor test-time bug -- didn't find
any show-stoppers which should prevent it from landing. While it may
be the case that the series has a superficial wart here and there
(such as #ifdef's in function bodies, and non-ASCII fancy comment
boxes), the review comments on the latest round have pretty much all
been subjective; I haven't seen any outright actionable observations.
Extra polishing based upon the subjective review comments can always
be done later atop Lénaïc's series (if someone -- not necessarily
Lénaïc -- wants to do so) without asking him for endless re-rolls.
From: Jeff King <hidden> Date: 2021-07-13 03:56:53
On Mon, Jul 12, 2021 at 10:22:22PM -0400, Eric Sunshine wrote:
On Mon, Jul 12, 2021 at 8:16 PM Jeff King [off-list ref] wrote:
quoted
On Tue, Jul 06, 2021 at 01:52:12PM -0700, Junio C Hamano wrote:
quoted
A helper function that takes a string and returns a strvec would be
a good fit, though.
I was going to second that, but I see we already have one. :) Dscho
introduced it in c5aa6db64f (argv_array: offer to split a string by
whitespace, 2018-04-25), and then it later became strvec_split().
And indeed, Lénaïc's patches use it elsewhere. I think it doesn't work
in this instance because it can't take an arbitrary delimiter. But I
wouldn't at all mind seeing it grow that feature (and I suspect it could
even share some code with string_list_split(), but didn't look).
Since Lénaïc is a relative newcomer to the project, can we, as
reviewers, be clear that we don't expect him to perform the task of
generalizing strvec_split() just to get this series -- which is
already at v7 -- landed?
Yeah, sorry if I was unclear on that. That is absolutely not a
requirement for this series.
(I do not have an opinion on Ævar's original question about using a
split function rather than open-coding. _If_ we were to do that,
string_list_split() would be the sensible existing mechanism. But it is
also not worth derailing the series for).
-Peff
From: Eric Sunshine <hidden> Date: 2021-07-13 05:17:59
On Mon, Jul 12, 2021 at 11:56 PM Jeff King [off-list ref] wrote:
On Mon, Jul 12, 2021 at 10:22:22PM -0400, Eric Sunshine wrote:
quoted
On Mon, Jul 12, 2021 at 8:16 PM Jeff King [off-list ref] wrote:
quoted
And indeed, Lénaïc's patches use it elsewhere. I think it doesn't work
in this instance because it can't take an arbitrary delimiter. But I
wouldn't at all mind seeing it grow that feature (and I suspect it could
even share some code with string_list_split(), but didn't look).
Since Lénaïc is a relative newcomer to the project, can we, as
reviewers, be clear that we don't expect him to perform the task of
generalizing strvec_split() just to get this series -- which is
already at v7 -- landed?
Yeah, sorry if I was unclear on that. That is absolutely not a
requirement for this series.
Thanks, and sorry if my response seemed to be aimed only at your
reply. It wasn't. It was a reaction to a number of recent reviews --
in general -- containing only subjective comments or a mix of
subjective comments and genuine actionable items without necessarily
making it clear which is which. Such ambiguity is hard on newcomers,
who may end up doing unnecessary work trying to get their patches
accepted, or might scare them away altogether.
Since Lénaïc is a relative newcomer to the project, can we, as
reviewers, be clear that we don't expect him to perform the task of
generalizing strvec_split() just to get this series -- which is
already at v7 -- landed? I gave the previous round a pretty thorough
going-over and -- aside from one minor test-time bug -- didn't find
any show-stoppers which should prevent it from landing. While it may
be the case that the series has a superficial wart here and there
(such as #ifdef's in function bodies, and non-ASCII fancy comment
boxes), the review comments on the latest round have pretty much all
been subjective; I haven't seen any outright actionable observations.
Extra polishing based upon the subjective review comments can always
be done later atop Lénaïc's series (if someone -- not necessarily
Lénaïc -- wants to do so) without asking him for endless re-rolls.
In such situation when there is endless re-roll of patch series due to
subjective reviews, we can ask for final call from the maintainer, right?
--
An old man doll... just what I always wanted! - Clara