Re: [PATCH v2 2/4] oidset: refactor oidset_insert_from_set()

[Linus Arver <linusa@google.com>]

Christian Couder <christian.couder@gmail.com> writes:

> In a following commit, we will need to add all the oids from a set into
> another set. In "list-objects-filter.c", there is already a static
> function called add_all() to do that.

Nice find.

> Let's rename this function oidset_insert_from_set() and move it into
> oidset.{c,h} to make it generally available.

At some point (I don't ask for it in this series) we should add unit
tests for this newly-exposed function. Presumably the stuff around
object/oid handling is stable enough to receive unit tests.

> While at it, let's remove a useless `!= NULL`.

Nice cleanup. It would have been fine to also put this in a separate
patch, but as it is so simple I think it's also fine to keep it mixed in
with the move as you did here.

> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  list-objects-filter.c | 11 +----------
>  oidset.c              | 10 ++++++++++
>  oidset.h              |  6 ++++++
>  3 files changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/list-objects-filter.c b/list-objects-filter.c
> index da287cc8e0..4346f8da45 100644
> --- a/list-objects-filter.c
> +++ b/list-objects-filter.c
> @@ -711,15 +711,6 @@ static void filter_combine__free(void *filter_data)
>  	free(d);
>  }
>  
> -static void add_all(struct oidset *dest, struct oidset *src) {
> -	struct oidset_iter iter;
> -	struct object_id *src_oid;
> -
> -	oidset_iter_init(src, &iter);
> -	while ((src_oid = oidset_iter_next(&iter)) != NULL)
> -		oidset_insert(dest, src_oid);
> -}
> -
>  static void filter_combine__finalize_omits(
>  	struct oidset *omits,
>  	void *filter_data)
> @@ -728,7 +719,7 @@ static void filter_combine__finalize_omits(
>  	size_t sub;
>  
>  	for (sub = 0; sub < d->nr; sub++) {
> -		add_all(omits, &d->sub[sub].omits);
> +		oidset_insert_from_set(omits, &d->sub[sub].omits);
>  		oidset_clear(&d->sub[sub].omits);
>  	}
>  }
> diff --git a/oidset.c b/oidset.c
> index d1e5376316..91d1385910 100644
> --- a/oidset.c
> +++ b/oidset.c
> @@ -23,6 +23,16 @@ int oidset_insert(struct oidset *set, const struct object_id *oid)
>  	return !added;
>  }
>  
> +void oidset_insert_from_set(struct oidset *dest, struct oidset *src)
> +{
> +	struct oidset_iter iter;
> +	struct object_id *src_oid;
> +
> +	oidset_iter_init(src, &iter);
> +	while ((src_oid = oidset_iter_next(&iter)))

Are the extra parentheses necessary?

> +		oidset_insert(dest, src_oid);
> +}
> +
>  int oidset_remove(struct oidset *set, const struct object_id *oid)
>  {
>  	khiter_t pos = kh_get_oid_set(&set->set, *oid);
> diff --git a/oidset.h b/oidset.h
> index ba4a5a2cd3..262f4256d6 100644
> --- a/oidset.h
> +++ b/oidset.h
> @@ -47,6 +47,12 @@ int oidset_contains(const struct oidset *set, const struct object_id *oid);
>   */
>  int oidset_insert(struct oidset *set, const struct object_id *oid);
>  
> +/**
> + * Insert all the oids that are in set 'src' into set 'dest'; a copy
> + * is made of each oid inserted into set 'dest'.
> + */

Just above in oid_insert() there is already a comment about needing to
copy each oid.

    /**
     * Insert the oid into the set; a copy is made, so "oid" does not need
     * to persist after this function is called.
     *
     * Returns 1 if the oid was already in the set, 0 otherwise. This can be used
     * to perform an efficient check-and-add.
     */

so perhaps the following wording is simpler?

    Like oid_insert(), but insert all oids found in 'src'. Calls
    oid_insert() internally.

> +void oidset_insert_from_set(struct oidset *dest, struct oidset *src);

Perhaps "oidset_insert_all" would be a simpler name? I generally prefer
to reuse any descriptors in comments to guide the names. Plus this
function used to be called "add_all()" so keeping the "all" naming style
feels right.

> +
>  /**
>   * Remove the oid from the set.
>   *
> -- 
> 2.43.0.565.g97b5fd12a3.dirty