Re: [PATCH 2/2] close_lock_file(): new function in the lockfile API

From: Brandon Casey <casey@nrlssc.navy.mil>
Date: Jan 16, 2008, 22:46 UTC
Message-ID: <478E893F.4070100@nrlssc.navy.mil>
In-Reply-To: <7v7ii9o2ld.fsf@gitster.siamese.dyndns.org>

Junio C Hamano wrote:

Show 7 quoted lines

> Brandon Casey <casey@nrlssc.navy.mil> writes:
> 
>> My patch does this, though I understand it may take some time to review.
> 
> This is what I have right now, squashed your change into [2/2] 
> I sent earlier, along with a couple of further fixups.
>

Mainly, I prefer to not modify the data structures when a failure occurs.

Generally when commit_lock_file fails, the caller die()'s and then remove_lock_file will delete the temporary file. If the caller were to not call die, I think it should call rollback_lock_file() similarly to how unlock_ref() is called everywhere in refs.c.

So I think the semantics should be:

	lock_fd = hold_lock_file(&lock);
	<do_something>
	if (close_lock_file(&lock)) {
		rollback_lock_file(&lock);
		return error;
	}
	if (commit_lock_file(&lock)) {
		rollback_lock_file(&lock);
		return error;
	}

Show 24 quoted lines

> diff --git a/lockfile.c b/lockfile.c
> index f45d3ed..fcf9285 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -13,7 +13,8 @@ static void remove_lock_file(void)
>  	while (lock_file_list) {
>  		if (lock_file_list->owner == me &&
>  		    lock_file_list->filename[0]) {
> -			close(lock_file_list->fd);
> +			if (lock_file_list->fd >= 0)
> +				close(lock_file_list->fd);
>  			unlink(lock_file_list->filename);
>  		}
>  		lock_file_list = lock_file_list->next;
> @@ -159,11 +160,23 @@ int hold_lock_file_for_update(struct lock_file *lk, const char *path, int die_on
>  	return fd;
>  }
>  
> +int close_lock_file(struct lock_file *lk)
> +{
> +	int fd = lk->fd;
> +	lk->fd = -1;
> +	return close(fd);
> +}

minor nit on this that I mentioned in another email.

Show 12 quoted lines

> +
>  int commit_lock_file(struct lock_file *lk)
>  {
>  	char result_file[PATH_MAX];
>  	int i;
> -	close(lk->fd);
> +
> +	if (lk->fd >= 0 && close_lock_file(lk)) {
> +		unlink(lk->filename);
> +		lk->filename[0] = 0;
> +		return -1;
> +	}

I would rather have the caller call rollback_lock_file, or fall back to remove_lock_file rather than do this unlinking and modifying lk->filename here in commit_lock_file.

Show 18 quoted lines

>  	strcpy(result_file, lk->filename);
>  	i = strlen(result_file) - 5; /* .lock */
>  	result_file[i] = 0;
> @@ -185,9 +198,15 @@ void set_alternate_index_output(const char *name)
>  int commit_locked_index(struct lock_file *lk)
>  {
>  	if (alternate_index_output) {
> -		int result = rename(lk->filename, alternate_index_output);
> -		lk->filename[0] = 0;
> -		return result;
> +		const char *newname = alternate_index_output;
> +		alternate_index_output = NULL;
> +
> +		if (lk->fd >= 0 && close_lock_file(lk)) {
> +			unlink(lk->filename);
> +			lk->filename[0] = 0;
> +			return -1;
> +		}

ditto here.

> +		return rename(lk->filename, newname);

If rename succeeds, we'll try to do an unnecessary unlink atexit.

Having said those things, I will defer to your more experienced judgment.

-brandon