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

From: Shourya Shukla <shouryashukla.oo@gmail.com>
Date: Jun 10, 2020, 17:11 UTC
Message-ID: <20200610171153.GA39055@konoha>
In-Reply-To: <20200610063049.74666-7-sunshine@sunshineco.com>

On 10/06 02:30, Eric Sunshine wrote:

Show 7 quoted lines

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

Show 19 quoted lines

> -static void validate_worktree_add(const char *path, const struct add_opts *opts)
> +/* check that path is viable location for worktree */
> +static void check_candidate_path(const char *path,
> +				 int force,
> +				 struct worktree **worktrees,
> +				 const char *cmd)
>  {
> -	struct worktree **worktrees;
>  	struct worktree *wt;
>  	int locked;
>  
>  	if (file_exists(path) && !is_empty_dir(path))
>  		die(_("'%s' already exists"), path);
>  
> -	worktrees = get_worktrees(0);
>  	wt = find_worktree_by_path(worktrees, path);
>  	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?

Show 13 quoted lines

>  	locked = !!worktree_lock_reason(wt);
> -	if ((!locked && opts->force) || (locked && opts->force > 1)) {
> +	if ((!locked && force) || (locked && force > 1)) {
>  		if (delete_git_dir(wt->id))
> -		    die(_("unable to re-add worktree '%s'"), path);
> -		goto done;
> +		    die(_("unusable worktree destination '%s'"), path);
> +		return;
>  	}
>  
>  	if (locked)
> -		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.

Show 5 quoted lines

> -		die(_("'%s' is a missing but already registered worktree;\nuse 'add -f' to override, or 'prune' or 'remove' to clear"), path);
> -
> -done:
> -	free_worktrees(worktrees);
> +		die(_("'%s' is a missing but already registered worktree;\nuse '%s -f' to override, or 'prune' or 'remove' to clear"), cmd, path);

Similarly here.

Show 16 quoted lines

>  
>  static int add_worktree(const char *path, const char *refname,
> @@ -324,8 +323,12 @@ static int add_worktree(const char *path, const char *refname,
>  	struct commit *commit = NULL;
>  	int is_branch = 0;
>  	struct strbuf sb_name = STRBUF_INIT;
> +	struct worktree **worktrees;
>  
> -	validate_worktree_add(path, opts);
> +	worktrees = get_worktrees(0);
> +	check_candidate_path(path, opts->force, worktrees, "add");
> +	free_worktrees(worktrees);
> +	worktrees = NULL;
>  
>  	/* is 'refname' a branch or commit? */
>  	if (!opts->detach && !strbuf_check_branch_ref(&symref, refname) &&

It is necessary to call 'free_worktrees(worktrees)' at the end right?
The 'get_worktrees()' function states that
    The caller is responsible for freeing the memory from the returned
    worktree(s).