Re: [PATCH] alloc_ref(): allow for trailing NUL

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

On Fri, 28 Sep 2007, Daniel Barkalow wrote:

> On Fri, 28 Sep 2007, Johannes Schindelin wrote:
> 
> > Hi,
> > 
> > On Fri, 28 Sep 2007, Junio C Hamano wrote:
> > 
> > > Daniel Barkalow <barkalow@iabervon.org> writes:
> > > 
> > > > On Fri, 28 Sep 2007, Johannes Schindelin wrote:
> > > >
> > > >> The parameter name "namelen" suggests that you pass the equivalent of
> > > >> strlen() to the function alloc_ref().  However, this function did not
> > > >> allocate enough space to put a NUL after the name.
> > > >> 
> > > >> Since struct ref does not have any member to describe the length of the
> > > >> string, this just does not make sense.
> > > >> 
> > > >> So make space for the NUL.
> > > >
> > > > Good point, but shouldn't you then fix call sites that use strlen(name) + 
> > > > 1?
> > > 
> > > Good point.
> > > 
> > > I audited "git grep -A2 -B4 -e alloc_ref next master" output,
> > > and it appears almost everybody knows alloc_ref() wants the
> > > caller to count the terminating NUL.
> > > 
> > > There however are a few gotchas.
> > > 
> > >  * There is one overallocation in connect.c, which would not
> > >    hurt but is wasteful;
> > > 
> > >  * next:transport.c has alloc_ref(strlen(e->name)) which is a
> > >    no-no;
> > > 
> > > Discarding Johannes's patch, the following would fix it.
> > 
> > But should the signature of alloc_ref() not be changed, then, to read
> > 
> > 	struct ref *alloc_ref(unsigned name_alloc);
> > 
> > Hm?
> > 
> > Further, I am quite sure that the same mistake will happen again, until we 
> > change the function to get the name length, not the number of bytes to 
> > allocate.
> 
> I agree. But leaving the majority of cases using the old convention is 
> just confusing.

Yeah, sorry, that patch was only half-cooked.

If people agree with me, I'll redo the patch (fixing all calling sites, 
too).

Ciao,
Dscho