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@xxxxxxxxxxxx> 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 - 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
