Re: [PATCH 3/4] fetch: use new branch_checked_out() and add tests

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Stolee

On 08/06/2022 21:09, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <derrickstolee@github.com>
> [...]

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index ac29c2b1ae3..1ba56240312 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)) {
>   		/*
>   		 * 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]"),
> -			       wt->is_current ?
> -				       _("can't fetch in current branch") :
> -				       _("checked out in another worktree"),
> +			       path ? _("can't fetch in current branch") :
> +				      _("checked out in another worktree"),

I'm confused by this, isn't path always non-null?

Best Wishes

Phillip

>   			       remote, pretty_ref, summary_width);
> +		free(path);
>   		return 1;
>   	}
>   
> @@ -1434,19 +1434,16 @@ cleanup:
>   	return result;
>   }
>   
> -static void check_not_current_branch(struct ref *ref_map,
> -				     struct worktree **worktrees)
> +static void check_not_current_branch(struct ref *ref_map)
>   {
> -	const struct worktree *wt;
> +	char *path;
>   	for (; ref_map; ref_map = ref_map->next)
>   		if (ref_map->peer_ref &&
>   		    starts_with(ref_map->peer_ref->name, "refs/heads/") &&
> -		    (wt = find_shared_symref(worktrees, "HEAD",
> -					     ref_map->peer_ref->name)) &&
> -		    !wt->is_bare)
> +		    branch_checked_out(ref_map->peer_ref->name, &path))
>   			die(_("refusing to fetch into branch '%s' "
>   			      "checked out at '%s'"),
> -			    ref_map->peer_ref->name, wt->path);
> +			    ref_map->peer_ref->name, path);
>   }
>   
>   static int truncate_fetch_head(void)
> @@ -1650,7 +1647,7 @@ static int do_fetch(struct transport *transport,
>   	ref_map = get_ref_map(transport->remote, remote_refs, rs,
>   			      tags, &autotags);
>   	if (!update_head_ok)
> -		check_not_current_branch(ref_map, worktrees);
> +		check_not_current_branch(ref_map);
>   
>   	retcode = open_fetch_head(&fetch_head);
>   	if (retcode)
> diff --git a/t/t2407-worktree-heads.sh b/t/t2407-worktree-heads.sh
> index 12faca7f655..f3f8b0b2b79 100755
> --- a/t/t2407-worktree-heads.sh
> +++ b/t/t2407-worktree-heads.sh
> @@ -10,6 +10,15 @@ test_expect_success 'setup' '
>   		test_commit $i &&
>   		git branch wt-$i &&
>   		git worktree add wt-$i wt-$i || return 1
> +	done &&
> +
> +	# Create a server that updates each branch by one commit
> +	git clone . server &&
> +	git remote add server ./server &&
> +	for i in 1 2 3 4
> +	do
> +		git -C server checkout wt-$i &&
> +		test_commit -C server A-$i || return 1
>   	done
>   '
>   
> @@ -21,6 +30,16 @@ test_expect_success 'refuse to overwrite: checked out in worktree' '
>   	done
>   '
>   
> +test_expect_success 'refuse to overwrite during fetch' '
> +	test_must_fail git fetch server +refs/heads/wt-3:refs/heads/wt-3 2>err &&
> +	grep "refusing to fetch into branch '\''refs/heads/wt-3'\''" err &&
> +
> +	# General fetch into refs/heads/ will fail on first ref,
> +	# so use a generic error message check.
> +	test_must_fail git fetch server +refs/heads/*:refs/heads/* 2>err &&
> +	grep "refusing to fetch into branch" err
> +'
> +
>   test_expect_success 'refuse to overwrite: worktree in bisect' '
>   	test_when_finished test_might_fail git -C wt-4 bisect reset &&
>   
> @@ -31,7 +50,10 @@ test_expect_success 'refuse to overwrite: worktree in bisect' '
>   	) &&
>   
>   	test_must_fail git branch -f wt-4 HEAD 2>err &&
> -	grep "cannot force update the branch '\''wt-4'\'' checked out at" err
> +	grep "cannot force update the branch '\''wt-4'\'' checked out at" err &&
> +
> +	test_must_fail git fetch server +refs/heads/wt-4:refs/heads/wt-4 2>err &&
> +	grep "refusing to fetch into branch '\''refs/heads/wt-4'\''" err
>   '
>   
>   . "$TEST_DIRECTORY"/lib-rebase.sh
> @@ -47,7 +69,10 @@ test_expect_success 'refuse to overwrite: worktree in rebase' '
>   	) &&
>   
>   	test_must_fail git branch -f wt-4 HEAD 2>err &&
> -	grep "cannot force update the branch '\''wt-4'\'' checked out at" err
> +	grep "cannot force update the branch '\''wt-4'\'' checked out at" err &&
> +
> +	test_must_fail git fetch server +refs/heads/wt-4:refs/heads/wt-4 2>err &&
> +	grep "refusing to fetch into branch '\''refs/heads/wt-4'\''" err
>   '
>   
>   test_done