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

[Junio C Hamano <gitster@pobox.com>]

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