From: Tyler Cipriani Date: Wed, 16 Sep 2026 15:52:34 GMT Subject: Re: [PATCH v5 3/3] push: --force-if-includes should allow fast-forward Message-ID: In-Reply-To: On Wed, Sep 16, 2026 at 6:29 AM D. Ben Knoble wrote: > > Hi Tyler, > > On Tue, Sep 15, 2026 at 7:33 PM Tyler Cipriani 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" ? :) Whoops, good catch, thanks! > > 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. > > 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… > > > + /* > > + * 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.