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

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

[Kristofer Karlsson via GitGitGadget]

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(-)

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

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

[Junio C Hamano]

"Kristofer Karlsson via GitGitGadget" <gitgitgadget@gmail.com>
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, 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.

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.

> 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.

...

>     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.

>     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".

> 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();

OK.  This is straightforward "we want to verify the underlying, real
objects".

>  		} else if (skip_prefix(arg, "--missing=", &arg)) {
>  			parse_missing_action_value(repo, arg);
>  		} else if (!strcmp(arg, "--missing-only")) {

> 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) {

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.

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

[Kristofer Karlsson]

On Mon, 2 Nov 2026 at 23:05, Junio C Hamano <gitster@pobox.com> wrote:
>
> "Kristofer Karlsson via GitGitGadget" <gitgitgadget@gmail.com>
> 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.
>
> [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

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

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 904 bytes --]

"Kristofer Karlsson via GitGitGadget" <gitgitgadget@gmail.com> writes:

> 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)]`::

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).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

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

[Patrick Steinhardt]

On Tue, Nov 03, 2026 at 09:35:07AM +0100, Kristofer Karlsson wrote:
> On Mon, 2 Nov 2026 at 23:05, Junio C Hamano <gitster@pobox.com> wrote:
> >
> > "Kristofer Karlsson via GitGitGadget" <gitgitgadget@gmail.com>
> > 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.
> >
> > [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?

> 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

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

[Patrick Steinhardt]

On Mon, Nov 02, 2026 at 03:47:09PM +0000, Kristofer Karlsson via GitGitGadget wrote:
[snip]
> 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?

> 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) {

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

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

[Kristofer Karlsson]

On Tue, 3 Nov 2026 at 11:17, Patrick Steinhardt <ps@pks.im> wrote:
>
> On Mon, Nov 02, 2026 at 03:47:09PM +0000, Kristofer Karlsson via GitGitGadget wrote:
> [snip]
> > 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.

>
> > 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) {
>
> 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

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

[Kristofer Karlsson]

On Tue, 3 Nov 2026 at 10:54, Karthik Nayak <karthik.188@gmail.com> wrote:
>
> 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

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

[Junio C Hamano]

Kristofer Karlsson <krka@spotify.com> writes:

> 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.