Re: [PATCH 4/5] odb: remove read loop for found objects

[Justin Tobler <jltobler@gmail.com>]

On 26/10/13 09:36AM, Patrick Steinhardt wrote:
> Before moving alternates into the "files" backend we had to handle
> alternates in `do_oid_object_info_extended()`. This made the retry logic
> somewhat complex, as we had to juggle reloading of the sources, the
> second read and fetching missing objects. And to handle the potential
> retries we loop around that logic multiple times.
> 
> Nowadays though, the reload and retry logic are located in the "files"
> backend itself. Consequently, using a loop is a bit on the heavy side
> and makes the code harder to reason about.

The "files" backend is now responsible for retrying a read if it need to
reload its packfiles. IIUC, this means that we should only expect
missing objects from partial clones. Only if we want to fetch these
missing objects from the promisor remote do we need to retry.

> Simplify the logic by removing the loop. Instead, we explicitly re-read
> exactly once after having fetched objects from a promisor remote.

Make sense.

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  odb.c | 46 +++++++++++++++++++++++-----------------------
>  1 file changed, 23 insertions(+), 23 deletions(-)
> 
> diff --git a/odb.c b/odb.c
> index 69a4c203bc..095183aec0 100644
> --- a/odb.c
> +++ b/odb.c
> @@ -265,7 +265,6 @@ static enum odb_read_status do_oid_object_info_extended(struct object_database *
>  	struct strbuf corrupt_err = STRBUF_INIT;
>  	const struct object_id *real = oid;
>  	enum odb_read_status ret;
> -	int already_retried = 0;
>  	bool corrupt = false;
>  
>  	if (flags & OBJECT_INFO_LOOKUP_REPLACE)
> @@ -277,38 +276,39 @@ static enum odb_read_status do_oid_object_info_extended(struct object_database *
>  	if (!odb_source_read_object_info(odb->inmemory_objects, oid, oi, flags, NULL))
>  		return 0;
>  
> -	while (1) {
> +	ret = odb_source_read_object_info(odb->source, real, oi, flags,
> +					  &corrupt_err);

Here is try to read the object, the backend is responsible for handling
any retries internally that it cares about.

> +	if (!ret)
> +		goto out;
> +	if (ret != ODB_READ_NOT_FOUND)
> +		corrupt = true;

If the repsitory is correct, does it even make sense to continue to try
and fetch the missing object? I wonder if we should set `ret =
ODB_READ_ERROR` and `goto out`?

> +
> +	/* Try to fetch the missing object from a promisor remote. */
> +	if (odb->repo->fetch_if_missing && repo_has_promisor_remote(odb->repo) &&
> +	    !(flags & OBJECT_INFO_SKIP_FETCH_OBJECT)) {
> +		promisor_remote_get_direct(odb->repo, real, 1);
> +
>  		ret = odb_source_read_object_info(odb->source, real, oi, flags,
>  						  corrupt_err.len ? NULL : &corrupt_err);

Here we read the missing object after it has been fetched. This is only
case the ODB layer needs to care about retrying itself. Makes sense.

The rest of this patch is functionally the same just without the loop.
Looks good.

-Justin