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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jun 10, 2020, 17:18 UTC
Message-ID: <CAPig+cSLwPmDbk8Wjqb06OrNGHfmTLDOHjs88C+q1FhLGqLrrQ@mail.gmail.com>
In-Reply-To: <20200610171153.GA39055@konoha>

On Wed, Jun 10, 2020 at 1:12 PM Shourya Shukla <shouryashukla.oo@gmail.com> wrote:

Show 10 quoted lines

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

Show 6 quoted lines

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

Show 5 quoted lines

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

Show 10 quoted lines

> > -   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"?)