On Wed, 2015-02-25 at 16:55 -0500, Eric Sunshine wrote:
> 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
Thanks for all your inputs, I will work on the points you've mentioned
considering what David and Junio also have mentioned.
--
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