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