Re: [PATCH v5 3/3] push: --force-if-includes should allow fast-forward

From: D. Ben Knoble <ben.knoble@gmail.com>
Date: Sep 16, 2026, 12:29 UTC
Message-ID: <CALnO6CCpxwenphyZvEX7gjvAmLPidv+4iT98uh95mXj_MvshQg@mail.gmail.com>
In-Reply-To: <20260915233305.334115-4-tyler@tylercipriani.com>

Hi Tyler,

On Tue, Sep 15, 2026 at 7:33 PM Tyler Cipriani <tyler@tylercipriani.com> wrote:

Show 18 quoted lines

>
> In set_ref_status_for_push, we verify --force-if-includes's reflog
> reachability checks before fast-forward rules. As a result, valid
> fast-forward pushes may be rejected when a force push is unneeded; like
> when the reflog is expired:
>
>     git clone repo.git repo
>     git commit --allow-empty -m 1
>     git reflog expire --expire=all --all
>     git push --force-with-lease --force-if-includes origin main
>     ! [rejected]    main -> main (remote ref updated since checkout)
>
> Rejecting fast-forwards is a mismatch with the --force-if-includes
> documentation "Force an update only if the tip of the remote-tracking
> ref has been integrated locally."
>
> Instead, defer check for --force-if-includes until after determining if
> a push force is needed.

"push force" ? :)

> Opted to create a deferred_reject_reason in set_ref_status_for_push
> rather than move the computation of reachability or verifiability to
> winnow scope of changes in this patch. Lazily checking for reachability
> or verifiability is a valid followup.

This paragraph does not match our usual style (Documentation/SubmittingPatches[[imperative-mood]]) and feels somewhat artificial to me.

Show 33 quoted lines

> diff --git a/remote.c b/remote.c
> index b7b5ac0d28..db0b50b030 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1669,6 +1669,7 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
>         for (ref = remote_refs; ref; ref = ref->next) {
>                 int force_ref_update = ref->force || force_update;
>                 int reject_reason = 0;
> +               int deferred_reject_reason = 0;
>
>                 if (ref->peer_ref)
>                         oidcpy(&ref->new_oid, &ref->peer_ref->new_oid);
> @@ -1693,16 +1694,17 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
>                  *
>                  * If the tip of the remote-tracking ref is unreachable
>                  * from any reflog entry of its local ref indicating a
> -                * possible update since checkout; reject the push.
> +                * possible update since checkout, then remember the
> +                * rejection in case the push is non-fast-forward.
>                  */
>                 if (ref->expect_old_sha1) {
>                         if (!oideq(&ref->old_oid, &ref->old_oid_expect))
>                                 reject_reason = REF_STATUS_REJECT_STALE;
>                         else if (ref->check_reachable && ref->unreachable)
> -                               reject_reason =
> +                               deferred_reject_reason =
>                                         REF_STATUS_REJECT_REMOTE_UPDATED;
>                         else if (ref->check_reachable && ref->unverifiable)
> -                               reject_reason =
> +                               deferred_reject_reason =
>                                         REF_STATUS_REJECT_UNVERIFIABLE;
>                         else
>                                 /*

From these 2 hunks, I haven't yet seen the connection to avoiding a rejected force-push in the fast-forward case, but my read is: we remember why we might reject a force-push for refs whose reachability we are supposed to check.

> @@ -1746,6 +1748,14 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
>                                 reject_reason = REF_STATUS_REJECT_NONFASTFORWARD;
>                 }

Then in unshown code, in those 2 "remembered" cases, we check the must fast-forward rules. If any fail, we set reject_reason…

Show 8 quoted lines

> +               /*
> +                * If push is non-fast-forward and we were asked to
> +                * verify the reflog but were unable to, then reflog
> +                * verification is the right reject_reason.
> +                */
> +               if (deferred_reject_reason && reject_reason)
> +                       reject_reason = deferred_reject_reason;
> +

…which we now overwrite with our remembered reason in the rejected case. I think that makes sense.

At first I thought the unshown code above, conditional on !reject_reason, would collude to make it so "deferred_reject_reason && reject_reason" could never be true, but I was misreading the results of this patch. There are some arms in which we set both (namely, because those remembered cases don't set reject_reason, allowing the fast-forward rules checks).

I still wonder a bit about cases where we remember deferred_reject_reason and never set reject_reason, but I think those are supposed to only be the fast-forward cases. Perhaps we want to make "deferred_reject_reason" more clearly indicate that to save future readers headache if they insert code around here? I'm not sure the best way to do that, though, so maybe blaming to the log message will suffice.

-- 
D. Ben Knoble