bug#17361: Tramp does not save history across sessions.

bug#17361: Tramp does not save history across sessions.

[Le Wang]

Tramp installs its own sentinel -- tramp-process-sentinel -- over
shell-mode's shell-write-history-on-exit.  What's the way to write to
history file when the process ends without using defadvice?

Could the default behave better?


-- 
Le

bug#17361: Tramp does not save history across sessions.

[Michael Albinus]

Le Wang <l26wang@gmail.com> writes:

> Tramp installs its own sentinel -- tramp-process-sentinel -- over
> shell-mode's shell-write-history-on-exit.  What's the way to write to
> history file when the process ends without using defadvice?
>
> Could the default behave better?

The problem seems to be more general. Emacs does not support to have
several process sentinels for a given process. If several sentinels are
declared for a process by ?`set-process-sentinel', they compete for
being attached to the process. And the last one wins.

So we need to support several sentinels per process. Maybe this exist
already, but I'm not aware of such a mechanism.

Stefan?

Best regards, Michael.

bug#17361: Tramp does not save history across sessions.

[Stefan Monnier]

> The problem seems to be more general. Emacs does not support to have
> several process sentinels for a given process. If several sentinels are
> declared for a process by ?`set-process-sentinel', they compete for
> being attached to the process. And the last one wins.

Not true any more: Emacs-24.4's `add-function' was invented in response
to this specific request (or maybe it was for set-process-filter, but
the point stands).


        Stefan

bug#17361: Tramp does not save history across sessions.

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Date: Mon, 28 Apr 2014 15:45:34 +0200
> Cc: 17361@debbugs.gnu.org
> 
> So we need to support several sentinels per process.

Or a protocol: if you install a sentinel, and another one already
exists, call it after yours.

bug#17361: Tramp does not save history across sessions.

[Michael Albinus]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

> Not true any more: Emacs-24.4's `add-function' was invented in response
> to this specific request (or maybe it was for set-process-filter, but
> the point stands).

Ah, thanks. I wasn't aware of this use case. However, shouldn't this be
propagated further? The Elisp info pages do not mention this use case,
when speaking about `set-process-filter' or `set-process-sentinel'.

Anyway, I'll try to rewrite Tramp's process filters and sentinels next
days. As usual, the crucial point will be backward compatibility ...

>         Stefan

Best regards, Michael.

bug#17361: Tramp does not save history across sessions.

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> So we need to support several sentinels per process.
>
> Or a protocol: if you install a sentinel, and another one already
> exists, call it after yours.

Yes. But all of the sentinels (or filters) must play the same game
then. When Tramp's sentinel is the first one being activated, it
shouldn't be thrown away by a `set-process-sentinel' call from another
library. We don't know in advance, in which order sentinels are
installed for a given process.

Best regards, Michael.

bug#17361: Tramp does not save history across sessions.

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: l26wang@gmail.com,  17361@debbugs.gnu.org
> Date: Mon, 28 Apr 2014 20:05:17 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> So we need to support several sentinels per process.
> >
> > Or a protocol: if you install a sentinel, and another one already
> > exists, call it after yours.
> 
> Yes. But all of the sentinels (or filters) must play the same game
> then.

Indeed, the protocol should be universally enforced.

bug#17361: Tramp does not save history across sessions.

[Stefan Monnier]

>> > Or a protocol: if you install a sentinel, and another one already
>> > exists, call it after yours.
>> Yes. But all of the sentinels (or filters) must play the same game
>> then.
> Indeed, the protocol should be universally enforced.

AFAIK add-function does that.


        Stefan

bug#17361: Tramp does not save history across sessions.

[Stefan Monnier]

> Ah, thanks. I wasn't aware of this use case. However, shouldn't this be
> propagated further? The Elisp info pages do not mention this use case,
> when speaking about `set-process-filter' or `set-process-sentinel'.

I added some advertisement for add-function in those sections, thanks.


        Stefan

bug#17361: Tramp does not save history across sessions.

[Michael Albinus]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> Ah, thanks. I wasn't aware of this use case. However, shouldn't this be
>> propagated further? The Elisp info pages do not mention this use case,
>> when speaking about `set-process-filter' or `set-process-sentinel'.
>
> I added some advertisement for add-function in those sections, thanks.

Reading your text, I wonder how one should implement this. Usually, one
doesn't know which other library intends to set a filter or a process.

In stock Emacs, there are 43 files with calls of `set-process-filter',
and 53 files with calls of `set-process-sentinel'. In the elpa branch,
there are 4/4 additional such files. All those places must be adapted,
checking whether there could be several filters or sentinels.

Wouldn't it be more consistent to modify `set-process-filter' and
´set-process-sentinel' to take care, when several filters or sentinels
are added to a given process? Both functions could be equipped with an
optional argument WHERE, which has the similar meaning as in
`add-function'. Maybe just :before, :after and :replace shall be
allowed, and one of them (:after?) could be the default.

>         Stefan

Best regards, Michael.

bug#17361: Tramp does not save history across sessions.

[Daimrod]

Michael Albinus <michael.albinus@gmx.de> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>>> Ah, thanks. I wasn't aware of this use case. However, shouldn't this be
>>> propagated further? The Elisp info pages do not mention this use case,
>>> when speaking about `set-process-filter' or `set-process-sentinel'.
>>
>> I added some advertisement for add-function in those sections, thanks.
>
> Reading your text, I wonder how one should implement this. Usually, one
> doesn't know which other library intends to set a filter or a process.

