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

[Rafael Silva <rafaeloliveira.cs@gmail.com>]

Junio C Hamano writes:

> Rafael Silva <rafaeloliveira.cs@gmail.com> writes:
>
>> 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.

That's a good point (and nice explanation, by the way). Before I was
thinking more on the line "while we do not found a good line from
process_line() and we do not finish processing the file, let's go to
the next line" which lead me to proposed changes for shorten the code.

However, after your explanation, I can see now and agree the original
does seems easier to follow and we can as it is.

-- 
Thanks
Rafael