Re: [PATCH 4/4] worktree add: let worktree_basename() return string copy

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 25, 2026, 20:04 UTC
Message-ID: <xmqqld9uklud.fsf@gitster.g>
In-Reply-To: <20260825180350.2099-5-l.s.r@web.de>

René Scharfe <l.s.r@web.de> writes:

> worktree_basename() requires callers to do pointer arithmetic to get the
> actual basename.  Simplify them by doing the calculations in the
> function and returning a copy of the basename directly.

OK.

> Remind programmers to free the result by renaming the function to
> worktree_basename_dup().  Two already do; convert the remaining one from

This is a bit surprising, depending on what "do" refers to, as I read it to mean "Two callers already free what is returned by the worktree_basename() function", which cannot be the case (or they would be segfaulting already). So I must have misunderstood this sentence. I count three callers of the function, so two do something while the other one that needs conversion does something else.

> resetting a shared strbuf to freeing the allocated string, which
> requires the same number of lines, but no arithmetic.  The added
> allocation is negligible because it's small and there's only one per run
> of "git worktree add".

This talks about the caller in builtin/worktree.c:add_worktree(), and it is indeed far easier to read with this patch applied, as there is no need to copy out only the basename part, and we no longer need to worry about chomping trailing directory separators.

Show 10 quoted lines

> @@ -766,10 +765,8 @@ static int dwim_orphan(const struct add_opts *opts, int opt_track, int remote)
>  
>  static char *dwim_branch(const char *path, char **new_branch)
>  {
> -	int n;
>  	int branch_exists;
> -	const char *s = worktree_basename(path, &n);
> -	char *branchname = xmemdupz(s, path + n - s);
> +	char *branchname = worktree_basename_dup(path);
>  	struct strbuf ref = STRBUF_INIT;

Ah, OK, so this is what you mean by "two already do". Not "two already free the result", but "two already make a copy before doing anything else anyway, so why not make worktree_basename_dup() give them their own copies?". Makes sense.

Show 8 quoted lines

> @@ -876,9 +873,7 @@ static int add(int ac, const char **av, const char *prefix,
>  	}
>  
>  	if (opts.orphan && !new_branch) {
> -		int n;
> -		const char *s = worktree_basename(path, &n);
> -		new_branch = new_branch_to_free = xmemdupz(s, path + n - s);
> +		new_branch = new_branch_to_free = worktree_basename_dup(path);

Likewise.

So going back to the confusing part of the log message,

    Remind ... to worktree_basename_dup().  Among the three callers
    of worktree_basename(), two immediately make copies of the
    returned string before using and freeing it, which makes for an
    easy conversion.  Convert the other one from resetting ...

or something like that, perhaps?

Thanks.