rev-list: ignore replacements in the connectivity check
PATCH 1/1 rev-list: ignore replacements in the connectivity check
From: Kristofer Karlsson <[email protected]>
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.
---
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/[email protected]/
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(-)
Documentation/rev-list-options.adoc +1 -0
@@ -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)]`::
builtin/rev-list.c +3 -0
@@ -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" )) {
t/t5413-connectivity-check.sh +1 -10
@@ -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
tree-verify.c +0 -2
@@ -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
Signature · 1 line
gitgitgadget
"Kristofer Karlsson via GitGitGadget" <[email protected]>
Quoted 11 lines · writes:
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, andcheck_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.
Sounds like absolutely the right thing to do. We might want to say that we check both the original and the replacement, i.e., instead of ignoring replace refs, we ensure that the commits, trees, and blobs reachable from the replacement objects all exist by treating those objects as if they were also at the tips of the connectivity graph.
Quoted 8 lines
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.
...
Quoted 3 lines
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.
Good.
Quoted 2 lines
Shallow boundaries are not affected, since they use the graft mechanism internally, not replace refs.
In other words, they do not introduce more links that need to be checked. They just declare "behind this commit you do not have to check as nobody is allowed to traverse past it".
Quoted 25 lines
Documentation/rev-list-options.adoc +1
-0@@ -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)]`::builtin/rev-list.c +1
-0@@ -30,6 +30,7 @@#include "strbuf.h" #include "tree-verify.h" #include "parse.h" +#include "replace-object.h" #define INCREMENTAL_MAX_COMMITS 1000
builtin/rev-list.c int cmd_rev_list(int argc, · line 812
Quoted 5 lines
builtin/rev-list.c +2
-0@@ -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 ();
OK. This is straightforward "we want to verify the underlying, real objects".
Quoted 17 lines
builtin/rev-list.c +0
-0}else if (skip_prefix (arg,"--missing=" , &arg)) {parse_missing_action_value (repo, arg); }else if (!strcmp (arg,"--missing-only" )) {tree-verify.c +0
-1@@ -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))
tree-verify.c static int read_tree_object(struct object_database *odb, · line 167
Quoted 7 lines
tree-verify.c +0
-1@@ -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 ) {
So are these two other hunks.
We can address the "shouldn't we make sure that replacement objects used when the resulting history is in use are backed by objects reachable from them?" issue separately.
Thanks.
Quoted 15 lines · Junio C Hamano
"Kristofer Karlsson via GitGitGadget" <[email protected]>
Quoted 3 lines · writes:
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.
Sounds like absolutely the right thing to do. We might want to say that we check both the original and the replacement, i.e., instead of ignoring replace refs, we ensure that the commits, trees, and blobs reachable from the replacement objects all exist by treating those objects as if they were also at the tips of the connectivity graph.
Quoted 4 lines
[snip]
We can address the "shouldn't we make sure that replacement objects used when the resulting history is in use are backed by objects reachable from them?" issue separately.
I have not fully thought this through yet, but I'm happy to follow up with further strengthening if there's a gap here.
My preliminary thinking is that since replacements are implemented as regular refs (refs/replace/OLD -> NEW), their reachable objects should already be covered by ordinary connectivity checks when those refs are fetched or pushed.
And thanks for the review!
Kristofer
Documentation/rev-list-options.adoc It is an error to use `--missing-only` without `--missing=print` or · line 1113
Quoted 12 lines · Kristofer Karlsson via GitGitGadget
Documentation/rev-list-options.adoc +1
-0@@ -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)]`::
Nit: Should we also modify 'Documentation/git-replace.adoc', which says:
Replacement references will be used by default by all Git commands except those doing reachability traversal (prune, pack transfer and fsck).
Quoted 16 lines · Kristofer Karlsson
Quoted 15 lines · Junio C Hamano
"Kristofer Karlsson via GitGitGadget" <[email protected]>
Quoted 3 lines · writes:
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.
Sounds like absolutely the right thing to do. We might want to say that we check both the original and the replacement, i.e., instead of ignoring replace refs, we ensure that the commits, trees, and blobs reachable from the replacement objects all exist by treating those objects as if they were also at the tips of the connectivity graph.
Quoted 4 lines
Quoted 4 lines
[snip]
We can address the "shouldn't we make sure that replacement objects used when the resulting history is in use are backed by objects reachable from them?" issue separately.
Yeah, I was wondering whether we shouldn't verify both original and replacement object. Previously we checked the latter, now we check the former. But is either of those variants the correct thing to do?
Quoted 8 lines
I have not fully thought this through yet, but I'm happy to follow up with further strengthening if there's a gap here.
My preliminary thinking is that since replacements are implemented as regular refs (refs/replace/OLD -> NEW), their reachable objects should already be covered by ordinary connectivity checks when those refs are fetched or pushed.
That's fair, I guess. The question though is whether we can rely on that property when the replace ref was already written locally. But I'm not sure that we even _should_ care about that. It's arguably the responsibility of the user that wrote the replace ref to make sure that the new object is valid. And in the context of the connectivity check done as part of a push we always assume that existing refs are fully connected anyway.
So overall I'm aligned with this change.
Patrick
On Mon, Nov 02, 2026 at 03:47:09PM +0000, Kristofer Karlsson via GitGitGadget wrote:
[snip]
Quoted 4 lines
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.
This here makes me pause a bit. Do we know whether this specific test was introduced intentionally to make a specific edge case work?
Quoted 13 lines
tree-verify.c +0
-1@@ -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))
tree-verify.c static int read_tree_object(struct object_database *odb, · line 167
Quoted 7 lines
tree-verify.c +0
-1@@ -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 ) {
Nit: this might have warranted a separate commit as it isn't even related to the connectivity check, is it? Did you also vet all the other sites that that verify objects?
Patrick
Quoted 10 lines · Patrick Steinhardt
On Mon, Nov 02, 2026 at 03:47:09PM +0000, Kristofer Karlsson via GitGitGadget wrote:
[snip]Quoted 4 lines
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.
This here makes me pause a bit. Do we know whether this specific test was introduced intentionally to make a specific edge case work?
Both the t5413 test and tree-verify.c were introduced as part of my (recent) incremental connectivity check series. I noticed the replacement "oddity" back then, but deliberately kept the existing behavior -- changing the semantics should be separate and intentional.
Quoted 17 lines
Quoted 17 lines
tree-verify.c +0
-1@@ -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,
Quoted 8 lines
tree-verify.c static int read_tree_object(struct object_database *odb, · line 167
Quoted 4 lines
tree-verify.c +0
-1-OBJECT_INFO_LOOKUP_REPLACE | OBJECT_INFO_SKIP_FETCH_OBJECT)) *bufp =NULL ;if (*bufp && type !=OBJ_TREE ) {Nit: this might have warranted a separate commit as it isn't even related to the connectivity check, is it? Did you also vet all the other sites that that verify objects?
tree-verify.c is only used by rev-list --connectivity-check=incremental, so the flag removal is part of the same change: without it, the incremental check would still nominally ask for replacements while the process has them disabled. The behavior would be the same, but it would look confusing (why ask for replacements if they are always globally disabled when this path is reached?)
This patch is intentionally limited to the connectivity check. I did review the other users of OBJECT_INFO_LOOKUP_REPLACE, but none of them seem related to it. Longer term it would be nice if each caller stated whether it wants replacements, rather than relying on the global switch, but that is a much larger change.
One of them does look like a separate issue, though: mktree checks that each entry exists, following replacements, but writes the original oid into the tree, so a missing object can pass the check without --missing if it has a replace ref. That could be looked at separately as a bugfix maybe? I have not looked into it more though.
Thanks,
Kristofer
Quoted 6 lines · Karthik Nayak
Nit: Should we also modify '
Documentation/git-replace.adoc', which says:Replacement references will be used by default by all Git commands except those doing reachability traversal (prune, pack transfer and fsck).
I think that sentence is actually more accurate with this patch than without it. The connectivity check runs as part of pack transfer (fetch and push), so until now that part of "pack transfer" did follow replacements, contrary to what the documentation says.
The list was never exhaustive either: index-pack, unpack-objects, commit-graph and multi-pack-index also disable replacements, so we can see it as a few examples of internal operations rather than a complete list.
Thanks,
Kristofer
Quoted 5 lines · Kristofer Karlsson
My preliminary thinking is that since replacements are implemented as regular refs (refs/replace/OLD -> NEW), their reachable objects should already be covered by ordinary connectivity checks when those refs are fetched or pushed.
You are right. So no need to do anything extra to worry about "doing both". It just "ignore replacements" had a wrong connotation that we ignore the replacing objects and only pay attention to the connection among underlying objects that misled me.
Thanks.