Re: [PATCH v5 3/3] push: --force-if-includes should allow fast-forward
From: D. Ben Knoble <hidden>
Date: 2026-09-16 12:29:40
Hi Tyler, On Tue, Sep 15, 2026 at 7:33 PM Tyler Cipriani [off-list ref] wrote:
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.
quoted hunk ↗ jump to hunk
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 hunk ↗ jump to hunk
@@ -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…
+ /* + * 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