Re: [PATCH v2 2/7] bisect--helper: reimplement `bisect_replay` shell function in C
- From
Rafael Silva <rafaeloliveira.cs@gmail.com>
- Date
- Dec 23, 2020, 10:03 UTC
- Message-ID
- <gohp6k35zw6drc.fsf@cpm12071.fritz.box>
- In-Reply-To
- <xmqq4kkdfgln.fsf@gitster.c.googlers.com>
Junio C Hamano writes:
Show 28 quoted lines
> 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