Re: ielm automatic saving of history -- bug 67000

[Madhu]

Hello,

*  Simen Heggestøyl <simenheg@runbox.com> <87bjw8dwa0.fsf@runbox.com>
Wrote on Wed, 15 Jan 2025 14:47:03 +0100

> Madhu <enometh@meer.net> writes:
>>  (defcustom ielm-history-file-name
>> -  (locate-user-emacs-file "ielm-history.eld")
>> +  (if t nil
>> +      (locate-user-emacs-file "ielm-history.eld"))
>>    "If non-nil, name of the file to read/write IELM input history."
>>    :type '(choice (const :tag "Disable input history" nil)
>>                   file)
>
> I don't think the default value should be changed. Having IELM keep
> history by default aligns it with how other Emacs shells behave, such as
> Eshell and other comint-based shells such as the Octave and IDLWAVE
> shells.

I haven't used those, but all the comint based shells i've used
(inferior lisp, etc.) the history mechanism is opt-in. There should be
a definite way to opt out, or to inform the user that his data is
being persisted, something he may not desire at all, if he is using
ielm as a scratch pad and most of the entries end up being junk, or
it only useful for forensics.

>> @@ -520,7 +521,9 @@ ielm--input-history-writer
>>    "Return a function writing IELM input history to BUF."
>>    (lambda ()
>>      (with-current-buffer buf
>> -      (comint-write-input-ring))))
>> +      (let ((coding-system-for-write 'utf-8-emacs-unix))
>> +        ;; cannot add a file-local section when using comint.
>> +        (comint-write-input-ring)))))
>
> Seems fine to be, but shouldn't a similar change also be needed
> everywhere else where we call `comint-write-input-ring`? Should this
> perhaps be handled by that function?

I think this is based on eli's suggestion that it is needed here,
every user of the function should bind it if needed on a case by case
basis.

>> @@ -623,17 +626,19 @@ inferior-emacs-lisp-mode
>>    (add-hook 'comint-indirect-setup-hook
>>              #'ielm-indirect-setup-hook 'append t)
>>    (setq comint-indirect-setup-function #'emacs-lisp-mode)
>> -
>> -  ;; Input history
>> -  (setq-local comint-input-ring-file-name ielm-history-file-name)
>> -  (setq-local ielm--exit (ielm--input-history-writer (current-buffer)))
>> -  (setq-local kill-buffer-hook
>> +  (when ielm-history-file-name
>> +    ;; Input history
>> +    (setq-local comint-input-ring-file-name ielm-history-file-name)
>> +    (setq-local ielm--exit (ielm--input-history-writer (current-buffer)))
>> +    (add-hook 'kill-buffer-hook
>
> The `when` check is unnecessary, since this is already handled in
> `comint-read-input-ring` and `comint-write-input-ring`.

I disagree. The entry point to opt in to this mechanism is through
this variable. Doing this means precludes any other history saving
mechanism from being used.

Say I may want to use a different mechanism (like the one i posted) to
save comint history, I cannot do that without modifying this library
code.  If ielm-history-file-name is nil ielm should not touch comint
variables.

> Another small bonus is that without the `when` check, a change to
> `ielm-history-file-name` will take effect without having to reactivate
> IELM mode.


> The change from `(setq-local kill-buffer-hook ...` to `(add-hook
> 'kill-buffer-hook ...` looks good to me.
>> +    (let ((coding-system-for-read 'utf-8-unix))
>> +      (comint-read-input-ring t)))
>
> Shouldn't this also be `utf-8-emacs-unix`?

It's there in the emacs-devel discussion but I have to check. (the
"-emacs" seems to add some "markup" which I haven't fully understood
yet.)