On 08/11/2015 10:03 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@xxxxxxxxxxxx> writes:
>
>> Signed-off-by: Michael Haggerty <mhagger@xxxxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html