Re: [PATCH 1/7] submodule: lazily add submodule ODBs as alternates

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

Jonathan Tan <jonathantanmy@google.com> writes:

> @@ -1592,6 +1593,10 @@ static int do_oid_object_info_extended(struct repository *r,
>  				break;
>  		}
>  
> +		if (register_all_submodule_odb_as_alternates())
> +			/* We added some alternates; retry */
> +			continue;
> +

OK.  Unless we are running in the "we no longer are relying on the
submodule-odb-as-alternates hack" mode, the control may reach this
point number of times, so this caller expects the function to return
how many new odbs were added with this single invocation.

> diff --git a/submodule.c b/submodule.c
> index 8e611fe1db..8fde90e906 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -165,6 +165,8 @@ void stage_updated_gitmodules(struct index_state *istate)
>  		die(_("staging updated .gitmodules failed"));
>  }
>  
> +static struct string_list added_submodule_odb_paths = STRING_LIST_INIT_NODUP;
> +

I see 2/7 allows callers to add paths for submodule odb to this
list.

> @@ -178,12 +180,28 @@ int add_submodule_odb(const char *path)
>  		ret = -1;
>  		goto done;
>  	}
> -	add_to_alternates_memory(objects_directory.buf);
> +	string_list_insert(&added_submodule_odb_paths,
> +			   strbuf_detach(&objects_directory, NULL));

OK.  By default, any codepath that still call add_submodule_odb()
will add the paths to submodules to the list (so that they will
lazily be loaded).

> +int register_all_submodule_odb_as_alternates(void)
> +{
> +	int i;
> +	int ret = added_submodule_odb_paths.nr;
> +
> +	for (i = 0; i < added_submodule_odb_paths.nr; i++)
> +		add_to_alternates_memory(added_submodule_odb_paths.items[i].string);
> +	if (ret) {
> +		string_list_clear(&added_submodule_odb_paths, 0);
> +		if (git_env_bool("GIT_TEST_FATAL_REGISTER_SUBMODULE_ODB", 0))
> +			BUG("register_all_submodule_odb_as_alternates() called");
> +	}
> +	return ret;
> +}

OK.  We add new ones that were added to the list since we were
called for the last time and clear the list.  When we didn't add
anything, we will return 0.  Seems to mean well.

I wonder if we need to prepare ourselves to catch callers that call
add_submodule_odb() to the same path twice or more.  Probably not.

Thanks.

The "pretend as if objects in submodule odb are locally available",
even though it may have been useful, was an ugly hack invented
before we started adding the "access more than one repository at the
time with the 'repo' arg" extended API.  It is pleasing to see this
step shows an approach to incrementally migrate the users of the
hack.