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

[Junio C Hamano <gitster@pobox.com>]

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.

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.

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?