Re: [PATCH v4 5/6] connected: add incremental connectivity check via rev-list

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

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

> -static void parse_connectivity_check(struct repository *repo UNUSED,
> +static void parse_connectivity_check(struct repository *repo,
>  				     const char *arg,
>  				     struct connectivity_check *check)
>  {
>  	if (!strcmp(arg, "full"))
>  		check->mode = CONNECTIVITY_CHECK_FULL;
> +	else if (!strcmp(arg, "incremental"))
> +		check->mode = CONNECTIVITY_CHECK_INCREMENTAL;
>  	else
>  		die(_("unknown --connectivity-check mode '%s'"), arg);
> +
> +	if (check->mode != CONNECTIVITY_CHECK_FULL)
> +		repo->fetch_if_missing = 0;
>  }

This assumes that everybody that is different from full want to
avoid lazy fetching.  It may be true right now as you have only full
and incremental, but it somehow feels like a trap waiting to bite
future developers.

> @@ -992,6 +999,29 @@ int cmd_rev_list(int argc,
>  
>  	prepare_maximal_independent(&revs);
>  
> +	/*
> +	 * Without a boundary of negative revisions, as in a clone or a
> +	 * push into an empty repository, every incoming commit is new.
> +	 * The incremental check would still work, but it would verify
> +	 * the whole object graph like the full check, with extra
> +	 * per-commit overhead on top, so use the full check.
> +	 */
> +	if (!revs.limited)
> +		connectivity_check.mode = CONNECTIVITY_CHECK_FULL;
> +
> +	if (connectivity_check.mode == CONNECTIVITY_CHECK_INCREMENTAL) {
> +		struct commit *commit;
> +		struct commit_list *new_commits = NULL;
> +
> +		while ((commit = get_revision(&revs)) != NULL)
> +			commit_list_insert(commit, &new_commits);
> +
> +		if (verify_commits_incremental(repo, &new_commits,
> +					       revs.exclude_promisor_objects))
> +			die(_("connectivity check failed"));
> +		commit_list_free(new_commits);
> +	}
> +

We only call verify_commits_incremental() when check.mode says INCREMENTAL,
which makes sense.

> +static int verify_blob(struct repository *repo,
> +		       const struct object_id *oid,
> +		       struct verify_state *vs)
> +{
> +	int type;
> +
> +	if (oidset_contains(&vs->trusted_blobs, oid))
> +		return 0;
> +
> +	vs->blobs_checked++;
> +	type = odb_read_object_info(repo->objects, oid, NULL);

I wonder if this needs to be _extended() kind that passes a flag bit
to avoid fetching from promisors.  repo->fetch_if_missing that is
set does prevent lazy fetching, but I wonder if it is better to be
more explicit.

> +int verify_commits_incremental(struct repository *repo,
> +			       struct commit_list **commits,
> +			       int exclude_promisor_objects)
> +{
> +	struct verify_state vs = { 0 };
> +	struct commit_list *iter;
> +	unsigned nr_before;
> +	int ret = 0;
> +
> +	if (repo->fetch_if_missing)
> +		BUG("verify_commits_incremental must not be called "
> +		    "with fetch_if_missing set");

And we make sure that the caller flipped the bit before they decide
to call us, which makes sense.