From: Ben Knoble Date: Sat, 05 Sep 2026 18:57:03 GMT Subject: Re: [PATCH 1/2] push: check pushed ref for --force-if-includes Message-ID: In-Reply-To: <20260904210122.431757-2-tyler@tylercipriani.com> > Le 4 sept. 2026 à 17:01, Tyler Cipriani a écrit : > > "--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. > > 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. > > 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. Hm. This case *could* be by design, to rewind and potentially modify a remote branch, discarding new work I’ve already checked. But the includes check is about reminding to do such a check. So failing and requiring me to bypass the check seems ok. > Find local reflog using ref->peer_ref. When using a refspec like > HEAD:refs/heads/main, we resolve HEAD to a branch and use that reflog. > In a detached HEAD state, the reflog cannot tell us if the history > being pushed includes the tip of the remote, so the push is rejected. This seems to be what I reported in the mail your cover letter cites. So, am I reading correctly that this is no change from current behavior? …ah, patch 2 addresses that specifically. Which, I now remember you said in the cover as well. Oops. It *could* be worth clarifying in the proposed log message that we are only preserving behavior here, but that’s a very small nit. > 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. > > Reported-by: Stefan Haller > Reported-by: D. Ben Knoble > Signed-off-by: Tyler Cipriani > --- > remote.c | 24 ++++++++++++++++- > t/t5533-push-cas.sh | 65 +++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 88 insertions(+), 1 deletion(-) > > 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); > + 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; > > 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 > -- > 2.47.3