Subject: Re: [PATCH v8 12/15] bugreport: count loose objects

From: Emily Shaffer <[email-masked]>
Date: Tue, 25 Feb 2020 15:26:26 -0800

On Tue, Feb 25, 2020 at 03:22:00PM -0800, Emily Shaffer wrote:
> On Thu, Feb 20, 2020 at 01:04:33PM -0800, Junio C Hamano wrote:
> > Emily Shaffer <[email-masked]> writes:
> > 
> > > The number of unpacked objects in a user's repository may help us
> > > understand the root of the problem they're seeing, especially if a
> > > command is running unusually slowly.
> > >
> > > Helped-by: Johannes Schindelin <[email-masked]>
> > > Signed-off-by: Emily Shaffer <[email-masked]>
> > > ---
> > >  Documentation/git-bugreport.txt |  1 +
> > >  bugreport.c                     | 52 +++++++++++++++++++++++++++++++++
> > >  2 files changed, 53 insertions(+)
> > >
> > > diff --git a/Documentation/git-bugreport.txt b/Documentation/git-bugreport.txt
> > > index 4d01528540..4fe1c60506 100644
> > > --- a/Documentation/git-bugreport.txt
> > > +++ b/Documentation/git-bugreport.txt
> > > @@ -32,6 +32,7 @@ The following information is captured automatically:
> > >   - $SHELL
> > >   - Selected config values
> > >   - A list of enabled hooks
> > > + - The number of loose objects in the repository
> > >  
> > >  This tool is invoked via the typical Git setup process, which means that in some
> > >  cases, it might not be able to launch - for example, if a relevant config file
> > > diff --git a/bugreport.c b/bugreport.c
> > > index b5a0714a7f..fb7bc72723 100644
> > > --- a/bugreport.c
> > > +++ b/bugreport.c
> > > @@ -10,6 +10,7 @@
> > >  #include "bugreport-config-safelist.h"
> > >  #include "khash.h"
> > >  #include "run-command.h"
> > > +#include "object-store.h"
> > >  
> > >  static void get_git_remote_https_version_info(struct strbuf *version_info)
> > >  {
> > > @@ -128,6 +129,54 @@ static void get_populated_hooks(struct strbuf *hook_info, int nongit)
> > >  	}
> > >  }
> > >  
> > > +static int loose_object_cb(const struct object_id *oid, const char *path,
> > > +			   void *data) {
> > > +	int *loose_object_count = data;
> > > +
> > > +	if (loose_object_count) {
> > > +		(*loose_object_count)++;
> > > +		return 0;
> > > +	}
> > > +
> > > +	return 1;
> > 
> > What is the point of returning 1 here to abort the iteration early?
> > Wouldn't it be a BUG() if this callback ends up gettting called with
> > NULL in data?
> 
> Hrm. Wouldn't BUG() throw away the rest of the generated report?
> 
> Maybe it's better, in NULL case, to indicate an error occurred there and
> write that information into the report, and continue gathering the rest
> of the info.

Oh, hm. Now that I look twice, you're saying "bugreport gave its own
callback a NULL". I guess it could also be that
for_each_loose_object() decided to give a NULL context instead of the
one we gave...

Even so, I dislike the idea of BUG() called from the tool you are trying
to use to report a BUG() message. We do already report if 1 was returned
"(problem during count)". I suppose I can try and make a more specific
error if we know the callback data was definitely the problem.

 - Emily