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

From: D. Ben Knoble <ben.knoble@gmail.com>
Date: Sep 14, 2026, 13:03 UTC
Message-ID: <CALnO6CDz8QBcBojmhjgwgWzi4oUbs+V4KVQ1h0+JgN7k0v-SYQ@mail.gmail.com>
In-Reply-To: <20260914040018.76111-1-tyler@tylercipriani.com>

Hi Tyler,

On Mon, Sep 14, 2026 at 12:00 AM Tyler Cipriani <tyler@tylercipriani.com> wrote:

Show 7 quoted lines

>
> 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…

Show 15 quoted lines

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

Show 14 quoted lines

>     ++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...<TAG>
    # push
    git pf origin <TAG>: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.)

If I'm reading this version right, I would now have to say

    git pf --no-force-if-includes origin <TAG>: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?)

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 "<TAG>: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.)

Thanks, D. Ben Knoble