Re: [PATCH v2 2/7] branch: add branch_checked_out() helper

From: Junio C Hamano <gitster@pobox.com>
To: Derrick Stolee <derrickstolee@github.com>
Cc: Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, johannes.schindelin@gmx.de, me@ttaylorr.com,
	Jeff Hostetler <git@jeffhostetler.com>,
	Phillip Wood <phillip.wood123@gmail.com>,
	Elijah Newren <newren@gmail.com>
Subject: Re: [PATCH v2 2/7] branch: add branch_checked_out() helper
Date: Tue, 07 Jun 2022 21:52:22 -0700	[thread overview]
Message-ID: <xmqqleu7sqnt.fsf@gitster.g> (raw)
In-Reply-To: <74d5f74b-c95f-6892-6a66-3e646b1f9e53@github.com> (Derrick Stolee's message of "Tue, 7 Jun 2022 22:43:00 -0400")

Derrick Stolee <derrickstolee@github.com> writes:

>> Yes, that's a good plan. I'll take a look.
>
> Here is a fixup for this patch that should work. I'll put it in v3.
>
> diff --git a/branch.c b/branch.c
> index 2e6419cdfa5..514212f5619 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -10,6 +10,7 @@
>  #include "worktree.h"
>  #include "submodule-config.h"
>  #include "run-command.h"
> +#include "strmap.h"
>  
>  struct tracking {
>  	struct refspec_item spec;
> @@ -369,17 +370,44 @@ int validate_branchname(const char *name, struct strbuf *ref)
>  	return ref_exists(ref->buf);
>  }
>  
> -int branch_checked_out(const char *refname, char **path)
> +static int initialized_checked_out_branches;
> +static struct strmap current_checked_out_branches = STRMAP_INIT;
> +
> +static void prepare_checked_out_branches(void)
>  {
> +	int i = 0;
> +	struct worktree **worktrees;
> +
> +	if (initialized_checked_out_branches)
> +		return;
> +	initialized_checked_out_branches = 1;
> +
> +	worktrees = get_worktrees();
> +
> +	while (worktrees[i]) {
> +		struct worktree *wt = worktrees[i];
>  
> +		if (!wt->is_bare && wt->head_ref)
> +			strmap_put(&current_checked_out_branches,
> +				   wt->head_ref,
> +				   wt->path);
> +
> +		i++;
> +	}
>  	free_worktrees(worktrees);
> -	return result;
> +}

Yeah, the above illustrates what I had in mind pretty closely.  The
above outline only checks where the HEAD symref points at, and it
does not protect a branch that is in the middle of getting bisected
or rebased, which find_shared_symref() tries to do, IIRC, though.

It should not be too hard to add that to the above to allow us to
achieve feature parity with the original.

> +
> +int branch_checked_out(const char *refname, char **path)
> +{
> +	const char *path_in_set;
> +	prepare_checked_out_branches();
> +
> +	path_in_set = strmap_get(&current_checked_out_branches, refname);
> +	if (path_in_set && path)
> +		*path = xstrdup(path_in_set);
> +
> +	return !!path_in_set;
>  }

This one looks quite straight-forward.

> And there is another use of find_shared_symref() in the same file, allowing
> us to do the following:
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index ac29c2b1ae3..3933c482839 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -885,7 +885,7 @@ static int update_local_ref(struct ref *ref,
>  			    struct worktree **worktrees)
>  {
>  	struct commit *current = NULL, *updated;
> -	const struct worktree *wt;
> +	char *path = NULL;
>  	const char *pretty_ref = prettify_refname(ref->name);
>  	int fast_forward = 0;
>  
> @@ -900,17 +900,17 @@ static int update_local_ref(struct ref *ref,
>  	}
>  
>  	if (!update_head_ok &&
> -	    (wt = find_shared_symref(worktrees, "HEAD", ref->name)) &&
> -	    !wt->is_bare && !is_null_oid(&ref->old_oid)) {
> +	    !is_null_oid(&ref->old_oid) &&
> +	    branch_checked_out(ref->name, &path)) {

Yes.  It looks like a good direction to go in.

Thanks.