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

From: Tyler Cipriani <tyler@tylercipriani.com>
Date: Sep 11, 2026, 23:47 UTC
Message-ID: <CAHLx=Om0-2J2ibJT+VeX3eEYsmsjY20QQcV_sns==7qOKLN7DA@mail.gmail.com>
In-Reply-To: <xmqq4ifverdh.fsf@gitster.g>

On Fri, Sep 11, 2026 at 9:31 AM Junio C Hamano <gitster@pobox.com> wrote:

Show 15 quoted lines

>
> Tyler Cipriani <tyler@tylercipriani.com> writes:
>
> >  static void check_if_includes_upstream(struct ref *remote)
> >  {
> > -     struct ref *local = get_local_ref(remote->name);
> > +     struct ref *local;
> > +     const char *name;
> > +     int flag;
> > +
> > +     if (!remote->peer_ref)
> > +             return;
>
> This function signals its displeasure by setting remote->unreachble
> to true, so any early return means it is OK to force the push, right?

That's true, for each ref that will be pushed. But this return does not imply it's OK to force push; refs with no peer_ref are not part of the push. The caller (apply_push_cas) walks every ref in remote_refs, then this function gets called for each ref that has check_reachable, regardless of whether it will later be pushed.

We could move this check to apply_push_cas to winnow what check_if_includes_upstream is responsible for checking and make every bare return mean "OK to force"; i.e., change apply_push_cas from:

if (ref->check_reachable)
    check_if_includes_upstream(ref);

to:

if (ref->peer_ref && ref->check_reachable)
    check_if_includes_upstream(ref);

And drop this return (and probably add a comment). I like that better.

Show 6 quoted lines

> What is the significance of remote not having peer_ref?  Is it a
> usage error (i.e., push is not updating anything over there, and it
> makes me wonder what the command line to do so looks like)?  Is it a
> programming error (i.e., if we are pushing to update no remote ref,
> this function should never be called)?  If the latter, I wonder if
> BUG() is more appropriate.

This is an ordinary path vs. BUG(). For the command:

git --force-with-lease --force-if-includes origin main

apply_push_cas checks all advertised refs. If there's no peer_ref, then remote.c's set_ref_status_for_push skips the ref before even checking ref->unreachable. When I ran the coverage report, this guard was hit regularly.

Show 7 quoted lines

> > +     /* A deletion has no local history to check against. */
> > +     if (is_null_oid(&remote->peer_ref->new_oid))
> > +             return;
>
> The comment for this condition is clear.  If we are pushing to
> delete, checking if our side once used to build on top of theirs
> does not guarantee us anything, so we accept the loss of history.

Agreed.

Show 16 quoted lines

> > +     name = remote->peer_ref->name;
> > +     if (!strcmp(name, "HEAD")) {
> > +             name = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),
> > +                                            "HEAD", 0, NULL, &flag);
> > +             if (!name || !(flag & REF_ISSYMREF)) {
> > +                     /* detached HEAD: no per-branch reflog to consult */
> > +                     remote->unreachable = 1;
> > +                     return;
> > +             }
> > +     }
> > +
> > +     local = get_local_ref(name);
> >       if (!local)
> >               return;
>
> The same question here.

get_local_ref should not return null. And when I ran the coverage report, this guard never ran. I'd be happy to remove it in v4.

> Are any of these silent "punt" returns tested below?  It does not
> seem to add a new test about pushing-to-delete.

There is an existing test for push-to-delete that this patch set kept.

> Thanks.

Thanks for the review!

Patrick suggested generalizing away from checking "HEAD" and I think that's the right call. I'll try that, plus adding your feedback (plus some additional detail in comments) in a v4.