[PATCH] rev-list: ignore replacements in the connectivity check

From: Kristofer Karlsson <krka@spotify.com>

The connectivity check follows replace refs, but other operations that depend on the repository being complete, such as fsck, prune, pack-objects and upload-pack, do not. They were taught to ignore replacements in dae556bdb1 (environment: add global variable to disable replacement, 2009-01-23), as they must work with the original objects, and check_connected() came later without doing the same.

Teach the connectivity check to ignore replacements as well, so that it verifies the same objects that the commands relying on its result will read. This is not a problem seen in practice, but the two can disagree. If a replace ref names an object that is not yet present locally, the check follows the replacement instead of that object when it arrives, and accepts an incoming pack that lacks objects below it. fsck and repack then fail. Such a replace ref can be the result of fetching refs/replace/* without the branch that contains the replaced commit, which fsck does not complain about, but it still takes a sender that sends an incomplete pack.

check_connected() passes --connectivity-check to its rev-list subprocess, so make rev-list disable replacements when that option is given, for both the full and the incremental check. Also drop OBJECT_INFO_LOOKUP_REPLACE from the object reads in tree-verify.c, used by the incremental check, to match. Disabling replacements already overrides the flag, but it suggests the opposite intent.

The t5413 test that expected the check to accept a commit whose original tree is incomplete because a replacement is complete now expects a rejection, and the test that disabled replacements explicitly to get one is redundant.

Signed-off-by: Kristofer Karlsson <krka@spotify.com>
---
    rev-list: ignore replacements in the connectivity check
    
    I noticed this while working on the incremental connectivity check [1].
    I could not find any discussion of why check_connected() follows replace
    refs when fsck, prune, pack-objects and upload-pack do not, and the
    history suggests that it was never a deliberate choice.
    
    This is meant both as a question and a proposed patch: should the
    connectivity check ignore replacements like those commands do, or is
    there a reason for it to respect them? If there is a good reason to keep
    the current behavior, I don't have any issue with dropping the patch
    directly -- you would not even have to look at the code :) If not, I
    think aligning them is a reasonable thing to do.
    
    The case described in the commit message, where the two disagree, is
    tricky to trigger in practice, so I do not expect it to be a common
    problem. The main benefit is logical consistency.
    
    Shallow boundaries are not affected, since they use the graft mechanism
    internally, not replace refs.
    
    [1]
    https://lore.kernel.org/git/pull.2211.v4.git.1792581744.gitgitgadget@gmail.com/

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2279%2Fspkrka%2Freplace-connectivity-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2279/spkrka/replace-connectivity-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2279

 Documentation/rev-list-options.adoc |  1 +
 builtin/rev-list.c                  |  3 +++
 t/t5413-connectivity-check.sh       | 11 +----------
 tree-verify.c                       |  2 --
 4 files changed, 5 insertions(+), 12 deletions(-)