Re: [PATCH v2 6/7] worktree: generalize candidate worktree path validation

[Eric Sunshine <sunshine@sunshineco.com>]

On Wed, Jun 10, 2020 at 1:12 PM Shourya Shukla
<shouryashukla.oo@gmail.com> wrote:
> On 10/06 02:30, Eric Sunshine wrote:
> > "git worktree add" checks that the specified path is a valid location
> > for a new worktree by ensuring that the path does not already exist and
> > is not already registered to another worktree (a path can be registered
> > but missing, for instance, if it resides on removable media). Since "git
> > worktree add" is not the only command which should perform such
> > validation ("git worktree move" ought to also), generalize the the
> > validation function for use by other callers, as well.
>
> There is an extra 'the' after generalize.

Thanks for noticing. I'll fix it if I re-roll, otherwise it can stay
(unless Junio happens to fix it when queuing).

> >    if (!wt)
> > -       goto done;
> > +       return;
>
> Should we do a 'return 1' on failure instead of just a blank 'return' so
> that we can denote failure of finding a worktree?

This function is declared as returning 'void', so we can't "return 1".
The function instead signals a problem by die()'ing.

Changing it to return a success or failure result rather than dying is
a different matter which can be done later if someone wants to do so,
but is outside the scope of this patch series which is only making the
minimal necessary changes to adapt the function for wider use.

> > -       die(_("'%s' is a missing but locked worktree;\nuse 'add -f -f' to override, or 'unlock' and 'prune' or 'remove' to clear"), path);
> > +       die(_("'%s' is a missing but locked worktree;\nuse '%s -f -f' to override, or 'unlock' and 'prune' or 'remove' to clear"), cmd, path);
>
> Let's wrap this to 72 characters at maximum per line maybe? Meaning that
> the error message gets split into 2 lines.

I'm not sure what you want to see wrapped; the warning message itself
or the source code line? As for the warning message, it already is
wrapped (see the embedded "\n").

At any rate, this patch makes the minimal change necessary to meet the
goal of making the function re-usable. Anything beyond that (such as
wrapping long lines) is outside the scope of the patch and would make
it harder to reason about the changes. Wrapping the line is certainly
something that someone can do later as a follow-up, but is not the
goal of this series.

> > -   validate_worktree_add(path, opts);
> > +   worktrees = get_worktrees(0);
> > +   check_candidate_path(path, opts->force, worktrees, "add");
> > +   free_worktrees(worktrees);
> > +   worktrees = NULL;
>
> It is necessary to call 'free_worktrees(worktrees)' at the end? The
> 'get_worktrees()' states that
>   The caller is responsible for freeing the memory from the returned
>   worktree(s).

This code _does_ call free_worktrees() as it should, so I'm not sure
what you're asking or saying. (Perhaps you're confusing "caller" and
"callee"?)