bugGNU Octave - Bugs: bug #60696, octave --interactive should not...

 
 

bug #60696: octave --interactive should not imply --no-line-editing when running in a terminal

Submitter:  Mike Miller <mtmiller>
Submitted:  Sat 29 May 2021 06:54:13 PM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * GNU/Linux
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Fri 23 Jul 2021 04:03:13 PM UTC, comment #12: 

Thanks for making this change.  I think the previous behavior may have been due to some incorrect edit or an unintended logic error in a previous change.

John W. Eaton <jwe>
Group administrator
Thu 22 Jul 2021 06:40:52 PM UTC, comment #11: 

No response, so I'm going to go ahead and push this change.  See http://hg.savannah.gnu.org/hgweb/octave/rev/da5cc930ff10.

Rik <rik5>
Group administrator
Fri 16 Jul 2021 09:16:34 PM UTC, comment #10: 

I thought this would be easy, but it turned out that one of the variables was declared 'const' so the patch required shadowing the app_context version of the "forced_interactive" state in a local variable.  In any case, the attached cset works for me.  Marking as Patch Submitted.

(file #51669)

Rik <rik5>
Group administrator
Tue 22 Jun 2021 09:12:35 PM UTC, comment #9: 

Do we want to make a change?  Seems like this bug report could be resolved easily if we want to use logic from comment #3 from mtmiller.

Rik <rik5>
Group administrator
Fri 04 Jun 2021 12:30:08 AM UTC, comment #8: 

I traced this change in behavior to the fix for bug #48620 (https://hg.savannah.gnu.org/hgweb/octave/rev/a032ffb80704), which was first released with version 4.2.0. In this change, the logic was dropped to set 'forced_interactive' to false when Octave is already automatically interactive. I think that was a mistake.

I can confirm that version 4.0.3 treats '--interactive' as a no-op when running in a tty. And I can confirm that version 4.2.2 treats '--interactive' the same way that current builds do.

Mike Miller <mtmiller>
Group Member
Thu 03 Jun 2021 11:01:58 PM UTC, comment #7: 

The intent of that change, in the commit log, seems to agree with me, if I'm reading it right.

> Interactive means we are running a interactive session, forced interactive means the interactive session needed to be forced.


Can we agree first on the intent? My preference would be that in a tty, when Octave would be interactive anyway, "octave" and "octave --interactive" should result in the same behavior. That's the way bash works as far as I can tell.

I've confirmed the following behavior differences when I run "octave --interactive" in a tty environment:

  • readline line editing is disabled
  • readline setting blink_matching_paren is disabled
  • more has no effect, pager is treated as disabled
  • confirm_recursive_rmdir has no effect, treated as disabled
  • scanf(..., "C") doesn't flush/consume an entire line from stdin


I think these should not be different, unless stdin is actually not a tty but the "--interactive" option is used.

Mike Miller <mtmiller>
Group Member
Thu 03 Jun 2021 12:19:04 PM UTC, comment #6: 

See also

http://hg.savannah.gnu.org/hgweb/octave/diff/1c9ed5b4c73d/libinterp/octave.cc

when the change was made to disable line editing using the condition


if ((! interactive || forced_interactive) && ! forced_line_editing)


instead of
if (! interactive && ! forced_line_editing)



John W. Eaton <jwe>
Group administrator
Thu 03 Jun 2021 12:04:08 PM UTC, comment #5: 

I think the -i option for Octave was originally patterned after the same option in bash.  In bash, it is mostly about what happens when the shell starts and exits and whether prompts are printed.

I'm not saying we have to follow the behavior of bash, but for comparison, recent bash versions do this:


  /* First, let the outside world know about our interactive status.
     A shell is interactive if the `-i' flag was given, or if all of
     the following conditions are met:
        no -c command
        no arguments remaining or the -s flag given
        standard input is a terminal
        standard error is a terminal
     Refer to Posix.2, the description of the `sh' utility. */

  if (forced_interactive ||             /* -i flag */
      (!command_execution_string &&     /* No -c command and ... */
       wordexp_only == 0 &&             /* No --wordexp and ... */
       ((arg_index == argc) ||          /*   no remaining args or... */
        read_from_stdin) &&             /*   -s flag with args, and */
       isatty (fileno (stdin)) &&       /* Input is a terminal and */
       isatty (fileno (stderr))))       /* error output is a terminal. */
    init_interactive ();
  else
    init_noninteractive ();


and interactive shells (whether forced with the -i option or because stdin/stderr are connected to terminals) use line editing unless it is explicitly disabled.

John W. Eaton <jwe>
Group administrator
Thu 03 Jun 2021 02:14:19 AM UTC, comment #4: 

I have no opinion so if you want to push your change that's fine.

Rik <rik5>
Group administrator
Thu 03 Jun 2021 01:31:22 AM UTC, comment #3: 

I haven't had a chance to test yet, but I was thinking more of a change like this:


        m_interactive = (! is_octave_program && stdin_is_tty
                         && octave_isatty_wrapper (fileno (stdout)));

        // Not forced to be interactive if we're already interactive!
        if (m_interactive)
          m_app_context->forced_interactive (false);

        // Check if the user forced an interactive session.
        if (options.forced_interactive ())
          m_interactive = true;


The reason is because there are other parts of the code base that check for similar conditions, and consider "forced_interactive" to mean that Octave normally wouldn't have been interactive.

Mike Miller <mtmiller>
Group Member
Wed 02 Jun 2021 10:52:52 PM UTC, comment #2: 

The code for this is in libinterp/corefcn/interpreter.cc.  Decision point is


        line_editing = options.line_editing ();
        if ((! m_interactive || options.forced_interactive ())
            && ! options.forced_line_editing ())
          line_editing = false;


Taking out "|| options.forced_interactive ()" is enough to make the logic work the way we want.  The new line would be


        if (! m_interactive && ! options.forced_line_editing ())


Is there anything wrong with making this change?  If we are not operating from a terminal and someone forces interactive do we just say it's their own fault if things don't work out?

Rik <rik5>
Group administrator
Tue 01 Jun 2021 11:19:48 PM UTC, comment #1: 

That seems right.  The '--interactive' should force Octave to behave as if was running in a normal terminal, even if it may not be.

Starting in a terminal, I was able to force readline to work by adding the additional option '--line-editing'


run-octave --interactive --line-editing


This combination works just fine.

Rik <rik5>
Group administrator
Sat 29 May 2021 06:54:13 PM UTC, original submission:  

In bug #60693, it's observed that 'octave -i' or 'octave --interactive' disables readline line editing and history, even when run from a terminal. This seems like a logic error in command line option parsing.

If running 'octave' in a terminal would enable readline line editing, then 'octave --interactive' in the same context should be the same. The only time it should disable line editing is when '--interactive' is used and Octave wouldn't have been running interactively otherwise.

Mike Miller <mtmiller>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #51669:  bug60696.cset added by rik5 (2KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2021-08-01 rik5 StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2021-07-22 rik5 StatusPatch Submitted Ready For Test
    2021-07-16 rik5 Attached File- Added bug60696.cset, #51669
        StatusConfirmed Patch Submitted
    2021-06-01 rik5 StatusNone Confirmed
    2021-05-29 mtmiller Carbon-CopyRemoved 80942 -

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code