Re: [PATCH] Improved error messages when temporary file creation fails

[Arnout Engelen <arnouten@bzzt.net>]

Thanks to you and Jonathan again for the feedback.

On Tue, Dec 07, 2010 at 12:56:17PM -0800, Junio C Hamano wrote:
> > +	char origtemplate[255];
> > +	strlcpy(origtemplate, template, 255);
> 
> Why "255"?

Random - 'i had to choose something'.

> It may happen to be sufficiently large for the current callers, but what
> provisions if any are made to help the compiler or the runtime protect us
> from new and broken callers?  Use of strlcpy() there hides the issue from
> the runtime by avoiding segfault, but it actively harms us by making the
> code silently behave incorrectly without segfaulting, no?

Only in a small way: when a bigger template is encountered and the mkstemp 
call succeeds, there is no problem. Only when xmkstemp fails *and* clears the
template, the diagnostic error message shows a truncated version of the 
original.

I *could* dynamically allocate space for the original template string, but that
would mean I'd need to do a malloc() instead of putting the buffer on the stack
like this, and free() it afterwards. I'm not too concerned about the 
performance hit here (presumably the I/O that comes with creating and using 
the temporary file here is orders of magnitude slower than that malloc() 
anyway), but it would also make the code a bit less easy to read.

What do you think would be preferable here, a simple fixed-length buffer on the
stack that might cause a truncated error message or a dynamically-allocated 
one that makes the code somewhat more complicated?

> > +++ b/wrapper.h
> 
> Somewhat questionable...

Agreed, this whole file is unneeded and, well, wrong anyway. 

I'll remove wrapper.h and apply Jonathan's improvements some time this week, 
unless of course someone beats me to it :). 


Kind regards,

Arnout