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. -Daniel *This .sig left intentionally blank* - 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
