From: Tyler Cipriani Date: Mon, 14 Sep 2026 19:27:19 GMT Subject: Re: [PATCH v4 0/2] push: check pushed ref for --force-if-includes Message-ID: In-Reply-To: On Mon, Sep 14, 2026 at 7:03 AM D. Ben Knoble wrote: > Hi Tyler, Hi Ben! > On Mon, Sep 14, 2026 at 12:00 AM Tyler Cipriani wrote: > > > > Changes since v3: > > > > - check_if_includes_upstream unconditionally resolves peer_ref with > > RESOLVE_REF_READING, now all non-branch ref pushes will be rejected > > when using --force-if-includes > > - add test for --force-if-includes tag push 1/2 > > This is intriguing and seems like a significant behavior change, let's read on… It's definitely true that this is a behavior change and it'll add some friction to your process. And it's also true that the current behavior is failing to provide the guarantees it claims. > > Range-diff against v3: > > 1: da27c421ed ! 1: e7912c3fd0 push: check pushed ref for --force-if-includes > > @@ Commit message > > - 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. > > + Instead, use ref->peer_ref to locate a branch with a reflog. But if ref > > + does not resolve to a branch (e.g., a detached HEAD, a tag, an oid), > > + then we reject the push. The alternative would be to use HEAD's reflog, > > + which is too broad to tell us if the history being pushed includes the > > + tip of the remote. We need a per-branch reflog, which means that pushes > > + of a ref that do not resolve to a branch are rejected. Rejecting the > > + push of a ref like a detached HEAD already happens today (if the > > + same-named local branch lacks the remote tip); now the detached HEAD and > > + other non-branch pushes are explicitly rejected. > > So, we would now reject a force-push whose source is anything but a branch (with > force-if-includes, and that presumably includes push.useForceIfIncludes)? I should clarify, reject the force-push of any source not ultimately resolvable to a branch; e.g., HEAD will work if resolves to a branch. > > ++test_expect_success '"--force-if-includes" should reject forced update from tag' ' > > ++ 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^ && > > ++ git tag stable && > > ++ test_must_fail git push --force-if-includes --force-with-lease origin stable:main > > ++ ) > > ++' > > Which is what I think this test says. > > I think this would break a common thing I do at work (although this is soon to > be deprecated, so take my anecdote with appropriate salt; I can't claim that no > one else relies on it, of course): > > As I think I described in the message you linked, I have an alias "pf = push > --force-with-lease" and push.useForceIfIncludes=true in config. Our team has a > "main" release branch and a "hotfix" release branch for emergencies. When > hotfixing, we first reset the hotfix branch to the last tag to go out to our > production environment, which I typically do like this: > > # validate that we won't lose any interesting commits (no regressions) with > # something like > git log --oneline --graph --boundary --cherry-mark --left-right > origin/hotfix... > # push > git pf origin :hotfix > > (On a second pass before sending, I can't recall if this works as-is when I > don't have a local hotfix branch tracking origin/hotfix.) Yes, this workflow will break. And it will not work today without a local branch named "hotfix". It's broken today, insofar as this is a false pass since push.useForceIfIncludes is unable to say anything about whether you've integrated origin's hotfix branch into the , you're pushing so it only incidentally works. Today, git pf is actually checking that your refs/heads/hotfix's reflog has the tip of origin's refs/heads/hotfix. But it makes no promises about . That is, you could: git checkout hotfix && git pull # This line is what makes it work today git checkout --orphan junk git commit -m --allow-empty 'Totally unrelated empty commit' git tag git pf origin :hotfix And pf will allow that to happen since origin/hotfix's tip has been integrated with your local refs/heads/hotfix, which is what it's checking today. > If I'm reading this version right, I would now have to say > > git pf --no-force-if-includes origin :hotfix > > or perhaps better > > git pf --no-force-if-includes --force-with-lease=hotfix[:origin/hotfix] … > > probably after seeing a (hopefully improved?) message after the original > command. (Do I need to disable force-if-includes in the more-specific lease > command?) git pf --force-with-lease=hotfix:origin/hotfix origin :hotfix Should be sufficient and as I understand your process, that's what you're after. The explicit --force-with-lease argument makes --force-if-includes a no-op, so --no-force-if-includes should be unnecessary. > Now, on the one hand, enshrining existing behavior is good for backwards > compatibility but has earned us a bit of a reputation for not innovating in > useful ways ;) On the other, I wonder if the description of force-if-includes > allows some latitude to break with existing behavior here. > > The relevant docs say > > --force-if-includes, --no-force-if-includes > Force an update only if the tip of the remote-tracking ref has been > integrated locally. > > This option enables a check that verifies if the tip of the > remote-tracking ref is reachable from one of the "reflog" entries of > the local branch based in it for a rewrite. The check ensures that > any updates from the remote have been incorporated locally by > rejecting the forced update if that is not the case. > > It is unclear to me what "one of the 'reflog' entries of the local branch based > in it" means! Ignoring that, the surrounding text only talks about whether the > remote-tracking ref's tip (or "updates from the remote") have been "integrated > locally." > > So I think we *could* say that, in this case, we don't have enough information > from ":hotfix" to check whether "origin/hotfix" has been integrated locally > or not, and we should tighten the meaning of the check. (Perhaps when > "--force-with-lease=hotfix" is given, though, we now have more information > available to check---but that could be outside the scope of this series if we > don't mind breaking backwards compatibility now.) From my perspective, this is similar to the detached HEAD discussion from 2020[0] where "[the reflog of HEAD not attached to a branch] _does_ answer a different question from what we actually asked." [0]: I opted for a direction requiring explicit arguments to express intent, since that's the only way to ensure --force-if-includes aligns with (how I read) the documentation and the previous discussions. Specifically, with tags: - tags may have a reflog, but it answers a different question vs. "has this tag integrated changes from an upstream" it answers what oid/ref does this tag point to - tags may incidentally point at oids referenced by branches with reflogs, but there may also be several branches pointed to the same oid, so which would we choose? BUT I just realized there is existing, more fundamental breakage with --force-if-includes here that I'm making worse. There is one case where we do have enough information to say whether has integrated the tip of the remote-ref locally: fast-forward push. And that's actually broken today, too :) git --version git version 2.47.3 git clone repo.git repo && cd repo git commit --allow-empty -m 'Normal, no-force-needed fast forward commit' git reflog expire --expire=all --all # Regular fast-forward push fails, even though it does not require --force to begin with git push --force-with-lease --force-if-includes origin main ! [rejected] main -> main (remote ref updated since checkout) Checking for fast-forward happens after --force-if-includes checks the reflog. So that will need a fix… My change makes an existing problem more acute, and probably requires a fix before other fixes can merge. Otherwise, --force-if-includes will always fail when pushing tags and detached heads, even when they're fast forward changes, adding needless friction to otherwise safe pushes (e.g., for tags that fast-forward a branch). So v5 will require a third change that touches other functions in remote.c. :/ > Thanks, > D. Ben Knoble Thank you for all the review and thoughts!