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

On Fri, Sep 11, 2026 at 12:55 AM Patrick Steinhardt [off-list ref] wrote:

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

quoted

"--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"?

Exactly.

quoted

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?

You got it. It should read something like: "False rejections: when src
is up-to-date with the tip of the remote ref, but..." etc.

I can clarify in a v4.

quoted

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.

quoted

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.

quoted

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

For deletions, there's no history on our side to check. Also, there
was an existing test case that ensured deletions were allowed. I took
that as intent and opted to keep that behavior. I'll clarify in the
commit.

quoted

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.

RESOLVE_REF_READING: agreed. Will add.
RESOLVE_REF_NO_RECURSE: For the current (v3) state that only looks at
"HEAD" that makes sense. But I'd expect --force-if-includes to
resolve, e.g., STABLE -> HEAD -> refs/heads/main -- that is, to
recurse through multiple symlinks. Otherwise, we'd reject a push we
could verify.

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?

Oooh, great catch! Folks could put in tags or specific oids, none of
which have a reflog to check. These are rejected in v3, but only by
happenstance since they lack a reflog (and with bad advice about
running "git pull").

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

Yes, that makes sense. Resolve symrefs to the target branch, then
check the branch's reflog.

I'll try that in v4.

This comment left me spiraling for a bit about tags. Like: you can
push tags and a tag and a branch might point to the same commit. BUT
tags don't have reflogs, so I think it makes sense to reject those as
unverifiable here, too.

quoted

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.

Ack.

Patrick

Thanks for the review!