On Thu, Jul 31, 2025 at 03:54:24PM -0700, Junio C Hamano wrote:
> This is a catastrophe waiting to happen. If the callee causes the
> strbuf to be reallocated, the buf[] the caller has will become
> dangling, and when the caller does strbuf_release(), it would result
> in double-free.
>
> Stop calling the function with misleading call-by-value with strbuf.
This is definitely an improvement, though I wonder if we could go
further.
When I saw the original version of this patch, my first thought was: if
we are passing a strbuf that is not modified, then why not just pass a
const string pointer? It is more flexible for the caller and makes our
intention more clear. (A careful reader will note that this does not
pass in the length, so it is different if we expect embedded NULs, but I
don't think that is the case here).
But it does not quite work in this case because of this:
> if (is_single) {
> - choice_list = strbuf_split_max(&input, '\n', 0);
> + choice_list = strbuf_split_max(input, '\n', 0);
> } else {
> - char *p = input.buf;
> + char *p = input->buf;
> do {
> if (*p == ',')
> *p = ' ';
> } while (*p++);
> - choice_list = strbuf_split_max(&input, ' ', 0);
> + choice_list = strbuf_split_max(input, ' ', 0);
> }
We do modify the string! However, that code goes away in your next patch
when we are able to switch to a split function that handles multiple
delimiters. You do still do an in-place split, but isn't strictly
needed.
If we swapped patches 2 and 3 in this series, then the strbuf cleanup
can just become:
diff --git a/builtin/clean.c b/builtin/clean.c
index a01d130f62..e4b075df70 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -477,17 +477,17 @@ static int find_unique(const char *choice, struct menu_stuff *menu_stuff)
*/
static int parse_choice(struct menu_stuff *menu_stuff,
int is_single,
- struct strbuf input,
+ const char *input,
int **chosen)
{
- struct string_list choice = STRING_LIST_INIT_NODUP;
+ struct string_list choice = STRING_LIST_INIT_DUP;
struct string_list_item *item;
int nr = 0;
int i;
- string_list_split_in_place_f(&choice, input.buf,
- is_single ? "\n" : ", ", -1,
- STRING_LIST_SPLIT_TRIM);
+ string_list_split_f(&choice, input,
+ is_single ? "\n" : ", ", -1,
+ STRING_LIST_SPLIT_TRIM);
for_each_string_list_item(item, &choice) {
const char *string;
@@ -626,7 +626,7 @@ static int *list_and_choose(struct menu_opts *opts, struct menu_stuff *stuff)
nr = parse_choice(stuff,
opts->flags & MENU_OPTS_SINGLETON,
- choice,
+ choice.buf,
&chosen);
if (opts->flags & MENU_OPTS_SINGLETON) {
I dunno. Maybe it is nitpicking, but I think "don't take a strbuf if you
only need a string" is a good general rule. Of course there is only one
caller here, so flexibility is probably not that important.
-Peff
