Hi Peff,
On Thu, Apr 11, 2019 at 03:37:35PM -0400, Jeff King wrote:
> On Thu, Apr 11, 2019 at 12:14:52PM -0700, Taylor Blau wrote:
>
> > > This series cleans up most of the bare calls found by:
> > >
> > > git grep -E '[^a-z_](m|c|re)alloc\(' '*.c' :^compat :^contrib :^wrapper.c
> >
> > This (admittedly pretty awesome) 'git grep' invocation reminds me of a
> > series I was pretty sure you wrote to ban functions like 'strcpy' and
> > other obviously bad things.
> >
> > Some quick searching turned up [1], which landed as f225611d1c
> > (automatically ban strcpy(), 2018-07-26). Do we want something similar
> > here? Of course, the locations below would have to be exempt, but it
> > seems worthwhile (and would save a review cycle in the case that someone
> > added a 'malloc' in a patch sent here).
>
> I don't think we can ban malloc, since we have to use it ourselves. :)
>
> With some contortions, we probably could unban it specifically in
> wrapper.c (though note there are a few other calls I've left which would
> need to be handled somehow).
Right. I think that I should have made this point clearer in my initial
reply. I was thinking that we could #undef the banned macro in
wrapper.c, or some similar hula-hooping.
> Another option would be coccinelle patches to convert malloc() to
> xmalloc(), etc (with an exception for the wrappers). I'm not entirely
> comfortable with automatic conversion here because there's often some
> follow-up adjustments (i.e., we can stop handling allocation errors and
> maybe delete some code). I think coccinelle can identify callers and
> barf, though.
>
> I'm not sure whether coccinelle saves review cycles (since that implies
> people actually run it, though maybe that is better now that it's part
> of Travis?). It seems to me that it's usually more helpful for people
> periodically doing follow-up auditing.
>
> So I dunno. If this was a common mistake I'd be more concerned with
> saving review cycles, but all of the cases I found were actually just
> leftovers from the very early days of Git.
Yeah... maybe that's the bigger question that I hadn't asked. I made the
suggestion thinking that it would help newcomers avoid writing
'malloc()' and sending it if they didn't know we use our 'xmalloc()'
instead.
But I'm not sure if the argument holds up. I think that in general
exactly the sorts of new-comers that I'm thinking of would have more
than one review cycle anyway, so it might not be worth the effort,
anyway...
> -Peff
Thanks,
Taylor
