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

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

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.

> @@ -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.

> @@ -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.