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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 24, 2018, 22:01 UTC
Message-ID: <xmqqa7x2c3w7.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAPig+cQbG2s-LrAo9+7C7=dXifbWFJ3SzuNa-QePHDk7egK=jg@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 15 quoted lines

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