Re: [PATCH v5 1/4] fetch: Protect branches checked out in all worktrees

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 10, 2021, 22:09 UTC
Message-ID: <xmqqr1bnwtln.fsf@gitster.g>
In-Reply-To: <20211109230941.2518143-1-andersk@mit.edu>

Anders Kaseorg <andersk@mit.edu> writes:

Show 38 quoted lines

> +	if (!update_head_ok &&
> +	    (wt = find_shared_symref("HEAD", ref->name)) &&
> +	    !wt->is_bare &&
>  	    !is_null_oid(&ref->old_oid)) {
>  		/*
>  		 * If this is the head, and it's not okay to update
>  		 * the head, and the old value of the head isn't empty...
>  		 */
>  		format_display(display, '!', _("[rejected]"),
> -			       _("can't fetch in current branch"),
> +			       wt->is_current ?
> +			       _("can't fetch in current branch") :
> +			       _("checked out in another worktree"),
>  			       remote, pretty_ref, summary_width);
>  		return 1;
>  	}
> @@ -1387,16 +1390,14 @@ static int prune_refs(struct refspec *rs, struct ref *ref_map,
>  
>  static void check_not_current_branch(struct ref *ref_map)
>  {
> -	struct branch *current_branch = branch_get(NULL);
> -
> -	if (is_bare_repository() || !current_branch)
> -		return;
> -
> +	const struct worktree *wt;
>  	for (; ref_map; ref_map = ref_map->next)
> -		if (ref_map->peer_ref && !strcmp(current_branch->refname,
> -					ref_map->peer_ref->name))
> -			die(_("Refusing to fetch into current branch %s "
> -			    "of non-bare repository"), current_branch->refname);
> +		if (ref_map->peer_ref &&
> +		    (wt = find_shared_symref("HEAD", ref_map->peer_ref->name)) &&
> +		    !wt->is_bare)
> +			die(_("Refusing to fetch into branch '%s' "
> +			      "checked out at '%s'"),
> +			    ref_map->peer_ref->name, wt->path);
>  }

Another thing for the next development cycle.

The find_shared_symref() function is handy (and more correct than dereferencing HEAD in the current worktree alone, of course), but its memory ownership model may need to be rethought.

The current semantics is a caller can call find_shared_symref() to receive at most one worktree, and the caller can use it UNTIL anybody makes another call to find_shared_symref(), at which point, the worktree instance becomes unusable and off limit. The caller cannot, and should not attempt to, free the worktree instance.

Each time find_shared_symref() is called, we enumerate all the worktrees and store them in a list that is static to the function. The returned worktree instance points into that list. It is not technically leaked because the static "worktrees" list in the function holds onto it, and each time the function is called, the old list of worktrees is discarded and rebuilt anew. What it means is that a code like the above, a loop in check_not_current_branch(), that repeatedly calls find_shared_symref() is both inefficient (because it takes many "snapshots" of the worktrees attached to the repository), and also risks an inconsistent view of the world (because it takes many "snapshots", and each iteration uses different ones).

I suspect that having a new API function that lets the above be rewritten along the lines of ...

	struct worktree **all = get_worktrees();
	for ( ; ref_map; ref_map = ref_map->next) {
        	if (!ref_map->peer_ref)
			continue;
                wt = find_wt_with_HEAD(all, ref->map->peer_ref->name);
		if (!wt->is_bare)
			die(_("..."));
	}
	free_worktrees(all);

... would help.

Thanks.