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

[Eli Zaretskii]

Paul,

I've reviewed the changes you pushed to master for fixing this bug,
and I must say that most of them look like purely stylistic changes,
to make the code more to your personal liking.  The actual bugs were
very few and minor, and didn't necessitate such thorough changes.

I think we should try to avoid such thorough changes due to style,
because they risk introducing regressions into code that was working
fine for many years.  This is especially true in the absence of test
coverage for the functionality of the code that gets refactored.

One thing I find suboptimal in the new code is that you removed all
the unibyte code, and instead rely on this:

  Lisp_Object str = Fstring_make_multibyte (string);

But string-make-multibyte doesn't necessarily produce a multibyte
string, e.g.:

  (multibyte-string-p (string-make-multibyte "abcd"))
    => nil

So without any comments as to why we handle the input string as
multibyte for the rest of the function, I think this will confuse
someone down the road.

More importantly, I think the refactoring already introduced a
regression.  On the emacs-25 branch we have:

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

But on master:

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

I think that's because this chunk of code from the original
implementation disappeared without a trace:

          int len;
          int ch = STRING_CHAR_AND_LENGTH (strp, len);
          if ((ch == LEFT_SINGLE_QUOTATION_MARK
               || ch == RIGHT_SINGLE_QUOTATION_MARK)
              && quoting_style != CURVE_QUOTING_STYLE)
            {
              *bufp++ = ((ch == LEFT_SINGLE_QUOTATION_MARK
                          && quoting_style == GRAVE_QUOTING_STYLE)
                         ? '`' : '\'');
              strp += len;
              changed = true;
            }

Once again, we should have a test covering functionality before we
attempt such refactoring.

Or maybe we should just go to the original code, after fixing the
immediate bugs, as I proposed in a patch yesterday?

Thanks.