Re: [PATCH 06/14] tempfile: add several functions for creating temporary files

[Michael Haggerty <mhagger@alum.mit.edu>]

On 06/10/2015 07:48 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> Add several functions for creating temporary files with
>> automatically-generated names, analogous to mkstemps(), but also
>> arranging for the files to be deleted on program exit.
>>
>> The functions are named according to a pattern depending how they
>> operate. They will be used to replace many places in the code where
>> temporary files are created and cleaned up ad-hoc.
>>
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
>>  tempfile.c | 55 ++++++++++++++++++++++++++++++++++-
>>  tempfile.h | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 150 insertions(+), 1 deletion(-)
>>
>> diff --git a/tempfile.c b/tempfile.c
>> index f76bc07..890075f 100644
>> --- a/tempfile.c
>> +++ b/tempfile.c
>> @@ -48,7 +48,7 @@ static void register_tempfile_object(struct tempfile *tempfile, const char *path
>>  		tempfile->fp = NULL;
>>  		tempfile->active = 0;
>>  		tempfile->owner = 0;
>> -		strbuf_init(&tempfile->filename, strlen(path));
>> +		strbuf_init(&tempfile->filename, 0);
>>  		tempfile->next = tempfile_list;
>>  		tempfile_list = tempfile;
>>  		tempfile->on_list = 1;
> 
> This probably could have been part of the previous step.  Regardless
> of where in the patch series this change is done, I think it makes
> sense, as this function does not even know how long the final filename
> would be, and strlen(path) is almost always wrong as path is likely to
> be relative.
> 
> I notice this change makes "path" almost unused in this function,
> and the only remaining use is for assert(!tempfile->filename.len).
> Perhaps it is not worth passing the "path" parameter?

These are both good points. I will implement them in the upcoming v2.

Thanks,
Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu