On Tue, Oct 31, 2017 at 02:50:06PM +0100, René Scharfe wrote:
> The path of a loose object contains its hash value encoded into two
> substrings of 2 and 38 hexadecimal digits separated by a slash. The
> first part is handed to for_each_file_in_obj_subdir() in decoded form as
> subdir_nr. The current code builds a full hexadecimal representation of
> the hash in a temporary buffer, then uses get_oid_hex() to decode it.
>
> Avoid the intermediate step by taking subdir_nr as-is and using
> hex_to_bytes() directly on the second substring. That's shorter and
> easier.
This raises some of the same questions as the previous one on whether
hex_to_bytes() is the ideal abstraction. But as before, I'm on the
fence.
> @@ -1908,20 +1911,15 @@ int for_each_file_in_obj_subdir(unsigned int subdir_nr,
> strbuf_setlen(path, baselen);
> strbuf_addf(path, "/%s", de->d_name);
>
> - if (strlen(de->d_name) == GIT_SHA1_HEXSZ - 2) {
> - char hex[GIT_MAX_HEXSZ+1];
> - struct object_id oid;
> -
> - xsnprintf(hex, sizeof(hex), "%02x%s",
> - subdir_nr, de->d_name);
> - if (!get_oid_hex(hex, &oid)) {
> - if (obj_cb) {
> - r = obj_cb(&oid, path->buf, data);
> - if (r)
> - break;
> - }
> - continue;
> + if (strlen(de->d_name) == GIT_SHA1_HEXSZ - 2 &&
> + !hex_to_bytes(oid.hash + 1, de->d_name,
> + GIT_SHA1_RAWSZ - 1)) {
> + if (obj_cb) {
> + r = obj_cb(&oid, path->buf, data);
> + if (r)
> + break;
> }
> + continue;
> }
>
> if (cruft_cb) {
Now that this is one big conditional for "is this a valid object
filename", I think we could get rid of the "continue" in favor of:
if (...looks like an object...)
...call obj_cb...
else if (cruft_cb)
...call cruft_cb...
Not a big deal, but it may make the flow more clear (the original had to
use a continue because there were multiple independent steps to
determining it was an object file, so we had to "break out" from the
inner conditional).
-Peff
