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

From: Junio C Hamano <[email-masked]>
Date: Wed, 24 Jan 2018 14:01:44 -0800

Eric Sunshine <[email-masked]> writes:

>> +static int do_reset(const char *name, int len)
>> +{
>> +       [...]
>> +       if (hold_locked_index(&lock, LOCK_REPORT_ON_ERROR) < 0)
>> +               return -1;
>> +
>> +       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?

I must be missing the same thing.  Given that the callers of
do_reset(), other than the "bug" thing that passes the hard coded
"onto", uses item->arg/item->arg_len which includes everything after
the insn word on the line in the todo list, I do suspect that the
intention is to stop at the first whitespace char to avoid creating
a ref with whitespace in it, i.e. it is a bug that can be fixed with
s/len = i/break/.

The code probably should further check the resulting string with
check_ref_format() to detect strange chars and char sequences that
make the resulting refname invalid.  For example, you would not want
to allow a label with two consecutive periods in it.