Re: [PATCH 3/7] pack-bitmap: allow aborting iteration of bitmapped objects

[Justin Tobler <jltobler@gmail.com>]

On 26/07/09 10:35AM, Patrick Steinhardt wrote:
> In a subsequent commit we'll lift iteration of bitmapped objects into
> the "packed" backend and make it accessible via `odb_for_each_object()`.
> The calling convention for that function is that the callback may return
> a non-zero exit code, and if so we'll abort iteration. This is currently
> impossible to realize though, as `for_each_bitmapped_object()` will
> ignore any return value and just churn through all objects completely.

Ok.

> This doesn't matter to the callers of `for_each_bitmapped_object()`, as
> there's only one of them in git-cat-file(1), and the callbacks we pass
> always return zero. But once we move the logic into the generic
> infrastructure it becomes a latent bug waiting to happen.
> 
> Refactor the code so that the return value of the `show_reach` callback
> is not ignored anymore. Instead, returning a non-zero value will cause
> us to abort iteration in both `show_objects_for_type()` and in
> `for_each_bitmapped_object()`.

Make sense. We want to ensure that the `show_reach` callback can
properly signal back to `for_each_bitmapped_object()` to abort.

> Note though that there's a second user of `show_objects_for_type()` with
> `traverse_bitmap_commit_list()`, and that function does indeed invoke
> callbacks that may return non-zero. This non-zero return value never had
> any effect at all though, and the callbacks that return non-zero values
> are only ever invoked via `traverse_bitmap_commit_list()`. Consequently,
> we adapt them to always return 0.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/pack-objects.c |  2 +-
>  builtin/rev-list.c     |  2 +-
>  pack-bitmap.c          | 31 +++++++++++++++++++++----------
>  pack-bitmap.h          |  3 ++-
>  4 files changed, 25 insertions(+), 13 deletions(-)
> 
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index ea5eab4cf8..8ff92c5272 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -1909,7 +1909,7 @@ static int add_object_entry_from_bitmap(const struct object_id *oid,
>  		return 0;
>  
>  	create_object_entry(oid, type, name_hash, 0, 0, pack, offset);
> -	return 1;
> +	return 0;

I wonder why this was even returning 1 to begin with? As you mentioned,
the return value appears to be ignored anyways. I'm assuming it was
signal that an object entry was created?

>  }
>  
>  struct pbase_tree_cache {
> diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> index 8f63003709..02818b81c6 100644
> --- a/builtin/rev-list.c
> +++ b/builtin/rev-list.c
> @@ -486,7 +486,7 @@ static int show_object_fast(
>  	void *payload UNUSED)
>  {
>  	fprintf(stdout, "%s\n", oid_to_hex(oid));
> -	return 1;
> +	return 0;

Also curious about this one too. It probably doesn't matter though.

>  }
>  
>  static void print_disk_usage(off_t size)
> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index a47c231632..eda38a5433 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -1695,7 +1695,7 @@ static void init_type_iterator(struct ewah_or_iterator *it,
>  	}
>  }
>  
> -static void show_objects_for_type(
> +static int show_objects_for_type(
>  	struct bitmap_index *bitmap_git,
>  	struct bitmap *objects,
>  	enum object_type object_type,
> @@ -1704,6 +1704,7 @@ static void show_objects_for_type(
>  {
>  	size_t i = 0;
>  	uint32_t offset;
> +	int ret;
>  
>  	struct ewah_or_iterator it;
>  	eword_t filter;
> @@ -1749,11 +1750,17 @@ static void show_objects_for_type(
>  
>  			hash = bitmap_name_hash(bitmap_git, index_pos);
>  
> -			show_reach(&oid, object_type, 0, hash, pack, ofs, payload);
> +			ret = show_reach(&oid, object_type, 0, hash, pack, ofs, payload);
> +			if (ret)
> +				goto out;

The show_reach callback now wires back its return code.

>  		}
>  	}
>  
> +	ret = 0;
> +
> +out:
>  	ewah_or_iterator_release(&it);
> +	return ret;
>  }
>  
>  static int in_bitmapped_pack(struct bitmap_index *bitmap_git,
> @@ -2062,6 +2069,12 @@ int for_each_bitmapped_object(struct bitmap_index *bitmap_git,
>  			      show_reachable_fn show_reach,
>  			      void *payload)
>  {
> +	const enum object_type types[] = {
> +		OBJ_COMMIT,
> +		OBJ_TREE,
> +		OBJ_BLOB,
> +		OBJ_TAG,
> +	};
>  	struct bitmap *filtered_bitmap = NULL;
>  	uint32_t objects_nr;
>  	size_t full_word_count;
> @@ -2086,14 +2099,12 @@ int for_each_bitmapped_object(struct bitmap_index *bitmap_git,
>  		goto out;
>  	}
>  
> -	show_objects_for_type(bitmap_git, filtered_bitmap,
> -			      OBJ_COMMIT, show_reach, payload);
> -	show_objects_for_type(bitmap_git, filtered_bitmap,
> -			      OBJ_TREE, show_reach, payload);
> -	show_objects_for_type(bitmap_git, filtered_bitmap,
> -			      OBJ_BLOB, show_reach, payload);
> -	show_objects_for_type(bitmap_git, filtered_bitmap,
> -			      OBJ_TAG, show_reach, payload);
> +	for (size_t i = 0; i < ARRAY_SIZE(types); i++) {
> +		ret = show_objects_for_type(bitmap_git, filtered_bitmap,
> +					    types[i], show_reach, payload);
> +		if (ret)
> +			goto out;
> +	}

`for_each_bitmapped_object()` now has access to the underlying return
code and can abort. Looks good.

-Justin