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

[Jonathan Nieder <jrnieder@gmail.com>]

Torsten Bögershausen wrote:
> On 12/03/2014 06:14 AM, Jonathan Nieder wrote:

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