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

From: Patrick Steinhardt <ps@pks.im>
Date: Sep 11, 2026, 06:55 UTC
Message-ID: <aqOlx5dlprfc0bdO@pks.im>
In-Reply-To: <20260910230506.1631656-2-tyler@tylercipriani.com>

On Thu, Sep 10, 2026 at 05:05:05PM -0600, Tyler Cipriani wrote:

Show 6 quoted lines

> "--force-if-includes" ensures, "tip of the remote-tracking ref is
> reachable from one of the 'reflog' entries of the local branch."
> 
> But check_if_includes_upstream() uses the local per-branch reflog based
> on the destination branch rather than the branch being pushed; using
> ref->name vs. ref->peer_ref->name.

So... in a `git push origin foo:bar` we look up the reflog for "bar" and not "foo"?

Show 9 quoted lines

> This can cause confusing rejections or unintended data loss.
> 
> Using a command like:
> 
>     git push --force-if-includes --force-with-lease origin src:main
> 
> False rejections: when src is an up-to-date branch, but main is
> out-of-date or nonexistent, then the includes check will fail telling
> users the remote ref has been updated since the last checkout.

Hm. "up-to-date branch" in relation to what? You mean if we had commits A, B and C, with C being the most recent commit, then "src" points to C and "main" points to B?

> Data loss: when src is an orphan/out-dated branch, but main is
> up-to-date, then the if-includes check will allow the push, clobbering
> the remote main.

Right, here "src" would point to B and "main" would point to C.

Show 9 quoted lines

> Find local reflog using ref->peer_ref. When using a refspec like
> HEAD:refs/heads/main, we resolve HEAD. If HEAD is a branch, use that
> branch's reflog.
> 
> But if HEAD does not resolve to a branch (i.e. a detached HEAD), then we
> reject the push. HEAD's reflog is too broad to tell us if the history
> being pushed includes the tip of the remote. Rejecting a detached HEAD
> already happens today (if the same-named local branch lacks the remote
> tip); now the detached HEAD state is explicitly rejected.

Makes sense.

Show 7 quoted lines

> Skip deletions:
> 
>     git push --force-if-includes --force-with-lease origin :main
> 
> ref->deletion is set after apply_push_cas (which triggers
> check_if_includes_upstream). The ref->peer_ref name is "(delete)".
> Instead check with is_null_oid to detect and allow deletion.

This part feels a bit off to me. Deletions are the most risky operation that we can do, so why would we want to just blindly allow them? There may be good reasons for this, but if so those should be documented as part of the commit message. It would probably even be sufficient to say "it has worked this way before, and we don't want to break that case".

Show 24 quoted lines

> diff --git a/remote.c b/remote.c
> index 00723b385e..326af76eeb 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -2806,7 +2806,29 @@ static int is_reachable_in_reflog(const char *local, const struct ref *remote)
>   */
>  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;
> +
> +	/* A deletion has no local history to check against. */
> +	if (is_null_oid(&remote->peer_ref->new_oid))
> +		return;
> +
> +	name = remote->peer_ref->name;
> +	if (!strcmp(name, "HEAD")) {
> +		name = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),
> +					       "HEAD", 0, NULL, &flag);

Shouldn't we pass `RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE` here? Otherwise, the function will return "HEAD" even if it could not be resolved, and we don't want to recursively resolve symrefs, either.

Also, is it sufficient to single out "HEAD" here? It could for example be that the user passes "HEAD~", an object ID or really any other revision, and these should probably not be considered reachable, either, right?

Maybe we should instead verify whether this names a local reference and, if so, resolve potential symrefs to their target.

Show 19 quoted lines

> 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
> +	)
> +'

Nit: missing empty line between these two tests.

Patrick