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

[Tyler Cipriani <tyler@tylercipriani.com>]

Changes since v5:

- Update reflog rejection variable name to signal its use
  s/deferred_reject_reason/needs_force_reject_reason 3/3
- Typo fix: s/push force/force push/ in log message 3/3
- Remove imperative mood from log message 3/3
- Edit log message/comments for clarity and ensure the terms used for
  each agree 3/3
- Restored missing function words "to" and "a" in log message 2/3

Changes since v4:

- Add patch to series: Fix case where fast-forward pushes are being
  rejected by --force-if-includes: an existing bug that I made worse
  with the previous changes in my series.
- Add tests to cover allowed fast-forward merges when using
  --force-if-includes

Changes since v3:

- check_if_includes_upstream unconditionally resolves peer_ref with
  RESOLVE_REF_READING, now all non-branch ref pushes will be rejected
  when using --force-if-includes
- add test for --force-if-includes tag push 1/3
- clarify log message problem example 1/3
- clarify deletion in log message 1/3
- add missing blank line between test cases
- shorten long line in builtin/push.c
- reword advice-message wording 2/3
- rename 2/3 from "detached HEAD" to "non-branch"

Changes since v2:

- Correct patch threading of 1/3 and 2/3 to reply to cover letter of
  current patchset vs. cover letter of the initial iteration.

Changes since v1:

- Clarify in log message 1/3 that --force-if-includes will reject a
  detached HEAD today (when the same-named local branch lacks the remote
  tip). And note that this change makes it explicit to always reject the
  detached HEAD case.

--force-if-includes has been checking the reflog of the local branch
named after the destination branch regardless of what's being pushed.
This can cause false rejections or unintended data loss.

False rejection has been reported twice that I could find:

- 2023-07-26 - Stefan Haller reported local branch with a different name
  false rejection[0]
- 2025-05-08 - D. Ben Knoble reported detached HEAD false rejection[1]

The same root cause can result in data loss: when a same-name local
branch contains the remote tip but you --force-if-includes push an
unrelated branch, clobbering the remote repo. PoCs are in
t/t5533-push-cas.sh -- new test cases fail against maint, but pass with
patches applied.

Existing tests covered refspecs with different names for
--force-with-lease, but missed --force-if-includes. New patches cover:

- allow fast-forward push using --force-if-includes with an expired
  reflog
- allow fast-forward push of a tag on a different-named local branch
- allow forced update using refspec with different-named local branch
- allow same as above, but with HEAD
- reject force-update using refspec with different-named local branch
  lacking branch tip
- reject same as above using HEAD
- reject detached HEAD
- reject tag
- allow fast-forward push with an expired reflog
- allow fast-forward push from a tag

Resolved question: the detached HEAD case; HEAD's reflog was considered
and rejected as too broad for purpose in the original review. cf. [2]

Reject non-branch pushes due to lacking suitable reflogs for
--force-if-includes to determine if remote was integrated into ref being
pushed.

Fixes false rejections for pushes which never required force when using
--force-if-includes.

[0]: <https://lore.kernel.org/git/f51c73ed-eb03-83ca-fb31-d3e2645c9a63@haller-berlin.de>
[1]: <https://lore.kernel.org/git/CALnO6CCk0SgwObQRnpd5Pt_DvCKF8dBmyVHivU6Nr_O-GusGLA@mail.gmail.com>
[2]: <https://lore.kernel.org/git/CAHLx=O=tVhtiZpaRP9TpfiBfOMS2xPe3c3=mC3VNEdBrLOioFg@mail.gmail.com>

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 v5:
1:  e7912c3fd0 = 1:  e7912c3fd0 push: check pushed ref for --force-if-includes
2:  2a455d8a76 ! 2:  f85041efa8 push: fix --force-if-includes non-branch advice
    @@ Metadata
      ## Commit message ##
         push: fix --force-if-includes non-branch advice
     
    -    When a --force-if-includes push is rejected due lacking reflog to
    +    When a --force-if-includes push is rejected due to lacking a reflog to
         consult, the advice is misleading:
     
              ! [rejected] HEAD -> main (remote ref updated since checkout)
3:  1776f8d572 ! 3:  f9d97b0644 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 verify --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:
    @@ Commit message
         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.
    +    Instead, defer reflog rejection for --force-if-includes until after
    +    determining if a force push is needed.
     
    -    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.
    +    Remember the reflog rejection reason as needs_force_reject_reason. If
    +    the fast-forward rules reject the push for a ref, show the reflog
    +    rejection reason to preserve existing behavior. But if fast-forward
    +    rules allow a push (a fast-forward, deletion, or new ref), then a force
    +    push is unneeded, the reflog rejection reason is discarded, and the push
    +    proceeds.
     
         Signed-off-by: Tyler Cipriani <tyler@tylercipriani.com>
     
    @@ remote.c: 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;
    ++		int needs_force_reject_reason = 0;
      
      		if (ref->peer_ref)
      			oidcpy(&ref->new_oid, &ref->peer_ref->new_oid);
    @@ remote.c: void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
      		 * 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.
    ++		 * rejection in case force push is needed.
      		 */
      		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 =
    ++				needs_force_reject_reason =
      					REF_STATUS_REJECT_REMOTE_UPDATED;
      			else if (ref->check_reachable && ref->unverifiable)
     -				reject_reason =
    -+				deferred_reject_reason =
    ++				needs_force_reject_reason =
      					REF_STATUS_REJECT_UNVERIFIABLE;
      			else
      				/*
    @@ remote.c: void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
      		}
      
     +		/*
    -+		 * 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 fast-forward rules rejected the push 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;
    ++		if (needs_force_reject_reason && reject_reason)
    ++			reject_reason = needs_force_reject_reason;
     +
      		/*
      		 * "--force" will defeat any rejection implemented
-- 
2.47.3