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

> Eric Sunshine <sunshine@sunshineco.com> 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.