Re: [PATCH 04/14] hold_lock_file_for_append: pass error message back through a strbuf

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Dec 3, 2014, 07:04 UTC
Message-ID: <20141203070411.GA24344@google.com>
In-Reply-To: <547EA918.30003@web.de>

Torsten Bögershausen wrote:

> On 12/03/2014 06:14 AM, Jonathan Nieder wrote:

Show 17 quoted lines

>> --- 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.

The assertion documents an assumption that no caller will set the LOCK_DIE_ON_ERROR flag bit. A later patch in the series eliminates that flag bit completely.

> Now we die() in an assert() or two ?

assert() is not safe to use for anything other than documenting assumptions. It can't be relied on to exit (let alone to exit with a nice message like die()). So the die() that was removed and assert() that this patch adds have different purposes.

> And should'nt we assert in  strbuf_addf() instead ?

strbuf_addf can be used to append to a nonempty strbuf.

[...]

> (Unless it will be used in later commit, and the we could mention it here)

Good idea.  I'll add to the commit message if rerolling.

Thanks, Jonathan