Thread (44 messages) 44 messages, 4 authors, 2d ago

[PATCH v7 0/3] push: check pushed ref for --force-if-includes

flat view
WARM2d

From: Tyler Cipriani <hidden>
Date: 2026-10-18 20:33:35

Revision v7 of 4 in this series.

Revisions (4)
  1. v4 [diff vs current]
  2. v5 [diff vs current]
  3. v6 [diff vs current]
  4. v7 current
--force-if-includes is meant to refuse a forced push unless the tip of
the remote-tracking ref has been integrated into the ref you're pushing.
But it checks the reflog of the local branch named after the
destination, regardless of the ref being pushed.

For example, this command consults the reflog of main instead of src:

    git push --force-with-lease --force-if-includes origin src:main

Checking the wrong ref causes two problems:

- Data loss: if the local main has integrated origin/main, then the
  check passes even when src is an unrelated or outdated branch,
  allowing a push that clobbers the remote main.
- False rejections: if src has integrated origin/main but the local
  main is behind or missing, the push is rejected with "remote ref
  updated since checkout". Stefan Haller[0] and D. Ben Knoble[1] have
  both reported this.

1/3 checks the reflog of the branch that the pushed ref resolves to.
2/3 gives pushes that cannot be checked their own rejection status and
advice.
3/3 fixes an existing bug: --force-if-includes runs its reflog check
before the fast-forward rules, so it can reject a push that needs no
force at all, e.g., when the reflog has expired. 1/3 makes this worse,
since every push of a non-branch ref now fails the reflog check.

Behavior change
---------------

Only a branch's reflog records whether the remote tip was integrated
into it, so --force-if-includes can only verify a source that resolves
to a branch. HEAD's reflog spans all branches, and was rejected as too
broad in the original review[2]. A tag's reflog, when it exists, records
where the tag pointed. Mapping an oid to "the branch that points at it"
is ambiguous, since many branches can point at the same commit.

So a forced push whose source does not resolve to a branch (a detached
HEAD, a tag, an oid) is now rejected as unverifiable. The advice
suggests naming the expected value with
--force-with-lease=<ref>:<expect>, or skipping the check with
--no-force-if-includes.

This breaks workflows that only appear to work today. For example, this
passes today if a local "hotfix" branch has integrated origin/hotfix,
even though nothing about <tag> is checked:

    git push --force-with-lease --force-if-includes origin <tag>:hotfix

Fast-forward pushes of non-branch refs are still allowed (3/3), since a
fast-forward shows the remote tip has been integrated.

I'd welcome opinions on this trade-off.

Changes since v6:

- 1/3: fix a doubled "for" and the spelling of
  check_if_includes_upstream in the log message.
- 3/3: add missing "()" to function name in log message.
- Rebase on maint at v2.56.0, resolving conflicts with a85a43c480
  (push: suggest <remote> <branch> for a slash slip, 2026-06-27). The
  advice changes in the 2/3 range-diff are context from the
  pushRepoLooksLikeRef advice added there.
- The textual conflict in t5533 with as/push-force-if-includes-no-reflog
  remains: both topics add tests after the same existing test.

Earlier changes are listed in the v6 cover letter[3].

The new tests fail on maint and pass with this series applied.

[0]: <https://lore.kernel.org/git/f51c73ed-eb03-83ca-fb31-d3e2645c9a63@haller-berlin.de (local)>
[1]: <https://lore.kernel.org/git/CALnO6CCk0SgwObQRnpd5Pt_DvCKF8dBmyVHivU6Nr_O-GusGLA@mail.gmail.com (local)>
[2]: <https://lore.kernel.org/git/xmqqsgbdk69b.fsf@gitster.c.googlers.com/ (local)>
[3]: <https://lore.kernel.org/git/20260917224351.57171-1-tyler@tylercipriani.com/ (local)>

