Re: [PATCH 1/2] sha1_file: Add sha1_object_type_literally and export it.

[Karthik Nayak <karthik.188@gmail.com>]

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@gmail.com> 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@gmail.com>
> > ---
> > --- 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.