From: Torsten Bögershausen <tboegi@web.de>
Date: Wed, 03 Dec 2014 06:09:28 GMT
Subject: Re: [PATCH 04/14] hold_lock_file_for_append: pass error message back through a strbuf
Message-ID: <547EA918.30003@web.de>
In-Reply-To: <20141203051459.GN6527@google.com>

On 12/03/2014 06:14 AM, Jonathan Nieder wrote:
> This way, the code does not need to carefully safeguard errno to allow
> callers to print a reasonable error message when they choose to do
> some cleanup before die()-ing.
>
> Fixes a bug waiting to happen where copy_fd would clobber the errno
> passed back via hold_lock_file_for_append from read() or write() when
> flags did not contain LOCK_DIE_ON_ERROR.  Luckily the only caller uses
> flags == LOCK_DIE_ON_ERROR, avoiding that bug in practice.
>
> Reported-by: Ronnie Sahlberg <sahlberg@google.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> The advertised bugfix.
>
>   lockfile.c  | 29 ++++++++++-------------------
>   lockfile.h  |  3 ++-
>   sha1_file.c |  7 ++++++-
>   3 files changed, 18 insertions(+), 21 deletions(-)
>
> diff --git a/lockfile.c b/lockfile.c
> index b3020f3..8685c68 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -179,45 +179,36 @@ int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)
>   	return fd;
>   }
>   
> -int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)
> +int hold_lock_file_for_append(struct lock_file *lk, const char *path,
> +			      int flags, struct strbuf *err)
>   {
>   	int fd, orig_fd;
> -	struct strbuf err = STRBUF_INIT;
> +
> +	assert(!(flags & LOCK_DIE_ON_ERROR));
> +	assert(err && !err->len);
>   
What do I miss ?
In the old code we die() printing the errno when LOCK_DIE_ON_ERRORis set.
Now we die() in an assert() or two ?
And should'nt we assert in  strbuf_addf() instead ?

And in general, does the commit message deserve an update?

>Luckily the only caller uses flags == LOCK_DIE_ON_ERROR,
The first impression was: Why do we not simplify the code and remove
the flag completely ?
This feels somewhat like maintaining dead code, which may be used later.
(Unless it will be used in later commit, and the we could mention it here)