Tyler Cipriani (3):
  push: check pushed ref for --force-if-includes
  push: fix --force-if-includes non-branch advice
  push: --force-if-includes should allow fast-forward

 Documentation/config/advice.adoc |   4 ++
 advice.c                         |   1 +
 advice.h                         |   1 +
 builtin/push.c                   |  17 +++++
 builtin/send-pack.c              |   5 ++
 remote.c                         |  43 ++++++++++--
 remote.h                         |  10 ++-
 send-pack.c                      |   1 +
 t/t5533-push-cas.sh              | 115 ++++++++++++++++++++++++++++++-
 transport-helper.c               |   5 ++
 transport.c                      |   8 +++
 transport.h                      |   1 +
 12 files changed, 203 insertions(+), 8 deletions(-)

Range-diff against v6:
1:  e7912c3fd0 ! 1:  959490bb0a push: check pushed ref for --force-if-includes
    @@ Commit message
         The early return when peer_ref is missing in check_if_includes_upstream
         is necessary because apply_push_cas walks every advertised ref whenever
         use_tracking_for_rest is set (i.e., a bare --force-with-lease), so
    -    check_if_includes upstream is called for for refs that are not part of
    -    the push.
    +    check_if_includes upstream is called for refs that are not part of the
    +    push.
     
         Remove unnecessary check for empty return from get_local_ref, since it
         never returns NULL for a non-empty name.
2:  f85041efa8 ! 2:  92ef5412e1 push: fix --force-if-includes non-branch advice
    @@ Documentation/config/advice.adoc: all advice messages.
     +		Shown when linkgit:git-push[1] rejects a forced update of
     +		a branch when we are unable to verify the remote-tracking
     +		ref is integrated locally.
    - 	pushUnqualifiedRefname::
    - 		Shown when linkgit:git-push[1] gives up trying to
    - 		guess based on the source and destination refs what
    + 	pushRepoLooksLikeRef::
    + 		Shown when the repository given to linkgit:git-push[1] is not
    + 		a configured remote but looks like a `<remote>/<branch>` ref,
     
      ## advice.c ##
     @@ advice.c: static struct {
    @@ advice.c: static struct {
      	[ADVICE_PUSH_NON_FF_MATCHING]			= { "pushNonFFMatching" },
      	[ADVICE_PUSH_REF_NEEDS_UPDATE]			= { "pushRefNeedsUpdate" },
     +	[ADVICE_PUSH_REF_UNVERIFIABLE]			= { "pushRefUnverifiable" },
    + 	[ADVICE_PUSH_REPO_LOOKS_LIKE_REF]		= { "pushRepoLooksLikeRef" },
      	[ADVICE_PUSH_UNQUALIFIED_REF_NAME]		= { "pushUnqualifiedRefName" },
      	[ADVICE_PUSH_UPDATE_REJECTED]			= { "pushUpdateRejected" },
    - 	[ADVICE_PUSH_UPDATE_REJECTED_ALIAS]		= { "pushNonFastForward" }, /* backwards compatibility */
     
      ## advice.h ##
     @@ advice.h: enum advice_type {
    @@ advice.h: enum advice_type {
      	ADVICE_PUSH_NON_FF_MATCHING,
      	ADVICE_PUSH_REF_NEEDS_UPDATE,
     +	ADVICE_PUSH_REF_UNVERIFIABLE,
    + 	ADVICE_PUSH_REPO_LOOKS_LIKE_REF,
      	ADVICE_PUSH_UNQUALIFIED_REF_NAME,
      	ADVICE_PUSH_UPDATE_REJECTED,
    - 	ADVICE_PUSH_UPDATE_REJECTED_ALIAS,
     
      ## builtin/push.c ##
     @@ builtin/push.c: static const char message_advice_ref_needs_update[] =
3:  f9d97b0644 ! 3:  f97092092d push: --force-if-includes should allow fast-forward
    @@ Metadata
      ## Commit message ##
         push: --force-if-includes should allow fast-forward
     
    -    In set_ref_status_for_push, we apply --force-if-includes's reflog
    +    In set_ref_status_for_push(), we apply --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:
-- 
2.47.3
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help