Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#56381 closed enhancement (fixed)

It would be nice if shell mode saved command history immediately

Reported by: ylluminarious (George Plymale II) Owned by: ylluminarious (George Plymale II)
Priority: Normal Milestone: MacPorts 2.6.0
Component: base Version: 2.4.3
Keywords: Cc: raimue (Rainer Müller)
Port:

Description

Hi,

I'm new to Macports so I hope that this ticket is not a duplicate and that it meets the Ticket Guidelines. Anyway, as I was using Macports' shell mode, I noticed history is only saved after the main command loop exits. This approach has a major drawback (which I've often been a victim of while using bash).

If the port process is abruptly killed, then that session's history is gone. This can happen by pressing ^C twice, which can quite easily happen as an accident. There are also a myriad of other dangers which could abruptly kill a port shell session before its time.

It would be nice if each command got written to the history file right after it was entered. This is how zsh does it and I've always been fond of it, since you always know that your history is saved. I looked in src/port/port.tcl to see if there was a setting to enable such behavior, but I see no such setting there. Specifically, I looked in the process_command_file procedure, but found nothing relating to this.

Attachments (2)

macports_append_history.patch (3.0 KB) - added by ylluminarious (George Plymale II) 6 years ago.
First draft of a patch to fix this issue
2nd_macports_append_history.patch (3.1 KB) - added by ylluminarious (George Plymale II) 6 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 6 years ago by pmetzger (Perry E. Metzger)

I suspect no one else will be as interested in working on this as you might be, but patches to add an optional mode to save immediately, if cleanly implemented, would almost certainly be accepted. Are you interested in working on it?

comment:2 in reply to:  1 Changed 6 years ago by ylluminarious (George Plymale II)

Replying to pmetzger:

I suspect no one else will be as interested in working on this as you might be, but patches to add an optional mode to save immediately, if cleanly implemented, would almost certainly be accepted. Are you interested in working on it?

Hi, sorry for the somewhat delayed reply. Yes, I am interested in working on this. I probably need to brush up on my Tcl, but aside from that I don't see a problem. I'll try and submit a patch in the next few weeks or so.

comment:3 Changed 6 years ago by raimue (Rainer Müller)

Cc: raimue added

Hello, and welcome to MacPorts! Yes, you already found the right place with process_command_file in the port client.

I guess the solution would be to call rl_history write before executing the command. Then it would always be saved, even if the command itself is interrupted. Could even be done in get_next_cmdline where the line is added to the readline history in memory.

One drawback is that this would always write the full history file instead of just appending a single line. It looks like readline (actually libedit) has a append_history function we are not yet wrapping in rl_history that should be used for appends.

Let us know if you get stuck or need assistance when working on this!

comment:4 in reply to:  3 Changed 6 years ago by ylluminarious (George Plymale II)

Replying to raimue:

Hello, and welcome to MacPorts! Yes, you already found the right place with process_command_file in the port client.

I guess the solution would be to call rl_history write before executing the command. Then it would always be saved, even if the command itself is interrupted. Could even be done in get_next_cmdline where the line is added to the readline history in memory.

One drawback is that this would always write the full history file instead of just appending a single line. It looks like readline (actually libedit) has a append_history function we are not yet wrapping in rl_history that should be used for appends.

Let us know if you get stuck or need assistance when working on this!

Hi! Thanks so much for your advice. Those are certainly some good pointers and I'm sure it will help me out as I'm implementing this.

Honestly, I've begun to use Macports out of some disillusionment with Homebrew. That project is more and more taking away features and estranging certain kinds of users with certain use-cases. It seems that this is largely due to the maintainers' open and flagrant disregard for users. I am glad that it seems that the Macports project is more welcoming to improvements.

Last edited 6 years ago by ylluminarious (George Plymale II) (previous) (diff)

comment:5 in reply to:  3 Changed 6 years ago by ylluminarious (George Plymale II)

Hi again folks,

I've been working on this today and I've run into a bit of a wall. I think I've successfully wrapped the append_history function and I've also got some code which uses it in port.tcl. Yet, for some reason the use_readline variable is always false when I run a custom build of port. I'm using this custom build as per the instructions in the guide about running multiple installations of Macports.

It seems that use_readline is false because of the second clause in its definition:

set use_readline [expr {$isstdin && [readline init "port"]}].

That is, the [readline init "port"] command seems to be failing for some reason. I have no idea why that would be even after examining its definition here. Moreover, my code does not touch anything in the ReadlineCmd function.

I've attached a patch which contains my code. I currently have some debugging puts statements in it because of the above-mentioned bug. So that's why they're there, if you're wondering.

Some environment information:

  • I'm on macOS 10.13.4
  • The custom Macports build is on commit a6aa79a2 (which is v2.4.4)
  • I've got a real Macports installation under /opt/local
  • The custom Macports build has no ports installed and I've not run selfupdate on it

I would appreciate any help that could be offered.

Changed 6 years ago by ylluminarious (George Plymale II)

First draft of a patch to fix this issue

comment:6 Changed 6 years ago by raimue (Rainer Müller)

You need to use ./configure --enable-readline as it is disabled by default. HAVE_LIBREADLINE would not be defined on your installation.

comment:7 in reply to:  6 Changed 6 years ago by ylluminarious (George Plymale II)

Replying to raimue:

You need to use ./configure --enable-readline as it is disabled by default. HAVE_LIBREADLINE would not be defined on your installation.

Thanks again for the advice! You are right, I thought that option was on by default but didn't double-check.

However, I am now running into a different issue with this error when I try to recompile my changes:

Undefined symbols for architecture x86_64:
  "_append_history", referenced from:
      _RLHistoryCmd in readline.o
ld: symbol(s) not found for architecture x86_64

After grepping through Apple's libedit, it seems that actually there is no append_history function defined anywhere. It seems to only be in GNU readline... so this does put a bit of a wrench in things.

comment:8 Changed 6 years ago by ylluminarious (George Plymale II)

Just FYI: I've dug a little more into solving this problem, but haven't found much. So I posted a question on StackOverflow about it. Hopefully we can get some pointers on what to do here.

comment:9 Changed 6 years ago by raimue (Rainer Müller)

Indeed, if libedit does not implement append_history that is really unfortunate. Looks like it was only recently added to NetBSD, which would be where Apple borrowed their implementation, but it has also been patched since the initial import. It is unlikely that this will find its way into macOS, definitely not into older releases. Filing a rdar might help.

Not sure how we want to continue with this, then. Do we want to roll our own code to manage the history file? I have doubts that this is worth importing the whole libedit source to our vendor libraries...

comment:10 in reply to:  9 Changed 6 years ago by ylluminarious (George Plymale II)

Replying to raimue:

Indeed, if libedit does not implement append_history that is really unfortunate. Looks like it was only recently added to NetBSD, which would be where Apple borrowed their implementation, but it has also been patched since the initial import. It is unlikely that this will find its way into macOS, definitely not into older releases. Filing a rdar might help.

Yes, you're right. While fishing around for ideas, I found the original source of libedit and it seems that Apple has not made many changes to their distribution of it over the years. I doubt they would help even after filing a rdar... I'm very doubtful that they'd backport old releases of macOS which are no longer supported.


Not sure how we want to continue with this, then. Do we want to roll our own code to manage the history file? I have doubts that this is worth importing the whole libedit source to our vendor libraries...

Yes, importing the whole of libedit is not worth this one feature. I'd agree that to do so would be overkill. All of this has led me to the conclusion that we need to roll our own solution or drop the whole issue entirely. I prefer the former, so I've come up with a rather simplistic solution which may actually suffice.

I poked and prodded around Apple's sources, NetBSD's sources, and finally the GNU info manual for their History library and I finally wrote a few lines which actually seem to do the job. The key was that I found the current_history() function which yields the most recent history item. Its return value can be used to access the raw string via the line field. I.e., current_history()->line

After finding this function, I wrote a couple of lines with just fopen and fprintf and it seems to work. The code is very basic; there's no error-handling on I/O errors and there's no encoding/decoding of the history string. However, it seems that such en/de-coding is not necessary. This surprised me at first, until I realized that macOS's bash is also using Readline (I think) and it has no such encoding wherein the space character is translated to \040 and so forth.

Readline seems to be able to pull history items out of the list even if they are using raw whitespace, without any hitch whatsoever. I tried also to send lines with lots of different characters such as braces, semicolons, ampersands, and all kinds of stuff and Readline did not so much as hiccup. I'm surprised and I'm hoping that this code is sufficient to solve this issue. Please let me know what you think and if you have any improvements in mind.

Thanks.

Changed 6 years ago by ylluminarious (George Plymale II)

comment:11 Changed 6 years ago by ylluminarious (George Plymale II)

Friendly ping... any thoughts on these changes?

comment:12 Changed 6 years ago by ylluminarious (George Plymale II)

Owner: set to ylluminarious
Resolution: fixed
Status: newclosed

In 486565dafb882078f0e2a22399a08ef1966fb163/macports-base (master):

port: append history in interactive sessions

Hitherto, port.tcl has used bash-style history-writing at the end of
an interactive session. That is vulnerable to crashes and other abrupt
interruptions. Switch to writing commands to the history file
immediately.

Closes: #56381

comment:13 Changed 6 years ago by neverpanic (Clemens Lang)

Milestone: MacPorts Future

comment:14 Changed 5 years ago by jmroot (Joshua Root)

Milestone: MacPorts FutureMacPorts 2.6.0
Note: See TracTickets for help on using tickets.