2018-02-15 8:16 GMT+03:00 Jeff King :
> On Mon, Feb 12, 2018 at 08:08:54AM +, Olga Telezhnaya wrote:
>> Add return flag to format_ref_array_item(), show_ref_array_item(),
>> get_ref_array_info() and populate_value() for further using.
>> Need it to handle situations when item is broken but we can not invoke
>> die() because we are in batch mode and all items need to be processed.
> OK. The source of these errors would eventually be calls in
> populate_value(), but we don't flag any errors there yet (well, we do,
> but they all end up in die() for now). So I'd expect to see later in the
> series those die() calls converted to errors (I haven't looked further
> yet; just making a note to myself).
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -1356,8 +1356,9 @@ static const char *get_refname(struct used_atom *atom,
>> struct ref_array_item *re
>> * Parse the object referred by ref, and grab needed value.
>> + * Return 0 if everything was successful, -1 otherwise.
> We discussed off-list the concept that the caller may want to know one
> of three outcomes:
> - we completed the request, having accessed the object
> - we completed the request, but it didn't require accessing any
> - an error occurred accessing the object
> Since callers like "cat-file" would need to check has_sha1_file()
> manually in the second case. Should this return value actually be an
> enum, which would make it easier to convert later to a tri-state?
I decided not to implement this particular scenario because all other
callers are waiting that everything will be printed inside ref-filter.
We just add support for cat-file there. I don't think that I need to
re-think all printing process and move printing logic to all other
callers so that cat-file will behave fine. In my opinion, in the final
version cat-file must accept all ref-filter logic parts and adapt to
>> -static void populate_value(struct ref_array_item *ref)
>> +static int populate_value(struct ref_array_item *ref)
>> void *buf;
>> struct object *obj;
>> @@ -1482,7 +1483,7 @@ static void populate_value(struct ref_array_item *ref)
>> if (used_atom_cnt <= i)
>> - return;
>> + return 0;
> Most of these conversions are obviously correct, because they just turn
> a void return into one with a value. But this one is trickier:
>> @@ -2138,9 +2144,10 @@ void format_ref_array_item(struct ref_array_item
>> ep = strchr(sp, ')');
>> if (cp < sp)
>> append_literal(cp, sp, );
>> - get_ref_atom_value(info,
>> -parse_ref_filter_atom(format, sp + 2, ep),
>> + if (get_ref_atom_value(info,
>> +parse_ref_filter_atom(format, sp + 2,
>> + return -1;
>> atomv->handler(atomv, );
> since it affects the control flow. Might we be skipping any necessary
> cleanup in the function if we see an error?
> It looks like we may have called push_stack_element(), but we'd never
> get to the end of the function where we call pop_stack_element(),
> causing us to leak.
Agree, I will fix this.