Re: [PATCH v3 1/2] Git.pm: add new temp_is_locked function

From: David Rothenberger <daveroth@acm.org>
Date: Jul 18, 2013, 18:34 UTC
Message-ID: <loom.20130718T202918-857@post.gmane.org>
In-Reply-To: <1373170849-9150-2-git-send-email-mackyle@gmail.com>

Kyle J. McKay <mackyle <at> gmail.com> writes:

Show 19 quoted lines

> +sub temp_is_locked {
> +	my ($self, $name) = _maybe_self( <at> _);
> +	my $temp_fd = \$TEMP_FILEMAP{$name};
> +
> +	defined $$temp_fd && $$temp_fd->opened && $TEMP_FILES{$$temp_fd}{locked};
> +}
> +
>  =item temp_release ( NAME )
> 
>  =item temp_release ( FILEHANDLE )
>  <at>  <at>  -1248,7 +1277,7  <at>  <at>  sub _temp_cache {
> 
>  	my $temp_fd = \$TEMP_FILEMAP{$name};
>  	if (defined $$temp_fd and $$temp_fd->opened) {
> -		if ($TEMP_FILES{$$temp_fd}{locked}) {
> +		if (temp_is_locked($name)) {
>  			throw Error::Simple("Temp file with moniker '" .
>  				$name . "' already in use");
>  		}

There's a problem with this use of temp_is_locked. There is an else clause right after this:

	} else {
		if (defined $$temp_fd) {
			# then we're here because of a closed handle.

Prior to the patch, the comment is correct, but after the patch, the if block may also be entered if the file is open but locked. This is because temp_is_locked checks that the temp file is defined, open, and locked.

This issue leads to lots of

  Temp file 'svn_delta_3360_0' was closed. Opening replacement.

messages for me.

Reverting the change in _temp_cache solves the problem for me. Adding an " && !$$temp_fd->opened" clause to the if statement also works, but this is less efficient.