From: Junio C Hamano <gitster@pobox.com>
Date: Thu, 17 Jan 2008 17:57:35 GMT
Subject: Re: [PATCH] fast-import.c: don't try to commit marks file if write failed
Message-ID: <7vmyr4jpww.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <478F893A.6050105@nrlssc.navy.mil>

Brandon Casey <casey@nrlssc.navy.mil> writes:

> We also move the assignment of -1 to the lock file descriptor
> up, so that rollback_lock_file() can be called safely after a
> possible attempt to fclose(). This matches the contents of
> the 'if' statement just above testing success of fdopen().
>
> Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
> ---

Shawn, could you lend an extra set of eyeballs on this one
please?  It looks sane to me but obviously you are more familiar
with the code.

> Since we forget the lock file descriptor there is a chance
> that we will leave the file open if a write error occurs. We'll
> still delete the file. I don't think it's worth bending
> over backwards to make sure the file is closed on this failure.
>
> -brandon
>
>
>  fast-import.c |   24 ++++++++++++++++--------
>  1 files changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/fast-import.c b/fast-import.c
> index 3609c24..4cf5092 100644
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -1545,19 +1545,27 @@ static void dump_marks(void)
>  		return;
>  	}
>  
> -	dump_marks_helper(f, 0, marks);
> -	if (ferror(f) || fclose(f))
> -		failure |= error("Unable to write marks file %s: %s",
> -			mark_file, strerror(errno));
>  	/*
> -	 * Since the lock file was fdopen()'ed and then fclose()'ed above,
> -	 * assign -1 to the lock file descriptor so that commit_lock_file()
> +	 * Since the lock file was fdopen()'ed, it should not be close()'ed.
> +	 * Assign -1 to the lock file descriptor so that commit_lock_file()
>  	 * won't try to close() it.
>  	 */
>  	mark_lock.fd = -1;
> -	if (commit_lock_file(&mark_lock))
> -		failure |= error("Unable to write commit file %s: %s",
> +
> +	dump_marks_helper(f, 0, marks);
> +	if (ferror(f) || fclose(f)) {
> +		rollback_lock_file(&mark_lock);
> +		failure |= error("Unable to write marks file %s: %s",
>  			mark_file, strerror(errno));
> +		return;
> +	}
> +
> +	if (commit_lock_file(&mark_lock)) {
> +		rollback_lock_file(&mark_lock);
> +		failure |= error("Unable to commit marks file %s: %s",
> +			mark_file, strerror(errno));
> +		return;
> +	}
>  }
>  
>  static int read_next_command(void)
> -- 
> 1.5.4.rc3.17.gb63a4