Re: [PATCH v2 3/5] ref-filter: change parsing function error handling

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 15, 2018, 22:48 UTC
Message-ID: <xmqq8tat3qgj.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <0102016225e61cb4-b59773de-d22e-42cd-909a-d7ce71646898-000000@eu-west-1.amazonses.com>

Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:

> Continue removing any printing from ref-filter formatting logic,
> so that it could be more general.

Hmm.

> Change the signature of parse_ref_filter_atom() by changing return value,
> adding previous return value to function parameter and also adding
> strbuf parameter for error message.

This says what the patch changes, but it does not explain why it is a good idea to return something with different meaning to the callers (which of course forces us to update all callers so that they pass &result pointer and check the value returned in the variable), or more importantly what meaning the return value has and how the callers are expected to use it. While at it, it probably is a good idea to explain what the original return value means.

	The return value from parse_ref_filter_atom() used to be the
	position the atom is found in the used_atom[] array; that
	information is now returned in an integer pointed at by the
	*res parameter.  The function now returns 0 for success and
	-1 for failure.

or something like that.

Having said that, I wonder if a calling convention that does not force callers to pass in a pointer-to-int may make more sense. Because the original return value is an index into an array, we know the normal return values are not negative. An updated caller could become like this instead:

	pos = parse_ref_filter_atom(format, atom, ep, &err);
	if (pos < 0)
		die("%s", err.buf);
	... original code that used 'pos' can stay as before ...