Re: [PATCH 3/3] bisect: fix misuse of `refs_for_each_ref_in()`

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> On Wed, Jan 28, 2026 at 09:49:22AM +0100, Patrick Steinhardt wrote:
>
>> Furthermore, taking a closer look, we strip the prefix only to re-add it
>> in `mark_for_removal()`. This is somewhat roundabout, as we can instead
>> call `refs_for_each_fullref_in()` to not do any stripping at all. Do so
>> to simplify the code a bit.
>
> Yeah, I think the result is much better.
>
> We might also want this simplification on top:

Very good.  Thanks, both, for improvements.

> -- >8 --
> Subject: [PATCH] bisect: simplify string_list memory handling
>
> We declare the refs_for_removal string_list as NODUP, forcing us to
> manually allocate strings we insert. And then when it comes time to
> clean up, we set strdup_strings so that string_list_clear() will free
> them for us.
>
> This is a confusing pattern, and can be done much more simply by just
> declaring the list with the DUP initializer in the first place.
>
> It was written this way originally because one of the callsites
> generated the item using xstrfmt(). But that spot switched to a plain
> xstrdup() in cf01f617b9 (bisect: fix misuse of `refs_for_each_ref_in()`,
> 2026-01-28). That means we can now just let the string_list code handle
> allocation itself.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Even before cf01f617b9 we could have done:
>
>   string_list_append_nodup(&refs, xstrfmt(...));
>
> to get a similar simplification, but after that commit it is even
> easier.
>
>  bisect.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/bisect.c b/bisect.c
> index 2cd97bc9fe..6b9e9d81c3 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -1183,8 +1183,7 @@ int estimate_bisect_steps(int all)
>  static int mark_for_removal(const struct reference *ref, void *cb_data)
>  {
>  	struct string_list *refs = cb_data;
> -	char *bisect_ref = xstrdup(ref->name);
> -	string_list_append(refs, bisect_ref);
> +	string_list_append(refs, ref->name);
>  	return 0;
>  }
>  
> @@ -1193,16 +1192,15 @@ int bisect_clean_state(void)
>  	int result = 0;
>  
>  	/* There may be some refs packed during bisection */
> -	struct string_list refs_for_removal = STRING_LIST_INIT_NODUP;
> +	struct string_list refs_for_removal = STRING_LIST_INIT_DUP;
>  	refs_for_each_fullref_in(get_main_ref_store(the_repository),
>  				 "refs/bisect/", NULL, mark_for_removal,
>  				 &refs_for_removal);
> -	string_list_append(&refs_for_removal, xstrdup("BISECT_HEAD"));
> -	string_list_append(&refs_for_removal, xstrdup("BISECT_EXPECTED_REV"));
> +	string_list_append(&refs_for_removal, "BISECT_HEAD");
> +	string_list_append(&refs_for_removal, "BISECT_EXPECTED_REV");
>  	result = refs_delete_refs(get_main_ref_store(the_repository),
>  				  "bisect: remove", &refs_for_removal,
>  				  REF_NO_DEREF);
> -	refs_for_removal.strdup_strings = 1;
>  	string_list_clear(&refs_for_removal, 0);
>  	unlink_or_warn(git_path_bisect_ancestors_ok());
>  	unlink_or_warn(git_path_bisect_log());