From: Jonathan Nieder <jrnieder@gmail.com>
Date: Sun, 14 Nov 2010 21:22:43 GMT
Subject: Re: [PATCHv6 06/23] notes.h/c: Propagate combine_notes_fn return value to add_note() and beyond
Message-ID: <20101114212243.GC10150@burratino>
In-Reply-To: <AANLkTinQhkXvfa0BS0dFK6-mXFpU+uQ=rKRJ1QaXXA1S@mail.gmail.com>

Thiago Farina wrote:
> On Tue, Nov 9, 2010 at 7:49 PM, Johan Herland <johan@herland.net> wrote:

>> --- a/builtin/notes.c
>> +++ b/builtin/notes.c
>> @@ -573,8 +573,8 @@ static int add(int argc, const char **argv, const char *prefix)
>>
>>        if (is_null_sha1(new_note))
>>                remove_note(t, object);
>> -       else
>> -               add_note(t, object, new_note, combine_notes_overwrite);
>> +       else if (add_note(t, object, new_note, combine_notes_overwrite))
>> +               die("confused: combine_notes_overwrite failed");
>
> This message looks like more a debug information. Could it be
> rewritten to be more user friendly / more informative?

Sverre mentioned the same thing.  The intent is the same as the existing

		die("should not happen, someone must be hit on the forehead");

--- i.e., to provoke bug reports if it happens, without the appearance
of begging. :)

So how about:

		die("BUG: combine_notes_overwrite failed");

Something involving assert() could also work, but that tends to be
a bit ugly.