Re: [PATCH v3 1/2] push: check pushed ref for --force-if-includes

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

Tyler Cipriani <tyler@tylercipriani.com> writes:

>  static void check_if_includes_upstream(struct ref *remote)
>  {
> -	struct ref *local = get_local_ref(remote->name);
> +	struct ref *local;
> +	const char *name;
> +	int flag;
> +
> +	if (!remote->peer_ref)
> +		return;

This function signals its displeasure by setting remote->unreachble
to true, so any early return means it is OK to force the push, right?

What is the significance of remote not having peer_ref?  Is it a
usage error (i.e., push is not updating anything over there, and it
makes me wonder what the command line to do so looks like)?  Is it a
programming error (i.e., if we are pushing to update no remote ref,
this function should never be called)?  If the latter, I wonder if
BUG() is more appropriate.

> +	/* A deletion has no local history to check against. */
> +	if (is_null_oid(&remote->peer_ref->new_oid))
> +		return;

The comment for this condition is clear.  If we are pushing to
delete, checking if our side once used to build on top of theirs
does not guarantee us anything, so we accept the loss of history.

> +	name = remote->peer_ref->name;
> +	if (!strcmp(name, "HEAD")) {
> +		name = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),
> +					       "HEAD", 0, NULL, &flag);
> +		if (!name || !(flag & REF_ISSYMREF)) {
> +			/* detached HEAD: no per-branch reflog to consult */
> +			remote->unreachable = 1;
> +			return;
> +		}
> +	}
> +
> +	local = get_local_ref(name);
>  	if (!local)
>  		return;

The same question here.

Are any of these silent "punt" returns tested below?  It does not
seem to add a new test about pushing-to-delete.

Thanks.

> diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh
> index cba26a872d..0c02151747 100755
> --- a/t/t5533-push-cas.sh
> +++ b/t/t5533-push-cas.sh
> @@ -396,4 +396,69 @@ test_expect_success '"--force-if-includes" should allow deletes' '
>  	)
>  '
>  
> +test_expect_success '"--force-if-includes" should allow forced update when using differently named branches' '
> +	setup_src_dup_dst &&
> +	test_when_finished "rm -fr dst src dup" &&
> +	(
> +		cd src &&
> +		git fetch &&
> +		git switch -c newbranch origin/main &&
> +		git rebase HEAD --onto HEAD^ &&
> +		git push --force-if-includes --force-with-lease origin newbranch:main
> +	)
> +'
> +test_expect_success '"--force-if-includes" should allow forced update from HEAD' '
> +	setup_src_dup_dst &&
> +	test_when_finished "rm -fr dst src dup" &&
> +	(
> +		cd src &&
> +		git fetch &&
> +		git switch -c newbranch origin/main &&
> +		git rebase HEAD --onto HEAD^ &&
> +		git push --force-if-includes --force-with-lease origin HEAD:main
> +	)
> +'
> +
> +test_expect_success '"--force-if-includes" should reject forced update from differently named branches when local lacks remote ref' '
> +	setup_src_dup_dst &&
> +	test_when_finished "rm -fr dst src dup" &&
> +	(
> +		cd src &&
> +		git fetch &&
> +		git switch main &&
> +		git reset --hard origin/main &&
> +		git switch --orphan orphan &&
> +		test_commit I &&
> +		test_must_fail git push --force-with-lease --force-if-includes origin orphan:main
> +	)
> +'
> +
> +test_expect_success '"--force-if-includes" should reject forced update from HEAD when it lacks remote ref' '
> +	setup_src_dup_dst &&
> +	test_when_finished "rm -fr dst src dup" &&
> +	(
> +		cd src &&
> +		git fetch &&
> +		git switch main &&
> +		git reset --hard origin/main &&
> +		git switch --orphan orphan &&
> +		test_commit I &&
> +		test_must_fail git push --force-with-lease --force-if-includes origin HEAD:main
> +	)
> +'
> +
> +test_expect_success '"--force-if-includes" should reject forced update from detached HEAD' '
> +	setup_src_dup_dst &&
> +	test_when_finished "rm -fr dst src dup" &&
> +	(
> +		cd src &&
> +		git fetch &&
> +		git switch main &&
> +		git reset --hard origin/main &&
> +		git switch -c newbranch origin/main &&
> +		git checkout HEAD^ &&
> +		test_must_fail git push --force-if-includes --force-with-lease origin HEAD:main
> +	)
> +'
> +
>  test_done