Re: [PATCH] rebase: accept indented comments (fixes regression)

[Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>]

Junio C Hamano <gitster@pobox.com> writes:

> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
>
>> With Git <2.0.6, 'git rebase' used to accept lines starting with
>> whitespaces followed with '#' as a comment. This was broken by
>> 804098b (git rebase -i: add static check for commands and SHA-1,
>> 2015-06-29), which introduced additional checks on the TODO-list using
>> "git stripspaces" which only strips comments starting at the first
>> column.
>
> I cannot help thinking that this is sidestepping the real issue.
>
> The real issue, I think, is that the new check tokenises the input
> differently from how the expand_todo_ids -> transform_todo_ids
> callchain parses it.  The problem Nazri Ramliy noticed about the new
> check that does not ignore the indentation is merely one aspect of
> it.

Right.

> Stripping leading whilespaces with sed may ignore indented anything
> and help Nazri's script, but 804098b tightened checks to disallow
> other things that we historically allowed, e.g. if you replace SP
> between "pick" and the commit object name with an HT, the new check
> will not notice that HT is also a perfectly good token separator
> character and barfs.

Indeed. I'm adding a test for that too.

> I am actually tempted to say that we should revert 804098b, which is
> the simplest fix.

I think the commit has value, and reverting it makes the "drop" command
essentially useless.

> As far as I can tell, the hand-rolled parsing is there only in oder
> to report the incoming $line as-is.

Indeed, I remember finding the parsing code weird when I reviewed it,
and the reason was to provide the exact line.

> It is much easier to just identify with which line number the location
> of the problem, and show it when it is necessary from the original
> source, and we do not care about performance in the error codepath.

Agreed.

> Perhaps something along these lines instead, with your new tests
> added in?

Sounds good, yes. I'll send a patch with this and my updated tests.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/