| Message ID | 20240208135055.2705260-3-christian.couder@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | rev-list: allow missing tips with --missing | expand |
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. > > Let's rename this function oidset_insert_from_set() and move it into > oidset.{c,h} to make it generally available. > > While at it, let's remove a useless `!= NULL`. Makes sense. My initial reaction was that copying underlying bits may even be more efficient, but that was only because silly-me did not realize that dst can be non-empty when the operation starts. The name of the function is not "copy oidset" but "insert from set" that makes it crystal clear what is going on. Good.
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
On Tue, Feb 13, 2024 at 10:02 PM Linus Arver <linusa@google.com> wrote: > > 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. 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. > > +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. > > +/** > > + * 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()".) > /** > * 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. 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. > > +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. 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. 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.
Christian Couder <christian.couder@gmail.com> writes: > On Tue, Feb 13, 2024 at 10:02 PM Linus Arver <linusa@google.com> wrote: >> >> 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. > > 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. >> > +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. >> > +/** >> > + * 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. >> /** >> * 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. >> > +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.
On Fri, Feb 16, 2024 at 2:10 AM Linus Arver <linusa@google.com> wrote: > > Christian Couder <christian.couder@gmail.com> writes: > > > On Tue, Feb 13, 2024 at 10:02 PM Linus Arver <linusa@google.com> wrote: > >> 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. No worries. It's better to ask in case of doubt. > >> 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. I must say that in cases like this, it's difficult to be right for sure because it's mostly with enough hindsight that we can tell what turned out to be a good decision. Here for example, it might be that someone will find something more performant soon or it might turn out that the function will never change. We just can't know. So as long as the wording is clear and good enough, I think there is no point in trying to improve it as much as possible. Here both your wording and my wording seem clear and good enough to me. Junio might change his mind but so far it seems that he found my wording good enough too. So in cases like this, it's just simpler to keep current wording. This way I think there is a higher chance that things can be merged sooner and that we can all be more efficient. > >> > +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. Yeah, here also I think both "oidset_insert_from_set" and "oidset_insert_all" are clear and good enough. > 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. I agree that it could be a good thing to be consistent with similar structs, but so far for oidmap there is only oidmap_put(), and, for oid-array, only oid_array_append(). (And no, I didn't look further than this.) > 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(). Yeah, "oidset_insert_from_oidset" and perhaps "oidset_insert_all_from_oidset" would probably be fine too. Junio found my wording good enough though, so I think it's just simpler not to change it. Also it's not like it can't be improved later if there is a good reason like consistency with other oid related structs that might get oidmap_put_all() or oid_array_append_all(). But again we can't predict what will happen, so...
Christian Couder <christian.couder@gmail.com> writes: > On Fri, Feb 16, 2024 at 2:10 AM Linus Arver <linusa@google.com> wrote: >> >> Christian Couder <christian.couder@gmail.com> writes: >> >> 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. > > I must say that in cases like this, it's difficult to be right for > sure because it's mostly with enough hindsight that we can tell what > turned out to be a good decision. Here for example, it might be that > someone will find something more performant soon or it might turn out > that the function will never change. We just can't know. > > So as long as the wording is clear and good enough, I think there is > no point in trying to improve it as much as possible. Here both your > wording and my wording seem clear and good enough to me. Junio might > change his mind but so far it seems that he found my wording good > enough too. So in cases like this, it's just simpler to keep current > wording. Sounds very reasonable. > This way I think there is a higher chance that things can be > merged sooner and that we can all be more efficient. Thank you for pointing this out. There is definitely a balance between trying to find the best possible solution (which may require a much deeper analysis of the codebase, existing usage patterns, future prospects in this area, etc) and getting something that's good enough. Somehow I was under the impression that we always wanted the best possible thing during the review process (regardless of the number of rerolls), but you make a good point about "code review ergonomics", if you will. And on top of that I fully agree with all of your other comments below, so, SGTM. Thanks. >> >> > +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. > > Yeah, here also I think both "oidset_insert_from_set" and > "oidset_insert_all" are clear and good enough. > >> 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. > > I agree that it could be a good thing to be consistent with similar > structs, but so far for oidmap there is only oidmap_put(), and, for > oid-array, only oid_array_append(). (And no, I didn't look further > than this.) > >> 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(). > > Yeah, "oidset_insert_from_oidset" and perhaps > "oidset_insert_all_from_oidset" would probably be fine too. Junio > found my wording good enough though, so I think it's just simpler not > to change it. > > Also it's not like it can't be improved later if there is a good > reason like consistency with other oid related structs that might get > oidmap_put_all() or oid_array_append_all(). But again we can't predict > what will happen, so...
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))) + 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'. + */ +void oidset_insert_from_set(struct oidset *dest, struct oidset *src); + /** * Remove the oid from the set. *
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. Let's rename this function oidset_insert_from_set() and move it into oidset.{c,h} to make it generally available. While at it, let's remove a useless `!= NULL`. 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(-)