Re: [PATCH v2 12/16] diff: use tempfile module
- From
Michael Haggerty <mhagger@alum.mit.edu>
- Date
- Aug 12, 2015, 15:13 UTC
- Message-ID
- <55CB62B7.8060706@alum.mit.edu>
- In-Reply-To
- <xmqqbnedr3sp.fsf@gitster.dls.corp.google.com>
On 08/11/2015 10:03 PM, Junio C Hamano wrote:
Show 51 quoted lines
> 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.
Show 10 quoted lines
> 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