Re: [PATCH v2 2/4] oidset: refactor oidset_insert_from_set()
- From
- Linus Arver <linusa@google.com>
- Date
- Feb 13, 2024, 21:02 UTC
- Message-ID
- <owlyle7o9iyf.fsf@fine.c.googlers.com>
- In-Reply-To
- <20240208135055.2705260-3-christian.couder@gmail.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.
Show 51 quoted lines
> 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?
Show 18 quoted lines
> + 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.
Show 6 quoted lines
> + > /** > * Remove the oid from the set. > * > -- > 2.43.0.565.g97b5fd12a3.dirty