Re: [Nmh-workers] GNU readline whatnowproc (was tab completion)

[Peter Maydell]

address@hidden wrote:
>Enclosed please find a good patch for whatnowproc readine.

Thanks. This doesn't actually apply to current head of git,
mostly because we removed the CVS keyword-substitution lines
when we moved over, but also the configure.in patch doesn't
apply any more because the context has changed. It would
probably be a good idea to rebase it at some point.

Also, given that we basically have all the information about
accepted switches in the ansp parameter, it would be cool if we
could pass that along to readline so tab completion completed
commands rather than just filenames. This in particular would
mean you'd just want to always have getans() tab complete,
because it wouldn't tab-complete filenames unless it was
appropriate. That's more work than just calling readline(),
though. 

Having actually tried this patch, I think that would make this
patch a lot more useful -- at the moment it's a bit of a weird
easter egg that happens to be useful in one subcase of whatnow
usage. I'm a bit dubious about applying this with the barebones
functionality it has now.

Anyway, some nitpicking on points of detail:

> > >+    char *ans, **cpp;
> > >+
> > >+    for (;;) {
> > >+        ans = readline (prompt);
> > >+        if (ans[0] == '?' || ans[0] == 0 ) {
> > >+            printf ("Options are:\n");
> > >+            print_sw (ALL, ansp, "");
> > >+            free(ans);
> > >+      continue;
> > >+        }
> > >+  strcpy (ansbuf,ans); /* not sure why--but it makes brkstring() work */
> > >+        cpp = brkstring (ansbuf, " ", NULL);
> > 
> > ...so what happens if you don't strcpy but just pass ans to
> > brkstring()? I can't see anything in brkstring() that cares,
> > and it would be nice to avoid the fixed ansbuf[] buffer.
>
>Sorry, my comment was still in "debug tracing" mode.  The problem isn't with
>brkstring().  The problem is that readline() mallocs the char * it returns
>and expects it to be free'ed.  I assume that not free'ing ans might lead to
>a memory leak or something worse.

Yes, but brkstring() doesn't do anything like freeing the buffer
it manipulates. It just rewrites the string to put NULs in instead
of spaces, and sets up an array of pointers into the string.
And smatch() doesn't care either. So I still don't see why you need
the strcpy() -- you still have to free() the string from readline()
in the same places whether you pass it or a copy of it to brkstring().

>--- configure.in.orig  2008-05-21 11:44:16.000000000 -0500
>+++ configure.in       2010-12-20 08:49:09.000000000 -0600
>@@ -43,6 +43,21 @@
> dnl CHECK COMMAND LINE OPTIONS
> dnl --------------------------
> 
>+dnl Do you want whatnow to use readline()?
>+dnl (e.g. enable tab completion and editor support)
>+AC_ARG_WITH([readline],
>+  [AS_HELP_STRING([--with-readline],
>+    [enable whatnow readline() (e.g. enable tab completion and editor 
>support)])],
>+  [],
>+  [with_readline=no])

I would prefer the default to be "use readline if it is
present at compile time", rather than "do not use". Otherwise
most users (and probably most distro package builders!) will
never realise the option is there.

>+#if HAVE_LIBREADLINE
>+char **
>+getans_via_readline (char *prompt, struct swit *ansp)
>+{

So now some uses of getans() are just getans() and some
are getans_via_readline(). I would rather that we have a
function name which says what sort of getans() you're trying
to do, and then callers all use whatever the right one is.
And then in the implementation of the fancy getans(), if we
don't HAVE_LIBREADLINE we fall back to implementing it via
plain old getans(). [Question: when do we want the fancy
vs non fancy getans? Maybe there should just be one, and
we make getans() do tab completion if we have readline?]

That avoids introducing HAVE_LIBREADLINE ifdefs at all
the callers, which is messy.

>+        ans = readline (prompt);
>+        if (ans[0] == '?' || ans[0] == 0 ) {

The manpage says readline() can return NULL, which would
segfault here. (Try pressing ^D at a whatnow prompt.)

>+            printf ("Options are:\n");
>+            print_sw (ALL, ansp, "");

This doesn't compile (you forgot the 'stdout' param):
getans.c:85: error: too few arguments to function 'print_sw'

>--- uip/Makefile.in.orig       2005-12-24 11:17:38.000000000 -0600
>+++ uip/Makefile.in    2010-12-20 08:35:24.000000000 -0600
>@@ -30,6 +30,7 @@
> SASL_LIBS = @SASL_LIBS@
> HESIOD_LIBS = @HESIOD_LIBS@
> NDBM_LIBS = @NDBM_LIBS@
>+READLINE_LIBS = @LIBREADLINE@
> LOCALLIBS = ../config/version.o ../config/config.o $(MTSLIB) ../sbr/libmh.a
> LINKLIBS = $(LOCALLIBS) $(KRB4_LIBS) $(SASL_LIBS) $(HESIOD_LIBS) $(LIBS)
> 
>@@ -114,14 +115,14 @@
> burst: burst.o $(LOCALLIBS)
>       $(LINK) burst.o $(LINKLIBS)
> 
>-comp: comp.o whatnowproc.o whatnowsbr.o sendsbr.o annosbr.o distsbr.o 
>$(LOCALLIBS)
>-      $(LINK) comp.o whatnowproc.o whatnowsbr.o sendsbr.o annosbr.o distsbr.o 
>$(LINKLIBS)
>+comp: comp.o whatnowproc.o whatnowsbr.o sendsbr.o annosbr.o distsbr.o 
>$(LOCALLIBS) 
>+      $(LINK) comp.o whatnowproc.o whatnowsbr.o sendsbr.o annosbr.o distsbr.o 
>$(LINKLIBS) $(READLINE_LIBS)

[etc]

I think it would be simpler just to put $(READLINE_LIBS) into LINKLIBS
the way we do with the other optional libraries -- once we've built
key bits of nmh like comp against libreadline we might as well just
link everything against it. And it avoids weird effects later if we
use it in more places than just whatnow.

>@@ -243,7 +243,11 @@
> 
>     snprintf (prompt, sizeof(prompt), myprompt, invo_name);
>     for (;;) {
>+#ifndef HAVE_LIBREADLINE
>       if (!(argp = getans (prompt, aleqs))) {
>+#else
>+      if (!(argp = getans_via_readline (prompt, aleqs))) {
>+#endif
>           unlink (LINK);
>           done (1);
>       }

See my remarks above about getting rid of this ifdeffery.

-- PMM