Hi Junio,
On Wed, 24 Jan 2018, Junio C Hamano wrote:
> Eric Sunshine <sunsh...@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.
The code already checks that by creating a ref.
No need to go crazy and validate ref names every time we parse the todo
list (which is quite often), eh?
Ciao,
Dscho
