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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 11, 2015, 20:03 UTC
Message-ID: <xmqqbnedr3sp.fsf@gitster.dls.corp.google.com>
In-Reply-To: <404c8bc508639a5723420691d9daa122f10d7cd4.1439198011.git.mhagger@alum.mit.edu>

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.

Show 35 quoted lines

> 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?