Re: [PATCH v2 1/3] branch: fix die_if_checked_out() when ignore_current_worktree

[Junio C Hamano <gitster@pobox.com>]

Rubén Justo <rjusto@gmail.com> writes:

> Let's stop using find_shared_symref() in die_if_checked_out(), to handle
> correctly ignore_current_worktree.

This says what the code stops using, but does not say what it uses
instead.

Factoring is_shared_symref() out of find_shared_symref() is probably
a good idea that can stand alone without any other change in this
patch as a single step, and then a second step can update
die_if_checked_out() using the new function.

As the proposed log message explained, updating die_if_checked_out()
with this patch would fix a bug---can we demonstrate the existing
breakage and protect the fix from future breakages by adding a test
or two?

Other than that, it looks like it is going in the right direction.
Thanks for working on the topic.

> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>  branch.c   | 16 +++++++++++-----
>  worktree.c | 54 +++++++++++++++++++++++++++++-------------------------
>  worktree.h |  6 ++++++
>  3 files changed, 46 insertions(+), 30 deletions(-)
>
> diff --git a/branch.c b/branch.c
> index d182756827..2378368415 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -820,12 +820,18 @@ void remove_branch_state(struct repository *r, int verbose)
>  void die_if_checked_out(const char *branch, int ignore_current_worktree)
>  {
>  	struct worktree **worktrees = get_worktrees();
> -	const struct worktree *wt;
> +	int i;
> +
> +	for (i = 0; worktrees[i]; i++)
> +	{

Style.  WRite the above on a single line, i.e.

	for (i = 0; worktrees[i]; i++) {

Optionally, we can lose the separate declaration of "i" by using C99
variable declaration, i.e.

	for (int i = 0; worktrees[i]; i++) {

> diff --git a/worktree.c b/worktree.c
> index aa43c64119..d500d69e4c 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -403,6 +403,33 @@ int is_worktree_being_bisected(const struct worktree *wt,
>   * bisect). New commands that do similar things should update this
>   * function as well.
>   */

The above comment is about find_shared_symref() which iterates over
worktrees and find the one that uses the named symref.  Now the
comment appears to apply to is_shared_symref() which does not
iterate but takes one specific worktree instance.  Do their
differences necessitate some updates to the comment?

> +int is_shared_symref(const struct worktree *wt, const char *symref,
> +		     const char *target)
> +{

What this function does sound more like "is target in use in this
particular worktree by being pointed at by the symref?"  IOW, I do
not see where "shared" comes into its name from.

"HEAD" that is tentatively detached while bisecting or rebasing the
"target" branch is still considered to point at the "target", so
perhaps symref_points_at_target() or something?

>  const struct worktree *find_shared_symref(struct worktree **worktrees,
>  					  const char *symref,
>  					  const char *target)
> @@ -411,31 +438,8 @@ const struct worktree *find_shared_symref(struct worktree **worktrees,
>  	int i = 0;
>  
>  	for (i = 0; worktrees[i]; i++) {

Not a new problem, but the initialization on the declaration of "i"
is redundant and unnecessary.  Again, we can use the C99 style, i.e.

	const struct worktree *existing = NULL;
-	int i = 0;
-
-	for (i = 0; worktrees[i]; i++) {
+	for (int i = 0; worktrees[i]; i++) {

> +		if (is_shared_symref(worktrees[i], symref, target)) {
> +			existing = worktrees[i];
>  			break;
>  		}
>  	}
> diff --git a/worktree.h b/worktree.h
> index 9dcea6fc8c..7889c4761d 100644
> --- a/worktree.h
> +++ b/worktree.h
> @@ -149,6 +149,12 @@ const struct worktree *find_shared_symref(struct worktree **worktrees,
>  					  const char *symref,
>  					  const char *target);
>  
> +/*
> + * Returns true if a symref points to a ref in a worktree.
> + */

Make it clear that what you called "a ref" in the above is what is
called "target" below.

> +int is_shared_symref(const struct worktree *wt,
> +		     const char *symref, const char *target);
> +
>  /*
>   * Similar to head_ref() for all HEADs _except_ one from the current
>   * worktree, which is covered by head_ref().