2018-02-15 8:16 GMT+03:00 Jeff King <peff@xxxxxxxx>:
> On Mon, Feb 12, 2018 at 08:08:54AM +0000, 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
> objects
> - 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
them.
>
>> -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 *info,
>> ep = strchr(sp, ')');
>> if (cp < sp)
>> append_literal(cp, sp, &state);
>> - get_ref_atom_value(info,
>> - parse_ref_filter_atom(format, sp + 2, ep),
>> - &atomv);
>> + if (get_ref_atom_value(info,
>> + parse_ref_filter_atom(format, sp + 2, ep),
>> + &atomv))
>> + return -1;
>> atomv->handler(atomv, &state);
>> }
>
> 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.
>
> -Peff
