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

From: Jeff King <peff@peff.net>
Date: Feb 15, 2018, 05:16 UTC
Message-ID: <20180215051658.GB18780@sigill.intra.peff.net>
In-Reply-To: <01020161890f4311-ab635cac-1d48-47ee-90e1-f178f134db1c-000000@eu-west-1.amazonses.com>

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

Show 8 quoted lines

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

Show 11 quoted lines

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

Show 13 quoted lines

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