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

Junio C Hamano [off-list ref] writes:

Matthieu Moy [off-list ref] writes:

quoted

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/