Re: [PATCH] sha1_name: cache readdir(3) results in find_short_object_filename()

From: Jeff King <peff@peff.net>
Date: Jun 24, 2017, 12:20 UTC
Message-ID: <20170624122057.ndmlsa7e7dksejg3@sigill.intra.peff.net>
In-Reply-To: <97a8def3-d2ed-9212-4d87-8767de6f2695@web.de>

On Sat, Jun 24, 2017 at 02:12:30PM +0200, René Scharfe wrote:

Show 8 quoted lines

> > That's redundant with "subdir_nr". Should we push that logic down into
> > the function, and basically do:
> > 
> >    if (subdir_nr < 0 || subdir_nr > 256)
> > 	BUG("object subdir number out of range");
> 
> Hmm.  I don't expect many more callers, so do we really need this safety
> check?  It's cheap compared to the readdir(3) call, of course.

To me it's as much about documenting the assumptions as it is about catching buggy callers. I'd be OK with a comment, too.

> But wouldn't it make more sense to use an unsigned data type to avoid
> the first half?  And an unsigned char specifically to only accept valid
> values, leaving the overflow concern to the caller?  It warrants a
> separate patch in any case IMHO.

Using unsigned makes sense. Using "char" because you know it only goes to 256 is a bit too subtle for my taste. And yes, I'd do it in a preparatory patch (or follow-on if you prefer).

Show 9 quoted lines

> -- >8 --
> Subject: [PATCH] sha1_file: let for_each_file_in_obj_subdir() handle subdir names
> 
> The function for_each_file_in_obj_subdir() takes a object subdirectory
> number and expects the name of the same subdirectory to be included in
> the path strbuf.  Avoid this redundancy by letting the function append
> the hexadecimal subdirectory name itself.  This makes it a bit easier
> and safer to use the function -- it becomes impossible to specify
> different subdirectories in subdir_nr and path.

Yeah, this is what I had in mind.

Show 5 quoted lines

>  	for (i = 0; i < 256; i++) {
> -		strbuf_addf(path, "/%02x", i);
>  		r = for_each_file_in_obj_subdir(i, path, obj_cb, cruft_cb,
>  						subdir_cb, data);
> -		strbuf_setlen(path, baselen);

One side effect of the original code is that this trailing setlen() would catch any early-exits from for_each_file_in_obj_subdir() which forgot to reset the strbuf. If any exist, that's a bug that should be in fixed in that function, though. I double-checked, and there aren't any (your patch already handles the extra setlen required when opendir fails).

-Peff