Christian Couder [off-list ref] writes:

On Tue, Feb 13, 2024 at 10:02 PM Linus Arver [off-list ref] wrote:

quoted

Christian Couder [off-list ref] writes:

quoted

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.

quoted

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.

Yeah, ideally there should be unit tests for oidset and all its
features, but it seems to me that there aren't any. Also oidset is
based on khash.h which was originally imported from
https://github.com/attractivechaos/klib/ without tests. So I think
it's a different topic to add tests from scratch to oidset, khash.h or
both.

Actually after taking another look, it looks like khash.h or some of
its features are tested through "helper/test-oidmap.c" and
"t0016-oidmap.sh". I still think it's another topic to test oidset.

Agreed.

quoted

quoted

+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?

Yes. Without them gcc errors out with:

oidset.c: In function ‘oidset_insert_from_set’:
oidset.c:32:16: error: suggest parentheses around assignment used as
truth value [-Werror=parentheses]
  32 |         while (src_oid = oidset_iter_next(&iter))
     |                ^~~~~~~

Having extra parentheses is a way to tell the compiler that we do want
to use '=' and not '=='. This helps avoid the very common mistake of
using '=' where '==' was intended.

Ah, so it is a "please trust me gcc, I know what I am doing" thing and
not a "this is required in C" thing. Makes sense, thanks for clarifying.

Sorry for the noise.

quoted

quoted

+/**
+ * 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.

(It's "oidset_insert()" not "oid_insert()".)

Oops, yes, sorry for the typo.

quoted

    /**
     * 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.

(What you suggest would need s/oid_insert/oidset_insert/)

Yeah, it's a bit simpler and shorter, but on the other hand a reader
might have to read both this and the oidset_insert() doc, so in the
end I am not sure it's a big win for readability. And if they don't
read the oidset_insert() doc, they might miss the fact that we are
copying the oids we insert, which might result in a bug.

When functions are built on top of other functions, I think it is good
practice to point readers to those underlying functions. In this case
the new function is a wrapper around oidset_insert() which does all the
real work. Plus the helper function already has some documentation about
copying behavior that we already thought was important enough to call
out explicitly.

So, tying this definition to that (foundational) helper function sounds
like a good idea to me in terms of readability. IOW we can inform
readers "hey, we're just a wrapper around this other important function

--- go there if you're curious about internals" and emphasizing that

sort of relationship which may not be immediately obvious to those not
familiar with this area would be nice.

Alternatively, we could repeat the same comment WRT copying here but
that seems redundant and prone to maintenance burdens down the road (if
we ever change this behavior we have to change the comment in multiple
functions, possibly).

Also your wording ties the implementation with oidset_insert(), which
we might not want if we could find something more performant. See
Junio's comment on this patch saying his initial reaction was that
copying underlying bits may even be more efficient.

So I prefer not to change this.

OK.

quoted

quoted

+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.

We already have other related types like 'struct oid-array' and
'struct oidmap' to store oids, as well as code that inserts many oids
into an oidset from a 'struct ref *' linked list or array in a tight
loop.

Thank you for the additional context I was not aware of.

So if we want to add functions inserting all the oids from
instances of such types, how should we call them?

I would say we should use suffixes like: "_from_set", "_from_map",
"from_array", "_from_ref_list", "_from_ref_array", etc.

I agree.

However, I would like to point out that the function being added in this
patch is a bit special: it is inserting from one "oidset" into another
"oidset". IOW the both the dest and src types are the same.

For the cases where the types are different, I totally agree that using
the suffixes (to encode the type information of the src into the
function name itself) is a good idea.

So I think it's still fine to use "oidset_insert_all" because the only
type in the parameter list is an oidset.

BUT, maybe in our codebase we already use suffixes like this even for
cases where the types are the same? I don't know the answer to this
question. However if we really wanted to be consistent then maybe we
should be using the name oidset_insert_from_oidset() and not
oidset_insert_from_set().

If we start using just "_all" for oidset, then what should we use for
the other types? I don't see a good answer to that, so I prefer to
stick with "_from_set" for oidset.

Thanks.