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

Eric Sunshine [off-list ref] writes:

quoted

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