Re: [PATCH v2 07/15] rev-list: allow bitmaps when counting objects

[Jeff King <peff@peff.net>]

On Fri, Feb 14, 2020 at 04:45:55PM -0800, Taylor Blau wrote:

> >  static int try_bitmap_count(struct rev_info *revs)
> >  {
> > -	uint32_t commit_count;
> > +	uint32_t commit_count = 0,
> > +		 tag_count = 0,
> > +		 tree_count = 0,
> > +		 blob_count = 0;
> 
> Hmm, I don't usually see the comma-separated declaration/initialization
> in git.git. Is there a reason you did it here? Not that I really mind
> one way or the other, just interested.

The variables are all related, and all should have the same type. I'd
complain about a patch that did:

  int ret, count;

because there's no logical reason those two variables have the same
type. They just happen to. And putting them both on the same line is
even worse, because it makes a diff changing one of them noisy.

But in the code quoted above, if one of them changes, they would all
(presumably) change. So I think it communicates something to group them
like this. That said, if we had a style guideline that strictly forbade
this (because it's often applied in _bad_ spots), I wouldn't be that
sad to convert it.

> > +	if (revs->max_count >= 0 &&
> > +	    (revs->tag_objects || revs->tree_objects || revs->blob_objects))
> 
> An aside unrelated to the patch at hand: the expression
> 
>   (revs->tag_objects || revs->tree_objects || revs->blob_objects)
> 
> does occur in an awful lot of places throughout this file. Do you
> imagine it'd be useful to pull this check out into its own function,
> perhaps as a preparatory patch in a later version of this series?
> 
> I'm also not fussed if you don't think that such a change would be
> useful, it's just an observation I had after seeing this expression a
> few times.

The most amusing part about it is that there's actually no way to
specify one type without the other. Most of the revision code tries to
be respectful of the fact that we might one day change that (though
there are some subtleties; e.g., asking for blobs but not trees would
still need to traverse the trees, but just not show them).

So this really could have been revs->objects. :) But I think given the
effort to treat them individually so far, it would be a step backwards
to switch now.

As far as whether there should be something like:

  int revs_show_objects(struct rev_info *revs)
  {
	return revs->tag_objects || revs->tree_objects || revs->blob_objects;
  }

I don't feel strongly either way. I find doing it inline perfectly
readable, but it's entirely possible my brain has been rotted by years
of looking at the revision code.

-Peff