Re: [PATCH v2 2/3] textconv: support for blame

[Jeff King <peff@peff.net>]

[resending to cc git@vger]

On Tue, Jun 15, 2010 at 12:29:43PM +0200, bonneta wrote:

> We have changed the declaration of textconv_object() to:
> 
> static int textconv_object(const char *path,
>                            const unsigned char *sha1,
>                            char **buf,
>                            unsigned long *buf_size)
> 
> And now we can do:
> *buf_size = fill_textconv(textconv, df, buf);
> without any cast.

I assume you mean dropping the final buf_size parameter from that
declaration, which is what your usage example has. I would return either
an "unsigned long" or a size_t rather than an int. We are dealing with
potential whole-file sizes, so it is better to use at least as large a
data type as other parts of the code (we still may run into truncation
problems, but at least you are not making things any worse).

> But we have to do:
> textconv_object(read_from, null_sha1, &buf.buf, (unsigned long *)
> &buf.len))
> where buf.len is size_t.
> 
> Is that ok?

No, that has the same problem. Imagine a big endian machine with a
32-bit unsigned long and a 64-bit size_t. You would write into the first
32 bits of buf.len, which are the high bits, giving you a ridiculously
large answer.

The only portable way in C to convert between types is by assignment. So
you have to do:

  unsigned long foo;
  textconv_object(read_from, null_sha1, &buf.buf, &foo);
  buf.len = foo;

But now I'm confused. That matches the declaration you gave in the first
part of your email, but not the usage example.

-Peff