Re: [PATCH v3 1/2] push: check pushed ref for --force-if-includes
- From
Tyler Cipriani <tyler@tylercipriani.com>
- Date
- Sep 11, 2026, 22:58 UTC
- Message-ID
- <CAHLx=OkhLqR8eQaW9q9W4SgpdtGoCrX4T7CwoGoxaT-fiuEUxA@mail.gmail.com>
- In-Reply-To
- <aqOlx5dlprfc0bdO@pks.im>
On Fri, Sep 11, 2026 at 12:55 AM Patrick Steinhardt <ps@pks.im> wrote:
Show 11 quoted lines
> > On Thu, Sep 10, 2026 at 05:05:05PM -0600, Tyler Cipriani wrote: > > "--force-if-includes" ensures, "tip of the remote-tracking ref is > > reachable from one of the 'reflog' entries of the local branch." > > > > But check_if_includes_upstream() uses the local per-branch reflog based > > on the destination branch rather than the branch being pushed; using > > ref->name vs. ref->peer_ref->name. > > So... in a `git push origin foo:bar` we look up the reflog for "bar" and > not "foo"?
Exactly.
Show 13 quoted lines
> > This can cause confusing rejections or unintended data loss. > > > > Using a command like: > > > > git push --force-if-includes --force-with-lease origin src:main > > > > False rejections: when src is an up-to-date branch, but main is > > out-of-date or nonexistent, then the includes check will fail telling > > users the remote ref has been updated since the last checkout. > > Hm. "up-to-date branch" in relation to what? You mean if we had commits > A, B and C, with C being the most recent commit, then "src" points to C > and "main" points to B?
You got it. It should read something like: "False rejections: when src is up-to-date with the tip of the remote ref, but..." etc.
I can clarify in a v4.
Show 31 quoted lines
> > Data loss: when src is an orphan/out-dated branch, but main is > > up-to-date, then the if-includes check will allow the push, clobbering > > the remote main. > > Right, here "src" would point to B and "main" would point to C. > > > Find local reflog using ref->peer_ref. When using a refspec like > > HEAD:refs/heads/main, we resolve HEAD. If HEAD is a branch, use that > > branch's reflog. > > > > But if HEAD does not resolve to a branch (i.e. a detached HEAD), then we > > reject the push. HEAD's reflog is too broad to tell us if the history > > being pushed includes the tip of the remote. Rejecting a detached HEAD > > already happens today (if the same-named local branch lacks the remote > > tip); now the detached HEAD state is explicitly rejected. > > Makes sense. > > > Skip deletions: > > > > git push --force-if-includes --force-with-lease origin :main > > > > ref->deletion is set after apply_push_cas (which triggers > > check_if_includes_upstream). The ref->peer_ref name is "(delete)". > > Instead check with is_null_oid to detect and allow deletion. > > This part feels a bit off to me. Deletions are the most risky operation > that we can do, so why would we want to just blindly allow them? There > may be good reasons for this, but if so those should be documented as > part of the commit message. It would probably even be sufficient to say > "it has worked this way before, and we don't want to break that case".
For deletions, there's no history on our side to check. Also, there was an existing test case that ensured deletions were allowed. I took that as intent and opted to keep that behavior. I'll clarify in the commit.
Show 28 quoted lines
> > diff --git a/remote.c b/remote.c
> > index 00723b385e..326af76eeb 100644
> > --- a/remote.c
> > +++ b/remote.c
> > @@ -2806,7 +2806,29 @@ static int is_reachable_in_reflog(const char *local, const struct ref *remote)
> > */
> > 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;
> > +
> > + /* A deletion has no local history to check against. */
> > + if (is_null_oid(&remote->peer_ref->new_oid))
> > + return;
> > +
> > + name = remote->peer_ref->name;
> > + if (!strcmp(name, "HEAD")) {
> > + name = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),
> > + "HEAD", 0, NULL, &flag);
>
> Shouldn't we pass `RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE` here?
> Otherwise, the function will return "HEAD" even if it could not be
> resolved, and we don't want to recursively resolve symrefs, either.RESOLVE_REF_READING: agreed. Will add. RESOLVE_REF_NO_RECURSE: For the current (v3) state that only looks at "HEAD" that makes sense. But I'd expect --force-if-includes to resolve, e.g., STABLE -> HEAD -> refs/heads/main -- that is, to recurse through multiple symlinks. Otherwise, we'd reject a push we could verify.
> Also, is it sufficient to single out "HEAD" here? It could for example > be that the user passes "HEAD~", an object ID or really any other > revision, and these should probably not be considered reachable, either, > right?
Oooh, great catch! Folks could put in tags or specific oids, none of which have a reflog to check. These are rejected in v3, but only by happenstance since they lack a reflog (and with bad advice about running "git pull").
> Maybe we should instead verify whether this names a local reference and, > if so, resolve potential symrefs to their target.
Yes, that makes sense. Resolve symrefs to the target branch, then check the branch's reflog.
I'll try that in v4.
This comment left me spiraling for a bit about tags. Like: you can push tags and a tag and a branch might point to the same commit. BUT tags don't have reflogs, so I think it makes sense to reject those as unverifiable here, too.
Show 21 quoted lines
> > diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh > > index cba26a872d..0c02151747 100755 > > --- a/t/t5533-push-cas.sh > > +++ b/t/t5533-push-cas.sh > > @@ -396,4 +396,69 @@ test_expect_success '"--force-if-includes" should allow deletes' ' > > ) > > ' > > > > +test_expect_success '"--force-if-includes" should allow forced update when using differently named branches' ' > > + setup_src_dup_dst && > > + test_when_finished "rm -fr dst src dup" && > > + ( > > + cd src && > > + git fetch && > > + git switch -c newbranch origin/main && > > + git rebase HEAD --onto HEAD^ && > > + git push --force-if-includes --force-with-lease origin newbranch:main > > + ) > > +' > > Nit: missing empty line between these two tests.
Ack.
> Patrick
Thanks for the review!