Re: [PATCH v3 02/23] ref-filter: add return value to some functions

[Jeff King <peff@peff.net>]

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?

> -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.

-Peff