On Thu, Feb 20, 2020 at 02:04:52PM -0800, Junio C Hamano wrote:
> Emily Shaffer <emilyshaffer@xxxxxxxxxx> writes:
>
> > +static void get_packed_object_summary(struct strbuf *obj_info, int nongit)
> > +{
> > + struct packed_git *pack = NULL;
> > + int pack_count = 0;
> > + int object_count = 0;
> > +
> > + if (nongit) {
> > + strbuf_addstr(obj_info,
> > + "not run from a git repository - no objects to show\n");
> > + return;
> > + }
> > +
> > + for_each_pack(the_repository, pack) {
> > + pack_count++;
> > + /*
> > + * To accurately count how many objects are packed, look inside
> > + * the packfile's index.
> > + */
> > + open_pack_index(pack);
> > + object_count += pack->num_objects;
> > + }
> > +
> > + strbuf_addf(obj_info, "%d total packs (%d objects)\n", pack_count,
> > + object_count);
> > +
> > +}
>
> Makes sense.
>
> > @@ -447,4 +448,9 @@ int for_each_object_in_pack(struct packed_git *p,
> > int for_each_packed_object(each_packed_object_fn, void *,
> > enum for_each_object_flags flags);
> >
> > +#define for_each_pack(repo, pack) \
> > + for (pack = get_all_packs(repo);\
> > + pack; \
> > + pack = pack->next)
>
> I generally avoid #define'ing a control loop pseudo-syntax unless it
> makes the resulting code hide overly ugly implementation detail.
>
> for_each_string_list() is there to hide the fact that items are
> stored in an embedded array whose name is .items and size is .nr
> that is sufficiently ugnly to expose, but iterating over packs
> does not look so bad.
>
> If you MUST have this as a pseudo-syntax macro, we need
>
> - to match for_each_string_list_item(), have iterating variable
> 'pack' as the first parameter, and the scope of what's iterated
> 'repo' as the second.
>
> - to make sure the syntax works correctly even if a parameter is
> *not* a simple identifier (I think the above is OK, but there may
> be cases that it does not work well).
>
> Regarding the latter, the way 'item' is incremented at the end of
> iteration in for_each_string_list_item() is subtle and correct.
>
> #define for_each_string_list_item(item,list) \
> for (item = (list)->items; \
> item && item < (list)->items + (list)->nr; \
> ++item)
>
> You would break it if you changed pre-increment to post-increment
> for a user like this:
>
> struct string_list *list;
> struct string_list_item *i, **p;
> p = &i;
>
> for_each_string_list_item(*p, list) {
> ...
> }
>
> because ++*p is ++(*p), while *p++ is (*p)++, and we do want the
> former (i.e. increment the memory cell pointed at by pointer p).
>
> Personally, I would prefer not to introduce this macro if I were
> working on this topic.
Ah, I thought this is the kind of thing you meant here[1]. But I agree
with what you point out about the shortcomings of this kind of macro.
The implementation details are not so ugly; I'll drop it.
- Emily
[1] https://lore.kernel.org/git/xmqq8sli89eu.fsf@xxxxxxxxxxxxxxxxxxxxxxxxx
>
> > #endif /* OBJECT_STORE_H */
> > diff --git a/packfile.c b/packfile.c
> > index 99dd1a7d09..95afcc1187 100644
> > --- a/packfile.c
> > +++ b/packfile.c
> > @@ -2095,8 +2095,7 @@ int for_each_packed_object(each_packed_object_fn cb, void *data,
> > int r = 0;
> > int pack_errors = 0;
> >
> > - prepare_packed_git(the_repository);
> > - for (p = get_all_packs(the_repository); p; p = p->next) {
> > + for_each_pack(the_repository, p) {
> > if ((flags & FOR_EACH_OBJECT_LOCAL_ONLY) && !p->pack_local)
> > continue;
> > if ((flags & FOR_EACH_OBJECT_PROMISOR_ONLY) &&
