Re: [PATCH v2 5/5] ref-filter: get_ref_atom_value() error handling
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 15, 2018, 21:05 UTC
- Message-ID
- <xmqqd1053v8c.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <20180315204709.1900787-1-martin.agren@gmail.com>
Martin Ågren <martin.agren@gmail.com> writes:
Show 17 quoted lines
>> 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.
Show 13 quoted lines
>> 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.