On Mon, Jan 29, 2018 at 3:50 PM, Johannes Schindelin
<Johannes.Schindelin@xxxxxx> wrote:
> On Fri, 19 Jan 2018, Eric Sunshine wrote:
>> On Thu, Jan 18, 2018 at 10:35 AM, Johannes Schindelin
>> <johannes.schindelin@xxxxxx> wrote:
>> > +static int do_reset(const char *name, int len)
>> > +{
>> > + for (i = 0; i < len; i++)
>> > + if (isspace(name[i]))
>> > + len = i;
>>
>> What is the purpose of this loop? I could imagine that it's trying to
>> strip all whitespace from the end of 'name', however, to do that it
>> would iterate backward, not forward. (Or perhaps it's trying to
>> truncate at the first space, but then it would need to invert the
>> condition or use 'break'.) Am I missing something obvious?
>
> Yes, you are missing something obvious. The idea of the `reset` command is
> that it not only has a label, but also the oneline of the original commit:
>
> reset branch-point sequencer: prepare for cleanup
>
> In this instance, `branch-point` is the label. And for convenience of the
> person editing, it also has the oneline.
No, that's not what I was missing. What I was missing was that
assigning 'i' to 'len' also causes the loop to terminate. It's
embarrassing how long I had to stare at this loop to see that, and I
suspect that's what fooled a couple other reviewers, as well, since
idiomatic loops don't normally muck with the termination condition in
quite that fashion (and is why I suggested that a 'break' might be
missing).
Had the loop been a bit more idiomatic:
for (i = 0; i < len; i++)
if (isspace(name[i]))
break;
len = i;
then the question would never have arisen. Anyhow, it's a minor point
in the greater scheme of the patch series.
> In the Git garden shears, I separated the two arguments via `#`:
>
> reset branch-point # sequencer: prepare for cleanup
>
> I guess that is actually more readable, so I will introduce that into this
> patch series, too.
Given my termination-condition blindness, the extra "#" would not have
helped me understand the loop any better.
Having now played with the feature a tiny bit, I don't have a strong
opinion about the "#" other than to note that it seems inconsistent
with other commands which don't use "#" as a separator.
