Subject: Re: [PATCH v3 5/6] worktree: add "lock" command

From: Junio C Hamano <[email-masked]>
Date: Tue, 31 May 2016 11:10:17 -0700

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

> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index f9dac37..84fe63b 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -14,6 +14,7 @@
>  static const char * const worktree_usage[] = {
>  	N_("git worktree add [<options>] <path> [<branch>]"),
>  	N_("git worktree list [<options>]"),
> +	N_("git worktree lock [<options>] <path>"),
>  	N_("git worktree prune [<options>]"),
>  	NULL
>  };
> @@ -459,6 +460,41 @@ static int list(int ac, const char **av, const char *prefix)
>  	return 0;
>  }
>  
> +static int lock_worktree(int ac, const char **av, const char *prefix)
> +{
> +	const char *reason = "", *old_reason;
> +	struct option options[] = {
> +		OPT_STRING(0, "reason", &reason, N_("string"),
> +			   N_("reason for locking")),
> +		OPT_END()
> +	};
> +	struct worktree **worktrees, *wt;
> +
> +	ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
> +	if (ac != 1)
> +		usage_with_options(worktree_usage, options);
> +
> +	worktrees = get_worktrees();
> +	wt = find_worktree(worktrees, prefix, av[0]);
> +	if (!wt)
> +		die(_("'%s' is not a working directory"), av[0]);
> +	if (is_main_worktree(wt))
> +		die(_("'%s' is a main working directory"), av[0]);
> +
> +	old_reason = wt->lock_reason;

This use pattern suggests that the reading of lock_reason should be
done lazily, doesn't it?  The other user of the "is it locked?"
information, which is builtin/worktree.c::prune_worktree(), does not
even use the get_worktrees() interface and instead it just checks if
the lock marker git_path("worktrees/*/locked") exists.

Perhaps you want one of these as a public interface:

	int worktree_is_locked(const char *id, const char **reason);
	int worktree_is_locked(struct worktree *wt, const char **reason);

where the caller can learn if a worktree is locked, and optionally why?