On Tue, Mar 12, 2024 at 02:36:36PM +0000, phillip.wood123@gmail.com wrote:

quoted

Leading whitespace actually does work, though I think you'd be slightly
insane to use it.

For "git rebase" in only works if you edit the todo list with "git rebase
--edit-todo" which calls strbuf_stripspace() and therefore parse_insn_line()
never sees the comments. If you edit the todo list directly then it will
error out. You can see this with

    git -c core.commentChar=' ' rebase -x 'echo " this is a comment"

quoted

quoted

"$(git rev-parse --git-path rebase-merge/git-rebase-todo)"' HEAD^

which successfully picks HEAD but then gives

    error: invalid command 'this'

when it tries to parse the todo list after the exec command is run. Given it
is broken already I'm not sure we should worry about it here. In any case it
is not clear how much we should worry about problems caused by users editing
the todo list without using "git rebase --edit-todo". There is code in
parse_insn_line() which is clearly there to handle direct editing of the
file but I don't think it is tested and directly editing the file probably
bypasses the rebase.missingCommitsCheck checks as well.

Ah, thanks for the example. I guess it's not too surprising that it can
cause confusion. Given that it's an existing issue, I think my
preference would be to leave it out of the series under discussion
(given how long and complicated it is already), but I'd have no
objection to tightening things further on top as a separate series.

-Peff