On Mon, Jan 29, 2018 at 3:50 PM, Johannes Schindelin
[off-list ref] wrote:

On Fri, 19 Jan 2018, Eric Sunshine wrote:

quoted

On Thu, Jan 18, 2018 at 10:35 AM, Johannes Schindelin
[off-list ref] wrote:

quoted

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