Kristofer Karlsson via GitGitGadgetFrom: 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(-)
Show changes to 4 files +5 −12
Documentation/rev-list-options.adoc, builtin/rev-list.c, t/t5413-connectivity-check.sh, tree-verify.c
diff --git a/Documentation/rev-list-options.adoc b/Documentation/rev-list-options.adoc index 285fb3809f..b2c0d394ab 100644 --- a/Documentation/rev-list-options.adoc +++ b/Documentation/rev-list-options.adoc @@ -1107,6 +1107,7 @@ It is an error to use `--missing-only` without `--missing=print` or (For internal use only.) Used by `check_connected()` to select the connectivity check algorithm; see `transfer.connectivityCheck` in linkgit:git-config[1]. + Replacement objects are ignored, as with `--no-replace-objects`. endif::git-rev-list[] `--no-walk[=(sorted|unsorted)]`:: diff --git a/builtin/rev-list.c b/builtin/rev-list.c index 071a0e3cb5..bf66d2c9ae 100644 --- a/builtin/rev-list.c +++ b/builtin/rev-list.c @@ -30,6 +30,7 @@ #include "strbuf.h" #include "tree-verify.h" #include "parse.h" +#include "replace-object.h" #define INCREMENTAL_MAX_COMMITS 1000 @@ -807,6 +808,8 @@ int cmd_rev_list(int argc, revs.exclude_promisor_objects = 1; } else if (skip_prefix(arg, "--connectivity-check=", &arg)) { parse_connectivity_check(arg, &connectivity_check); + /* Check the original objects, like fsck and pack-objects do. */ + disable_replace_refs(); } else if (skip_prefix(arg, "--missing=", &arg)) { parse_missing_action_value(repo, arg); } else if (!strcmp(arg, "--missing-only")) { diff --git a/t/t5413-connectivity-check.sh b/t/t5413-connectivity-check.sh index 60edf99b63..341638a433 100755 --- a/t/t5413-connectivity-check.sh +++ b/t/t5413-connectivity-check.sh @@ -427,18 +427,9 @@ test_expect_success "$mode: handles directory-to-file transition" ' # Replacement objects. -test_expect_success "$mode: accepts with replacement objects" ' +test_expect_success "$mode: ignores replacement objects" ' ( cd replace-test && - check_connected "$original_oid" - ) -' - -test_expect_success "$mode: rejects without replacement objects" ' - ( - cd replace-test && - GIT_NO_REPLACE_OBJECTS=1 && - export GIT_NO_REPLACE_OBJECTS && test_expect_code 128 check_connected \ "$original_oid" 2>err && test_grep "missing blob object.*$orig_blob_oid" err diff --git a/tree-verify.c b/tree-verify.c index 25db76d30b..c228235150 100644 --- a/tree-verify.c +++ b/tree-verify.c @@ -133,7 +133,6 @@ static int verify_blob(struct repository *repo, vs->blobs_checked++; oi.typep = &type; if (odb_read_object_info_extended(repo->objects, oid, &oi, - OBJECT_INFO_LOOKUP_REPLACE | OBJECT_INFO_SKIP_FETCH_OBJECT)) { if (vs->exclude_promisor_objects && is_promisor_object(repo, oid)) @@ -163,7 +162,6 @@ static int read_tree_object(struct object_database *odb, oi.sizep = sizep; oi.contentp = bufp; if (odb_read_object_info_extended(odb, oid, &oi, - OBJECT_INFO_LOOKUP_REPLACE | OBJECT_INFO_SKIP_FETCH_OBJECT)) *bufp = NULL; if (*bufp && type != OBJ_TREE) { base-commit: ebdc087dc99d68bb340c282b2ba5f722def28d19
-- gitgitgadget
