Re: [PATCH 1/8] sequencer: introduce new commands to reset the revision

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jan 30, 2018, 07:12 UTC
Message-ID: <CAPig+cRjU6niXpT2FrDWZ0x1HmGf1ojVZj3uk2qXEGe-S7i_HQ@mail.gmail.com>
In-Reply-To: <nycvar.QRO.7.76.6.1801292146270.35@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz>

On Mon, Jan 29, 2018 at 3:50 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 22 quoted lines

> On Fri, 19 Jan 2018, Eric Sunshine wrote:
>> On Thu, Jan 18, 2018 at 10:35 AM, Johannes Schindelin
>> <johannes.schindelin@gmx.de> 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.

Show 6 quoted lines

> 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.