From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Wed, 29 Apr 2009 22:13:00 GMT
Subject: Re: [PATCH v2] diff -c -p: do not die on submodules
Message-ID: <alpine.DEB.1.00.0904300011140.10279@pacific.mpi-cbg.de>
In-Reply-To: <81b0412b0904291450w3d292ed5i3b2ab5164c0ae0f4@mail.gmail.com>

Hi,

On Wed, 29 Apr 2009, Alex Riesen wrote:

> 2009/4/29 Junio C Hamano <gitster@pobox.com>:
> > +
> > +       if (S_ISGITLINK(mode)) {
> > +               blob = xmalloc(100);
> > +               *size = snprintf(blob, 100,
> > +                                "Subproject commit %s\n", sha1_to_hex(sha1));
> 
> snprintf returns a signed value. It also has a bad record of returning
> negative values for obscure reasons (on obscure platforms, admittedly).
> 
> For this particular case,
> 
>   strcpy(blob, "Subproject commit ");
>   strcat(blob, sha1_to_hex(sha1));
>   strcat(blob, "\n");
>   *size = strlen(blob); /* that's a constant */
> 
> could be considered.

Actually, we know _exactly_ the size of the thing.  It is 18+40+1.  But I 
think that *size wants to have the size, not the length.  So add 1.

In any case, I don't think that we have to jump through hoops here: 
snprintf() is _most_ unlikely to return something negative here.  So I'd 
say that readability trumps paranoia here.

Ciao,
Dscho