From: Jonathan Nieder <jrnieder@gmail.com>
Date: Tue, 16 Sep 2014 20:37:41 GMT
Subject: Re: [PATCH v5 03/35] rollback_lock_file(): do not clear filename redundantly
Message-ID: <20140916203741.GC29050@google.com>
In-Reply-To: <1410896036-12750-4-git-send-email-mhagger@alum.mit.edu>

Michael Haggerty wrote:

> It is only necessary to clear the lock_file's filename field if it was
> not already clear.
[...]
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -276,6 +276,6 @@ void rollback_lock_file(struct lock_file *lk)
>  		if (lk->fd >= 0)
>  			close(lk->fd);
>  		unlink_or_warn(lk->filename);
> +		lk->filename[0] = 0;
>  	}
> -	lk->filename[0] = 0;

Now that it does nothing when lk->filename[0] == '\0', this could be
de-indented a little:

	if (!lk->filename[0])
		return;
	if (lk->fd >= 0)
		close(lk->fd);
	unlink_or_warn(lk->filename);
	lk->filename[0] = 0;

This could technically double-close if interrupted by a signal that
tries to remove the file again, right?  Should this use
close_lock_file which sets fd to -1 before closing?

	if (!lk->filename[0])
		return;
	close_lock_file(lk);
	unlink_or_warn(lk->filename);
	lk->filename[0] = 0;

With or without such changes,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>