I had written a longer review but was interrupted for a several hours,
and upon returning found that David and Junio covered many of the same
issues or overrode comments I was making, so the below review is pared
down quite a bit. Junio's proposed approach negates all of the below
review comments, but they may still be meaningful if kept in mind for
future submissions.
On Wed, Feb 25, 2015 at 6:07 AM, Karthik Nayak <karthik.188@xxxxxxxxx> wrote:
> sha1_file: Add sha1_object_type_literally and export it.
Style: downcase "Add"; drop terminating period.
> sha1_object_type_literally takes a sha value and
> gives the type of the given loose object, used by
> git cat-file -t --literally.
>
> Signed-off-by: Karthik Nayak <karthik.188@xxxxxxxxx>
> ---
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2635,6 +2635,33 @@ int sha1_object_info(const unsigned char *sha1, unsigned long *sizep)
> return type;
> }
>
> +int sha1_object_type_literally(const unsigned char *sha1, char *type)
This functionality is very specific to the --literally option you're
adding to cat-file, so it would make more sense to make it private to
builtin/cat-file.c rather than publishing it globally.
Also, this is an unsafe contract. The caller does not know how many
bytes to allocate for 'type', and this new function may write past the
end of the buffer. It is more common to also pass in the size of the
'type' buffer and ensure that you do not write beyond that. Or, if
this is intended for wider consumption, pass in a strbuf instead.
> +{
> + int status = 0;
> + unsigned long mapsize;
> + void *map;
> + git_zstream stream;
> + char hdr[32];
> + int i;
> +
> + map = map_sha1_file(sha1, &mapsize);
> + if (!map)
> + return -1;
> + if (unpack_sha1_header(&stream, map, mapsize, hdr, sizeof(hdr)) < 0)
> + status = error("unable to unpack %s header",
> + sha1_to_hex(sha1));
Since 'hdr' unpacking failed, shouldn't you be returning at this point
rather than continuing to the 'hdr' processing loop?
> + for (i = 0; i < 32; i++) {
> + if (hdr[i] == ' ') {
> + type[i] = '\0';
> + break;
> + }
> + type[i] = hdr[i];
> + }
David already mentioned that this loop is suspect. Perhaps take a look
at, sha1_file.c:parse_sha1_header() for an example of cleaner logic.
> +
> + return status;
> +}
> +
> static void *read_packed_sha1(const unsigned char *sha1,
> enum object_type *type, unsigned long *size)
> {
> --
> 2.3.1.129.g11acff1.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html