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

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

Anders Kaseorg <andersk@mit.edu> writes:

> +	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.