Re: [PATCH v3 1/2] push: check pushed ref for --force-if-includes
From: Patrick Steinhardt <hidden>
Date: 2026-09-11 06:55:17
On Thu, Sep 10, 2026 at 05:05:05PM -0600, Tyler Cipriani wrote:
"--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"?
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.
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.
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".
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
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