Re: [PATCH RFC 01/24] ref-filter: get rid of goto

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 26, 2018, 20:19 UTC
Message-ID: <xmqqfu6s74pf.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <0102016133ff3a86-44d354ec-13c6-4c38-bc75-1ba4422db5a7-000000@eu-west-1.amazonses.com>

Olga Telezhnaya <olyatelezhnaya@gmail.com> writes:

Show 6 quoted lines

> Get rid of goto command in ref-filter for better readability.
>
> Signed-off-by: Olga Telezhnaia <olyatelezhnaya@gmail.com>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored by: Jeff King <peff@peff.net>
> ---

How was this patch "mentored by" these two folks? Have they already reviewed and gave you OK, or are you asking them to also help reviewing with this message? Mostly just being curious.

It is not convincning that this splitting the last part of a single function into a separate helper function that is called from only one place improves readability. If better readability is the purpose, I would even say

         for (i = 0; i < used_atom_cnt; i++) {
		if (...)
-			goto need_obj;
+			break;
	}
-	return;
+	if (used_atom_cnt <= i)
		return;

-need_obj:

would make the result easier to follow with a much less impact.

If we later in the series will use this new helper function from other places, it certainly makes sense to create a new helper like this patch does, but then "get rid of goto for readability" is not the justification for such a change.

Show 12 quoted lines

>  ref-filter.c | 103 ++++++++++++++++++++++++++++++-----------------------------
>  1 file changed, 53 insertions(+), 50 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index f9e25aea7a97e..37337b57aacf4 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1354,16 +1354,60 @@ static const char *get_refname(struct used_atom *atom, struct ref_array_item *re
>  	return show_ref(&atom->u.refname, ref->refname);
>  }
>  
> +static void need_object(struct ref_array_item *ref) {

Style. The opening brace at the beginning of the function sits on its own line alone.