From: phillip.wood123@gmail.com
Date: Tue, 12 Mar 2024 14:36:36 GMT
Subject: Re: [PATCH 11/15] find multi-byte comment chars in unterminated buffers
Message-ID: <9b63eab9-7b6d-4024-9ec4-b58c5ca3b084@gmail.com>
In-Reply-To: <20240312081931.GC47852@coredump.intra.peff.net>

Hi Peff

On 12/03/2024 08:19, Jeff King wrote:
> On Fri, Mar 08, 2024 at 04:20:12PM +0000, Phillip Wood wrote:
>> 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).
> 
>>> 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