I was thinking about the same thing.

> In stock Emacs, there are 43 files with calls of `set-process-filter',
> and 53 files with calls of `set-process-sentinel'. In the elpa branch,
> there are 4/4 additional such files. All those places must be adapted,
> checking whether there could be several filters or sentinels.
>
> Wouldn't it be more consistent to modify `set-process-filter' and
> ´set-process-sentinel' to take care, when several filters or sentinels
> are added to a given process? Both functions could be equipped with an
> optional argument WHERE, which has the similar meaning as in
> `add-function'. Maybe just :before, :after and :replace shall be
> allowed, and one of them (:after?) could be the default.

What about having a default sentinel/filter for all process that does
nothing (just a placeholder)?

Then instead of using `set-process-filter' one could use
(add-function :whatever (process-filter process) ...)

-- 
Daimrod/Greg

bug#17361: Tramp does not save history across sessions.

[Michael Albinus]

Daimrod <daimrod@gmail.com> writes:

>> Wouldn't it be more consistent to modify `set-process-filter' and
>> ´set-process-sentinel' to take care, when several filters or sentinels
>> are added to a given process? Both functions could be equipped with an
>> optional argument WHERE, which has the similar meaning as in
>> `add-function'. Maybe just :before, :after and :replace shall be
>> allowed, and one of them (:after?) could be the default.
>
> What about having a default sentinel/filter for all process that does
> nothing (just a placeholder)?

There are already `internal-default-process-filter' and
`internal-default-process-sentinel'. They must be replaced, of course,
when a new filter/sentinel is added.

> Then instead of using `set-process-filter' one could use
> (add-function :whatever (process-filter process) ...)

Nope. One would need to change all places those functions are called. I
believe it is simpler to modify `set-process-filter' and
`set-process-sentinel'.

Best regards, Michael.

bug#17361: Tramp does not save history across sessions.

[Daimrod]

Michael Albinus <michael.albinus@gmx.de> writes:

> Daimrod <daimrod@gmail.com> writes:
>
>>> Wouldn't it be more consistent to modify `set-process-filter' and
>>> ´set-process-sentinel' to take care, when several filters or sentinels
>>> are added to a given process? Both functions could be equipped with an
>>> optional argument WHERE, which has the similar meaning as in
>>> `add-function'. Maybe just :before, :after and :replace shall be
>>> allowed, and one of them (:after?) could be the default.
>>
>> What about having a default sentinel/filter for all process that does
>> nothing (just a placeholder)?
>
> There are already `internal-default-process-filter' and
> `internal-default-process-sentinel'. They must be replaced, of course,
> when a new filter/sentinel is added.

Oh, I wasn't aware of those functions.

>> Then instead of using `set-process-filter' one could use
>> (add-function :whatever (process-filter process) ...)
>
> Nope. One would need to change all places those functions are called. I
> believe it is simpler to modify `set-process-filter' and
> `set-process-sentinel'.

I see. Thanks for the explanation.

-- 
Daimrod/Greg

bug#17361: Tramp does not save history across sessions.

[Stefan Monnier]

> What about having a default sentinel/filter for all process that does
> nothing (just a placeholder)?

That is already the case since 24.4.

> Then instead of using `set-process-filter' one could use
> (add-function :whatever (process-filter process) ...)

That's the idea.


        Stefan

bug#17361: Tramp does not save history across sessions.

[Stefan Monnier]

> Reading your text, I wonder how one should implement this. Usually, one
> doesn't know which other library intends to set a filter or a process.

That's a good question.  So far it works well when you know enough of
the various places that want to set a filter, so you can make sure only
one of them uses set-process-filter (and does it before the others).


        Stefan

bug#17361: Tramp does not save history across sessions.

[Michael Albinus]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

> So far it works well when you know enough of the various places that
> want to set a filter, so you can make sure only one of them uses
> set-process-filter (and does it before the others).

Maybe. But Tramp cannot know who is using it. It just offers (its own
implementation of) very primitive functions, that's it. And it cannot
know, whether the calling library is doing its settings before or after
Tramp's setup.

That's what this bug report is about.

>         Stefan

Best regards, Michael.

bug#17361: Tramp does not save history across sessions.

[Stefan Monnier]

>> So far it works well when you know enough of the various places that
>> want to set a filter, so you can make sure only one of them uses
>> set-process-filter (and does it before the others).
> Maybe. But Tramp cannot know who is using it. It just offers (its own
> implementation of) very primitive functions, that's it. And it cannot
> know, whether the calling library is doing its settings before or after
> Tramp's setup.
> That's what this bug report is about.

The fix is to make those other packages use add-function as well, then.


        Stefan

bug#17361: Tramp does not save history across sessions.

[Michael Albinus]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> Maybe. But Tramp cannot know who is using it. It just offers (its own
>> implementation of) very primitive functions, that's it. And it cannot
>> know, whether the calling library is doing its settings before or after
>> Tramp's setup.
>> That's what this bug report is about.
>
> The fix is to make those other packages use add-function as well, then.

And why not modify `set-process-sentinel'? This would be less changes in
the different packages.

