Re: [PATCH v2 12/16] diff: use tempfile module

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

On 08/11/2015 10:03 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
>>  diff.c | 29 +++++++----------------------
>>  1 file changed, 7 insertions(+), 22 deletions(-)
> 
> Nice code reduction.
> 
>> diff --git a/diff.c b/diff.c
>> index 7500c55..dc95247 100644
>> --- a/diff.c
>> +++ b/diff.c
>> @@ -2,6 +2,7 @@
>>   * Copyright (C) 2005 Junio C Hamano
>>   */
>>  #include "cache.h"
>> +#include "tempfile.h"
>>  #include "quote.h"
>>  #include "diff.h"
>>  #include "diffcore.h"
>> @@ -312,7 +313,7 @@ static struct diff_tempfile {
>>  	const char *name; /* filename external diff should read from */
>>  	char hex[41];
>>  	char mode[10];
>> -	char tmp_path[PATH_MAX];
>> +	struct tempfile tempfile;
>>  } diff_temp[2];
>>  
>>  typedef unsigned long (*sane_truncate_fn)(char *line, unsigned long len);
>> @@ -564,25 +565,16 @@ static struct diff_tempfile *claim_diff_tempfile(void) {
>>  	die("BUG: diff is failing to clean up its tempfiles");
>>  }
>>  
>> -static int remove_tempfile_installed;
>> -
>>  static void remove_tempfile(void)
>>  {
>>  	int i;
>>  	for (i = 0; i < ARRAY_SIZE(diff_temp); i++) {
>> -		if (diff_temp[i].name == diff_temp[i].tmp_path)
>> -			unlink_or_warn(diff_temp[i].name);
>> +		if (is_tempfile_active(&diff_temp[i].tempfile))
>> +			delete_tempfile(&diff_temp[i].tempfile);
> 
> I suspect that this indicates that there is something iffy in the
> conversion.  The original invariant, that is consistently used
> between claim_diff_tempfile() and remove_tempfile(), is that .name
> field points at .tmp_path for a slot in diff_temp[] that holds a
> temporary that is in use.  Otherwise, .name is NULL and it can be
> claimed for your own use.

No, prepare_temp_file() sometimes sets diff_tempfile::name to
"/dev/null", and sometimes to point at its argument `name`. In either of
these cases .tmp_path can hold anything, and the file is *not* cleaned
up even though the diff_temp entry is considered by
claim_diff_tempfile() to be in use.

If I'm not mistaken, the old invariant was:

* Iff diff_tempfile::name is NULL, the entry is not in use.
* Iff diff_tempfile::name == diff_tempfile::tmp_path, then the entry is
in use and refers to a temporary file that needs to be cleaned up.
* Otherwise, the entry is in use but the corresponding file should *not*
be cleaned up.

The new invariant is:

* Iff diff_tempfile::name is NULL, the entry is not in use. In these
cases, is_tempfile_active() is always false.
* Iff is_tempfile_active(diff_tempfile::tempfile), then it refers to a
file that needs to get cleaned up. In these cases name points at the
tempfile object's filename.
* If neither of the above is true, then the entry is in use but the
corresponding file should not be cleaned up.

> Here the updated code uses a different and new invariant: .tempfile
> satisfies is_tempfile_active() for a slot in use.  But the check in
> claim_diff_tempfile() still relies on the original invariant.

That is not true. The is_tempfile_active() check is only used in
remove_tempfile() when deciding whether to clean up the file. The check
in claim_diff_tempfile() wants to know whether the entry is in use, so
it uses the other check.

> The updated code may happen to always have an active tempfile in
> tempfile and always set NULL when it clears .name, but that would
> mean (1) future changes may easily violate one of invariants (we
> used to have only one, now we have two that have to be sync) by
> mistake, and (2) we are keeping track of two closely linked things
> as two invariants.
> 
> As the value that used to be in the .name field can now be obtained
> by calling get_tempfile_path() on the .tempfile field, perhaps we
> should drop .name (and its associated invariant) at the same time?

This is also incorrect. See my first paragraph above.

I will change this patch to document the invariants.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu