Re: [PATCH v8 13/15] bugreport: add packed object summary

[Emily Shaffer <emilyshaffer@google.com>]

On Thu, Feb 20, 2020 at 02:04:52PM -0800, Junio C Hamano wrote:
> Emily Shaffer <emilyshaffer@google.com> 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@gitster-ct.c.googlers.com

> 
> >  #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) &&