Re: [PATCH] Bugfix: GIT_EXTERNAL_DIFF with more than one changed files

From: Jeff King <peff@peff.net>
Date: Feb 12, 2009, 14:07 UTC
Message-ID: <20090212140740.GB3057@coredump.intra.peff.net>
In-Reply-To: <20090212133614.GA12746@bigbear>

On Thu, Feb 12, 2009 at 09:36:14PM +0800, Nazri Ramliy wrote:

Show 6 quoted lines

> Regression introduced in 479b0ae81c9291a8bb8d7b2347cc58eeaa701304.
> 
> When there is more than one file that are changed, running
> git diff with GIT_EXTERNAL_DIFF works only for the first file.
> 
> This patch fixes this problem and added a test case for it.

Yikes. Thanks for finding this.

Actually, the situation is a little more complex than what you describe.

We used to just re-use the diff_temp array unconditionally; the commit you mention introduced a safety check to make sure we are not overwriting an existing tempfile that should be cleaned up. That safety check looks for the "name" field being NULL to signal an unused slot.

But the "remove_tempfile" function uses a different test to see if a slot needs to be deleted: if the name and the tmp_path are the same. And they would not be if we were able to reuse a working tree file as part of the diff, in which case there would be nothing to clean up. And if we did clean something up, we set the name field to NULL.

So this bug should trigger only in the face of reusing worktree files. I checked your test; it constructs a diff between the worktree and the index, so it correctly finds the problem.

Show 11 quoted lines

>  {
>  	int i;
> -	for (i = 0; i < ARRAY_SIZE(diff_temp); i++)
> -		if (diff_temp[i].name == diff_temp[i].tmp_path) {
> +	for (i = 0; i < ARRAY_SIZE(diff_temp); i++) {
> +		if (diff_temp[i].name == diff_temp[i].tmp_path)
>  			unlink(diff_temp[i].name);
> -			diff_temp[i].name = NULL;
> -		}
> +		diff_temp[i].name = NULL;
> +	}

Note that the other possible fix for this bug is to change claim_diff_tempfile to use the same test. But I prefer this, as it is more idiomatic to use NULL as a marker for "not in use".

Acked-by: Jeff King <peff@peff.net>

-Peff