Re: [PATCH] sequencer: clarify intention to break out of loop

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 29, 2018, 03:43 UTC
Message-ID: <xmqqva5l76zs.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <CAPig+cR28OBA0rwy=X6XU0cjstAguvUiLU9Zxjvg2X7=UsB5qw@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 25 quoted lines

>> diff --git a/sequencer.c b/sequencer.c
>> @@ -2849,10 +2849,14 @@ static int do_reset(const char *name, int len, struct replay_opts *opts)
>>                 /* Determine the length of the label */
>> +               for (i = 0; i < len; i++) {
>> +                       if (isspace(name[i])) {
>>                                 len = i;
>> +                               break;
>> +                       }
>> +               }
>>                 strbuf_addf(&ref_name, "refs/rewritten/%.*s", len, name);
>
> At least for me, this would be more idiomatic if it was coded like this:
>
>     for (i = 0; i < len; i++)
>         if (isspace(name[i]))
>             break;
>     strbuf_addf(..., i, name);
>
> or, perhaps (less concise):
>
>     for (i = 0; i < len; i++)
>         if (isspace(name[i]))
>             break;
>     len = i;
>     strbuf_addf(..., len, name);

Yup, the former is more idiomatic between these two; the latter is also fine.

The result of Martin's patch shares the same "Huh?" factor as the original that mucks with the "supposedly constant" side of the termination condition, and from that point of view, it is not that much of a readability improvement, I would think.