bug#24206: 25.1; Curly quotes generate invalid strings, leading to a segfault

[Eli Zaretskii <eliz@gnu.org>]

> Cc: johnw@gnu.org, 24206@debbugs.gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Wed, 17 Aug 2016 13:52:42 -0700
> 
>         Some change in this area was needed because the 'multibyte' flag went away.
> 
>     Only because you removed it.  You could have left it alone, it would
>     have worked
> 
> Sure, but it was no longer necessary, as the code no longer needs to record whether the original string was multibyte. Keeping an unnecessary variable around would make the code harder to read.

The code that got removed was the easy and intuitive part: it dealt
with processing single-byte strings one byte at a time.  The
hard-to-read part of the code is still with us.  We have less 'if'
conditionals, but that's hardly the main complication in the original
code.

>     even after the call to Fstring_make_multibyte, for the
>     reasons I explained earlier: the result is not necessarily a multibyte
>     string.
> 
> That doesn't affect the fact that the 'multibyte' variable is no longer necessary. In emacs-25, 'multibyte' does not mean that the result is a multibyte string

You are missing my point: the code on master now processes a string,
that could be either unibyte or multibyte, using only multibyte
methods.  With the flag in place, each kind of string would have used
the method that's natural with it.  The way things are now, one has to
think hard about what the code does to convince oneself it's valid.

>     I don't see why it is tricky, we do that in Emacs in other places.
>
> Really? A call to STRING_CHAR_AND_LENGTH followed by a length test followed by a call to memcpy for length > 1 and a special case inline copy for length == 1? When copying multibyte data? Where else does Emacs do that?

What exactly confuses you in that snippet?  The call to
STRING_CHAR_AND_LENGTH itself? we have that in umpteen other places.
The single-byte optimization of not calling memcpy?  That's standard
practice in C.  If you need an example for using
STRING_CHAR_AND_LENGTH while copying text, you can find it in
copy_text, for example.  I really don't understand what's your problem
with that code.

>     it's more clear for you
> 
> Replacing 14 unusually and unnecessarily tricky lines with zero lines should help clarify things for most readers.

They are not unusually tricky at all.  And you replaced it with a
fall-through, which is harder to follow and easier to introduce subtle
bugs.

>     I could simply revert your commit, it would have saved us both quite
>     some time.  Would you prefer that?
> 
> It'd be even simpler to leave things alone, as the master code works better than emacs-25 does.

Sorry, leaving alone changes that I find questionable or gratuitous is
not in the job description.

>         Alan wanted something that he could put into his .emacs that would cause
>         (message PERCENTLESS) to output the string PERCENTLESS as-is, assuming
>         PERCENTLESS lacks %. This was the point of his original bug report; his original
>         example involved ` and ' but he wanted the same behavior for ‘ and ’, a point
>         that became clear during the discussion of Bug#23425.
> 
>     Then why not for '..' as well?  How is that different from ‘..’?
> 
> It's not different. Alan wanted the same behavior for '..', and he got that too.

But the behavior is not the same:

  (let ((text-quoting-style 'curve))
    (substitute-command-keys "'foo'"))
      => ’foo’

but

  (let ((text-quoting-style 'grave))
    (substitute-command-keys "‘foo’"))
      => ‘foo’

I would have expected the first example to yield 'foo', i.e. leave the
apostrophes alone, as we do with curved quotes in the second example.
What we have now is inconsistent, and its rationale evades me.