Re: [PATCH v2 2/7] bisect--helper: reimplement `bisect_replay` shell function in C

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 23, 2020, 01:36 UTC
Message-ID: <xmqq4kkdfgln.fsf@gitster.c.googlers.com>
In-Reply-To: <gohp6klfdpbca4.fsf@cpm12071.fritz.box>

Rafael Silva <rafaeloliveira.cs@gmail.com> writes:

Show 14 quoted lines

> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index b887413d8d..fb15587af8 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -988,11 +988,8 @@ static int process_replay_file(FILE *fp, struct bisect_terms *terms)
>         struct strbuf word = STRBUF_INIT;
>         int res = 0;
>
> -       while (strbuf_getline(&line, fp) != EOF) {
> +       while (!res && strbuf_getline(&line, fp) != EOF)
>                 res = process_line(terms, &line, &word);
> -               if (res)
> -                       break;
> -       }

I do not mind shorter and crisper code, but I somehow find that the original more cleanly expresses the intent.

"We'll grab input lines one by one until the input runs out" and "we stop when we see a line that process_line() likes" are conditions that the loop may stop at two logically distinct levels. You can conflate them into a single boolean, making it "unless we found a line the process_line() liked in the previous round, grab the next line but stop when we ran out the input", and it may make the result shorter, but it may be easier to follow by normal readers if we kept them separate, like the original does.