Hi Christian & Michael,
Michael McConville wrote on Thu, Dec 24, 2015 at 04:19:03PM -0500:
> Christian Heckendorf wrote:
>> A couple of somewhat recent changes in NetBSD's libedit permit
>> el_gets(3) to accept multibyte characters if the locale supports
>> it.
Ugh. The amount of indirection in that code is disturbing, and the
amount of contortion is disgusting. Such stuff is highly error
prone, in particular since the manual is way below OpenBSD standards
(functions mentioned in the SYNOPSIS but not actually documented,
vague statements, confusion regarding bytes versus characters, ...).
Besides, it's not completely clear which parts of the interface are
public and which are internal to the library.
In the vicinity of this particular diff: The IGNORE_EXTCHARS flag
appears to be private to the library, the users seems to have no
way to change it. Otherwise, the existing code in el_getc(3)
would already be broken because it clears the flag on exit even
if it was set on entry. But as an internal flag, it's completely
pointless. If CHARSET_IS_UTF8 is set, the present diff makes
sure it is never set. If CHARSET_IS_UTF8 is not set, it has no
effect because the only place where it is used also checks "bytes > 1",
which cannot happen in the C locale.
But if we want to stay in sync with upstream, freely borrowing Bob's
whale flensing knife may not be the best idea.
Also note that el_gets(3) is documented to return the number of
characters read, but actually, callers assume it returns the
number of *bytes* read, so what your diff is doing makes sense.
Michael, as you already looked at NetBSD, is there a documentation
update to go with this diff?
>> A notable user of this function is sftp(1) which will allow users to
>> input multibyte characters, in filenames for example, once the diff
>> is applied.
I inspected your diff and found it to be both correct and in line
with the existing (horrible) style. In particular, it is very
similar to the code already used in el_getc(3). I also tested
it with ftp(1) - adding setlocale(LC_CTYPE, "") to that program,
though it's quite unclear where exactly that call should go, so
no diff for that just yet.
But i would like to commit this eln.c diff, if i get any OK for it.
> I remember someone recently mentioning that libedit is due for an
> update. Maybe we should try to include this in a full sync.
Sure. But let's not deny focussed, non-intrusive diffs that make
specific, well-defined things better just because there are rumours
about grand plans. Besides, cherry-picking from upstream hardly
complicates later merges.
OK?
Yours,
Ingo
>> Index: eln.c
>> ===================================================================
>> RCS file: /cvs/src/lib/libedit/eln.c,v
>> retrieving revision 1.4
>> diff -u -p -r1.4 eln.c
>> --- eln.c 20 May 2014 11:59:03 -0000 1.4
>> +++ eln.c 24 Dec 2015 19:34:09 -0000
>> @@ -74,9 +74,18 @@ el_gets(EditLine *el, int *nread)
>> {
>> const wchar_t *tmp;
>>
>> - el->el_flags |= IGNORE_EXTCHARS;
>> + if (!(el->el_flags & CHARSET_IS_UTF8))
>> + el->el_flags |= IGNORE_EXTCHARS;
>> tmp = el_wgets(el, nread);
>> - el->el_flags &= ~IGNORE_EXTCHARS;
>> + if (tmp != NULL) {
>> + size_t nwread = 0;
>> + int i;
>> + for (i = 0; i < *nread; i++)
>> + nwread += ct_enc_width(tmp[i]);
>> + *nread = (int)nwread;
>> + }
>> + if (!(el->el_flags & CHARSET_IS_UTF8))
>> + el->el_flags &= ~IGNORE_EXTCHARS;
>> return ct_encode_string(tmp, &el->el_lgcyconv);
>> }
>>
>>
