Subject: Re: [PATCH 2/6] worktree.c: add update_worktree_location()

From: Junio C Hamano <[email-masked]>
Date: Thu, 20 Apr 2017 19:22:43 -0700

Nguyễn Thái Ngọc Duy  <[email-masked]> writes:

> Signed-off-by: Nguyễn Thái Ngọc Duy <[email-masked]>
> ---
>  worktree.c | 21 +++++++++++++++++++++
>  worktree.h |  6 ++++++
>  2 files changed, 27 insertions(+)
>
> diff --git a/worktree.c b/worktree.c
> index 40cc031ac9..c695dcf982 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -360,6 +360,27 @@ int validate_worktree(const struct worktree *wt, int quiet)
>  	return 0;
>  }
>  
> +int update_worktree_location(struct worktree *wt, const char *path_)
> +{
> +	struct strbuf path = STRBUF_INIT;
> +	int ret = 0;
> +
> +	if (is_main_worktree(wt))
> +		return 0;

There is no "refusing to move main worktree location" error message
issued?  Perhaps it is done elsewhere and this is merely for an
added safety (in which case it is OK)?

> +
> +	strbuf_add_absolute_path(&path, path_);
> +	if (fspathcmp(wt->path, path.buf)) {
> +		write_file(git_common_path("worktrees/%s/gitdir",
> +					   wt->id),
> +			   "%s/.git", real_path(path.buf));
> +		free(wt->path);
> +		wt->path = strbuf_detach(&path, NULL);
> +		ret = 0;
> +	}
> +	strbuf_release(&path);
> +	return ret;
> +}

Does anybody set "ret" to anything but 0 in this function?  It is
unclear what the return value from this function means in the first
place, but I am assuming that this is meant to follow the usual
convention of 0 for success and negative for error, in which case
the "return early if this is the primary one" would want to return
-1, I guess.

>  int is_worktree_being_rebased(const struct worktree *wt,
>  			      const char *target)
>  {
> diff --git a/worktree.h b/worktree.h
> index 33f7405e33..b896bdec55 100644
> --- a/worktree.h
> +++ b/worktree.h
> @@ -63,6 +63,12 @@ extern const char *is_worktree_locked(struct worktree *wt);
>  extern int validate_worktree(const struct worktree *wt, int quiet);
>  
>  /*
> + * Update worktrees/xxx/gitdir with the new path.
> + */
> +extern int update_worktree_location(struct worktree *wt,
> +				    const char *path_);
> +
> +/*
>   * Free up the memory for worktree(s)
>   */
>  extern void free_worktrees(struct worktree **);