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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to