Date: Sun, 25 Jul 1999 10:58:11 +0300 (IDT)
From: Eli Zaretskii <eliz AT is DOT elta DOT co DOT il>
X-Sender: eliz AT is
To: "Mark E." <snowball3 AT bigfoot DOT com>
cc: djgpp-workers AT delorie DOT com
Subject: Re: Current diffs against Readline 4.0
In-Reply-To: <199907102011.UAA247490@out1.ibm.net>
Message-ID: <Pine.SUN.3.91.990725105729.27488B-100000@is>
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Reply-To: djgpp-workers AT delorie DOT com
X-Mailing-List: djgpp-workers AT delorie DOT com
X-Unsubscribes-To: listserv AT delorie DOT com
Precedence: bulk


On Sat, 10 Jul 1999, Mark E. wrote:

> Attached is a zip archive containing  my current diffs against Readline 
> 4.0. Let me know about any korrections, changes, etc.

I finally found the time to look at these patches.  Here are some
minor comments to some of the patches:

1. complete.c:

    + #if defined (HAVE_PATHNAME_STYLE_DOS)
    +   if (!temp && rl_filename_completion_desired
    +       && (isalpha (pathname[0]) && pathname[1] == ':'))
    +       temp = pathname + 1;
    + #endif

Use of `isalpha' when checking for the drive letter is not 100% safe:
DOS allows the 6 characters between upper-case Z and lower-case a to
be used as drive letters as well (since there can be a maximum of 32
block devices, but only 26 letters).  This allowance is actually used
by Novell redirectors.  So I suggest simply to lose `isalpha' in
fragments like the above.  It is not really required here; but if you
want to play it safe, check the drive letter to be in the [A-z] range.

2.bind.c:

    + #if defined (HAVE_SETMODE) && defined(O_TEXT)
    +   setmode (file, O_TEXT);
    + #endif

Is it safe to read .inputrc in text mode?  Can we be sure that no
strange unprintable characters will ever be there, in particular in
the key-binding part?

One problem with text-mode reads is that it removes *all* CR
characters, not only those immediately preceding the LF.  So, for
example, if somebody puts a bare CR character into their bindings,
they are in for a surprise...

For these reasons, I have chosen in my patches to use binary reads and
strip the CRs from CR-LF pairs manually.  Perhaps I was too
paranoiac...

3. terminal.c:

    - #if defined (__GO32__)
    -   screenwidth = ScreenCols ();
    -   screenheight = ScreenRows ();
    -   screenchars = screenwidth * screenheight;
    -   term_cr = "\r";
    -   term_im = term_ei = term_ic = term_IC = (char *)NULL;
    -   term_up = term_dc = term_DC = visible_bell = (char *)NULL;
    - 
    -   /* Does the __GO32__ have a meta key?  I don't know. */
    -   term_has_meta = 0;
    -   term_mm = term_mo = (char *)NULL;

I'm probably missing something here--how is it supposed to work, after
you remove all this?  The usual code uses termcap functions like
tgetnum etc., which we don't have.

    - #if !defined (__GO32__)
	if (term_backspace)
	  for (i = 0; i < count; i++)
	    tputs (term_backspace, 1, _rl_output_character_function);
	else
    - #endif /* !__GO32__ */

Same here: this will call tputs to backspace the cursor.

4. histfile.c:

    + #if defined (HAVE_SETMODE) && defined (O_TEXT)
    +   setmode (file, O_TEXT);
    + #endif

Same comment as for .inputrc: are we sure that everything that goes to
the history file is printable text with no strange characters?  If
not, using binary mode and manually stripping the CRs is safer.

    ! #if defined (HAVE_SETMODE) && defined (O_TEXT)
    !       setmode (file, O_TEXT);
    ! #endif
    !       write (file, buffer + i, chars_read - i);

Why do you need to do this before writing history?  A file written in
binary mode will be read successfully in text mode, so this shouldn't
be necessary.

    + #if !defined (O_TEXT)
	mode = overwrite ? O_WRONLY|O_CREAT|O_TRUNC|O_BINARY : O_WRONLY|O_APPEND|O_BINARY;
    + #else
    +   mode = overwrite ? O_WRONLY|O_CREAT|O_TRUNC : O_WRONLY|O_APPEND;
    +   mode |= O_TEXT;
    + #endif
	output = history_filename (filename);

Same here.

I think `history_truncate_file' and `history_do_write' also need a
patch that handles the case of the initial dot in the history file
name.  My patches included it.

5. display.c:

      void
      _rl_clear_screen ()
      {
    - #if !defined (__GO32__)
	if (term_clrpag)
	  tputs (term_clrpag, 1, _rl_output_character_function);
	else
    - #endif /* !__GO32__ */
	  crlf ();
      }

This and other patches in display.c seem to require termcap functions
which we don't have.  See my comments under terminal.c above.