Re: [PATCH 11/15] find multi-byte comment chars in unterminated buffers

Hi Peff

On 12/03/2024 08:19, Jeff King wrote:

On Fri, Mar 08, 2024 at 04:20:12PM +0000, Phillip Wood wrote:

quoted

We could certainly leave it as-is and tell users they are only hurting
themselves if they complain when it does not work.

That was mostly my plan. To some degree I think this is orthogonal to my
series. You can already set core.commentChar to space or newline, and
I'm sure the results are not very good. Actually, I guess it is easy to
try:

   git -c core.commentChar=$'\n' commit --allow-empty

treats everything as not-a-comment.

Maybe it's worth forbidding this at the start of the series, and then
carrying it through. I really do think newline is the most special
character here, just because it's obviously going to be meaningful to
all of our line-oriented parsing. So you'll get weird results, as
opposed to broken multibyte characters, where things would still work if
you choose to consistently use them (and arguably we cannot even define
"broken" as the user can use a different encoding).

I agree newline is a special case compared to broken multibyte 
characters, I see you've disallowed it in v2 which seems like a good idea.

Likewise, I guess people might complain that their core.commentChar is
NFD and their editor writes out NFC characters or something, and we
don't match. I was hoping we could just punt on that and nobody would
ever notice (certainly I think it is OK to punt for now and somebody who
truly cares can make a utf8_starts_with() or similar).

quoted

quoted

Also, what exactly is the definition of "nonsense" will become can
of worms.  I can sympathise if somebody wants to use "#\t" to give
themselves a bit more room than usual on the left for visibility,
for example, so there might be a case to want whitespace characters.

That's fair, maybe we could just ban leading whitespace if we do decide to
restrict core.commentChar

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" 
 >>"$(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.

Best Wishes

Phillip

I'm currently using "! COMMENT !" (after using a unicode char for a few
days). It's horribly ugly, but I wanted to see if any bugs cropped up
(and vim's built-in git syntax highlighting colors it correctly ;) ).

-Peff