>         Stefan

Best regards, Michael.

bug#17361: Tramp does not save history across sessions.

[Stefan Monnier]

> And why not modify `set-process-sentinel'? This would be less changes in
> the different packages.

No, same difference: the calls need to be changed anyway.


        Stefan

bug#17361: Tramp does not save history across sessions.

[Michael Albinus]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> And why not modify `set-process-sentinel'? This would be less changes in
>> the different packages.
>
> No, same difference: the calls need to be changed anyway.

Not, if we use a proper default for WHERE, the new optional arg of
`set-process-sentinel'.

>         Stefan

Best regards, Michael.

bug#17361: Tramp does not save history across sessions.

[Stefan Monnier]

>>> And why not modify `set-process-sentinel'? This would be less changes in
>>> the different packages.
>> No, same difference: the calls need to be changed anyway.
> Not, if we use a proper default for WHERE, the new optional arg of
> `set-process-sentinel'.

The only default that's backward compatible would be :override, which
basically defeats the purpose.


        Stefan

bug#17361: Tramp does not save history across sessions.

[Michael Albinus]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>>>> And why not modify `set-process-sentinel'? This would be less changes in
>>>> the different packages.
>>> No, same difference: the calls need to be changed anyway.
>> Not, if we use a proper default for WHERE, the new optional arg of
>> `set-process-sentinel'.
>
> The only default that's backward compatible would be :override, which
> basically defeats the purpose.

In practice, your proposal means to throw away `set-process-sentinel'
(and `set-process-filter'). Since the default functions are already
enabled, in most cases one shall use `add-function' instead. Which is
more complicate.

Do we want such a radical change? Your wording, you have applied in
processes.texi, sounds different.

(I don't oppose completely, I just want that we understand and agree
such a change.)

>         Stefan

Best regards, Michael.

bug#17361: Tramp does not save history across sessions.

[Stefan Monnier]

>> The only default that's backward compatible would be :override, which
>> basically defeats the purpose.
> In practice, your proposal means to throw away `set-process-sentinel'
> (and `set-process-filter').

[ Of course, only on the surface, since these are the low-level accessors
  used by add-function.  ]

> Since the default functions are already enabled, in most cases one
> shall use `add-function' instead.

There is a real problem with the default filter/sentinel, indeed.
Basically, add-function is a mechanism that allows combining functions
onto a "single function spot", so it provides the tool we need.

But another problem remains: for historical reasons (and for
convenience), the default filters/sentinels don't "do nothing".  So in
many cases packages want to *replace* the default rather than extend it.
`set-process-filter' works well for those, until another package comes
along which needs to interact with it.  Saying "use add-function"
doesn't really solve this problem, because we'll just replace

  (set-process-filter PROC FUN)
with
  (add-function :override (process-filter PROC) FUN)

which, just like set-process-filter, will override not just the default
filter but other filters added via add-function as well.

I haven't thought about how to really solve this problem.  I'm open
to suggestions.

> (I don't oppose completely, I just want that we understand and agree
> such a change.)

I don't have a good answer yet, sorry.


        Stefan

bug#17361: Tramp does not save history across sessions.

[Glenn Morris]

Sorry, I closed this bug by mistake.
Reopening.

bug#17361: Tramp does not save history across sessions.

[Stefan Kangas]

Michael Albinus <michael.albinus@gmx.de> writes:

> Le Wang <l26wang@gmail.com> writes:
>
>> Tramp installs its own sentinel -- tramp-process-sentinel -- over
>> shell-mode's shell-write-history-on-exit.  What's the way to write to
>> history file when the process ends without using defadvice?
>>
>> Could the default behave better?
>
> The problem seems to be more general. Emacs does not support to have
> several process sentinels for a given process. If several sentinels are
> declared for a process by ?`set-process-sentinel', they compete for
> being attached to the process. And the last one wins.
>
> So we need to support several sentinels per process. Maybe this exist
> already, but I'm not aware of such a mechanism.

(That was 7.5 years ago.)

What followed was a discussion about how Tramp should use 'add-function'
instead of 'set-process-sentinel', IIUC.  Is this still an issue, or has
the situation changed since?

bug#17361: Tramp does not save history across sessions.

[Michael Albinus]

Stefan Kangas <stefan@marxist.se> writes:

Hi Stefan,

> (That was 7.5 years ago.)
>
> What followed was a discussion about how Tramp should use 'add-function'
> instead of 'set-process-sentinel', IIUC.  Is this still an issue, or has
> the situation changed since?

Tramp still uses set-process-sentinel. I will change it to use
add-function instead, and I will run my regression tests.

However, we still need to adapt the set-process-sentinel calls elsewhere
in the code base. And of course, this will have to go to master, which
means I have to cut a new Tramp branch.

And the same game for set-process-filter, right?

Best regards, Michael.