Re: [PATCH v2 5/5] ref-filter: get_ref_atom_value() error handling

[Junio C Hamano <gitster@pobox.com>]

Martin Ågren <martin.agren@gmail.com> writes:

>>  static int grab_objectname(const char *name, const unsigned char *sha1,
>> -                          struct atom_value *v, struct used_atom *atom)
>> +                          struct atom_value *v, struct used_atom *atom,
>> +                          struct strbuf *err)
>>  {
>> ...
>> +               } else {
>> +                       strbuf_addstr(err, "BUG: unknown %(objectname) option");
>> +                       return -1;
>> +               }
>>         }
>>         return 0;
>>  }
>
> This is interesting. This die() is never ever supposed to actually
> trigger, except to allow a developer adding some new O_xxx-value to
> quickly notice that they have forgotten to add code here.

Yup, BUG() is meant for a case like this; the original code predates
the BUG("message") which in turn predates the die("BUG: message")
convention, I think.

>>         default:
>> -               die("Eh?  Object of type %d?", obj->type);
>> +               strbuf_addf(err, "Eh?  Object of type %d?", obj->type);
>> +               return -1;
>>         }
>> +       return 0;
>>  }
>
> This seems similar. The string here is quite sloppy, and I do not
> believe that the author intended this to be user-visible. I believe this
> is more like a very short way of saying "how could we possibly get
> here??". It could also be written as die("BUG: unknown object type %d",
> obj->type), or even better: BUG(...).

Likewise.