git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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
Previous: Junio C HamanoNext: Junio C Hamano
Message 14 of 18 in “Finish converting git bisect to C part 3”
  1. 0/7 Finish converting git bisect to C part 3Miriam Rubio, Dec 21, 2020
  2. 5/7 bisect--helper: retire `--bisect-auto-next` subcommandMiriam Rubio, Dec 21, 2020
  3. 6/7 bisect--helper: reimplement `bisect_skip` shell function in CMiriam Rubio, Dec 21, 2020
  4. Johannes SchindelinJan 18, 2021
  5. 1/7 bisect--helper: reimplement `bisect_log` shell function in CMiriam Rubio, Dec 21, 2020
  6. Johannes SchindelinJan 18, 2021
  7. 3/7 bisect--helper: retire `--bisect-write` subcommandMiriam Rubio, Dec 21, 2020
  8. 7/7 bisect--helper: retire `--check-and-set-terms` subcommandMiriam Rubio, Dec 21, 2020
  9. 4/7 bisect--helper: use `res` instead of return in BISECT_RESET case optionMiriam Rubio, Dec 21, 2020
  10. 2/7 bisect--helper: reimplement `bisect_replay` shell function in CMiriam Rubio, Dec 21, 2020
  11. Rafael SilvaDec 22, 2020
  12. Rafael SilvaDec 23, 2020
  13. Junio C HamanoDec 23, 2020
  14. Rafael SilvaDec 23, 2020
  15. Junio C HamanoDec 23, 2020
  16. Johannes SchindelinJan 18, 2021
  17. Johannes SchindelinJan 18, 2021
  18. Junio C HamanoJan 18, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.