Re: [PATCH 7/9] object-store: provide helpers for loose_objects_cache

From: Jeff King <peff@peff.net>
Date: Nov 12, 2018, 20:16 UTC
Message-ID: <20181112201623.GB7598@sigill.intra.peff.net>
In-Reply-To: <a766f9c6-4df9-c249-2b7b-89a2f4ccf405@web.de>

On Mon, Nov 12, 2018 at 08:24:59PM +0100, René Scharfe wrote:

Show 8 quoted lines

> > +void odb_load_loose_cache(struct object_directory *odb, int subdir_nr)
> > +{
> > +	struct strbuf buf = STRBUF_INIT;
> > +
> > +	if (subdir_nr < 0 ||
> 
> Why not make subdir_nr unsigned (like in for_each_file_in_obj_subdir()), and
> get rid of this first check?

I stole the use of "int" from your code. ;)

More seriously, though, I wondered if callers might have sign issues assigning from a "signed char". Usually we hold object ids in an "unsigned char", but what happens if I do:

  signed char foo[] = { 1, 2, 3, 4 };
  odb_load_loose_cache(foo[0]);

when the parameter is "unsigned"?

I'll admit I get lost in all of the integer promotion rules there, but are we sure there's no way we can end up with a funky truncation?

If the answer is no, then I agree that your suggestion is a strict improvement.

> > +	    subdir_nr >= ARRAY_SIZE(odb->loose_objects_subdir_seen))
> 
> Using unsigned char for subdir_nr would allow removing the second check as
> well, but might hide invalid values in implicit conversions, I guess.

Yeah, I know that one could be a dangerous truncation.

I also considered just taking an object_id, which would make the function "load the cache such that this oid would be valid". And it's not necessarily the caller's business how much we load.

But that's OK for OBJECT_INFO_QUICK, but it's pretty darn subtle for the abbrev code. That code doesn't care about just one object, but wants all objects that share its prefix. That works now because we know that the prefix is always at least 2 hex chars, so it's OK to load just that subset.

> > +		BUG("subdir_nr out of range");
> 
> Showing the invalid value (like in for_each_file_in_obj_subdir()) would make
> debugging easier in case the impossible actually happens.

Good suggestion.

Show 13 quoted lines

> > +	strbuf_addstr(&buf, odb->path);
> > +	for_each_file_in_obj_subdir(subdir_nr, &buf,
> > +				    append_loose_object,
> > +				    NULL, NULL,
> > +				    &odb->loose_objects_cache);
> > +	odb->loose_objects_subdir_seen[subdir_nr] = 1;
> 
> About here would be the ideal new home for ...
> [...]
> > -
> > -	strbuf_release(&buf);
> 
> ... this line.

Oops, thanks. I toyed with making the strbuf here static, which is why I dropped the release. But since we only use it on a cache miss, I decided it was better to avoid the hidden global (and then of course forgot to re-add the release).

-Peff