On Sat, Jun 24, 2017 at 02:12:30PM +0200, René Scharfe wrote:
> > 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).
> -- >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.
> 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
