Re: [PATCH v5 4/6] ref-filter: change parsing function error handling

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 21, 2018, 20:36 UTC
Message-ID: <xmqqh8p9np2a.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <0102016249d21c96-29f058b0-a43d-45b7-9c1d-5ea2382858a2-000000@eu-west-1.amazonses.com>

Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:

Show 19 quoted lines

> @@ -2144,13 +2151,15 @@ int format_ref_array_item(struct ref_array_item *info,
>  
>  	for (cp = format->format; *cp && (sp = find_next(cp)); cp = ep + 1) {
>  		struct atom_value *atomv;
> +		int pos;
>  
>  		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);
> +		pos = parse_ref_filter_atom(format, sp + 2, ep, error_buf);
> +		if (pos < 0)
> +			return -1;
> +		get_ref_atom_value(info, pos, &atomv);
>  		if (atomv->handler(atomv, &state, error_buf))
>  			return -1;
>  	}

These error returns leave the formatting state "state" on the stack holding onto its resources, no?

The only thing the caller of format_ref_array_item() that notices an error return does is to die even after this series, so in that sense it does not matter (yet), but it still feels somewhat wrong.