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

On Wed, Sep 16, 2026 at 6:29 AM D. Ben Knoble [off-list ref] wrote:

Hi Tyler,

On Tue, Sep 15, 2026 at 7:33 PM Tyler Cipriani [off-list ref] wrote:

quoted

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

Whoops, good catch, thanks!

quoted

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.

Ack, I can update the mood. My goal was to make a note that moving the
reachability check seems possible and might be a decent idea, but it's
a lot of change in one patch.

quoted

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.

quoted

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

quoted

+               /*
+                * 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.

That's correct to me, too.

I hemmed and hawed a bit about whether to only check _some of_ the
reject_reasons from the fast-forward check. But decided that the
advice in 2/3 would get people to the right outcome in cases I could
think of.

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.

I tried to indicate the rationale with comments, but I'm open to
changing the variable name, too. I felt that the "deferred" in the
name captured it, but the name also feels a little broad vs. what it
does.

Before I take a stab at a reroll for commit message updates + variable
names, I'd like to gather more feedback on the direction and
implementation of this series.

Thanks you for your thoughtful comments, Ben! I've appreciated how
you've helped me think about this feature.