bug#41099: 28.0.50; TRAMP process-file ignores exit status of remote process

bug#41099: 28.0.50; TRAMP process-file ignores exit status of remote process

[Philipp Stephani]

emacs -Q -batch -eval '(let ((default-directory "/ssh:HOST:/")) (print (process-file "bash" nil nil nil "-c" "exit 42")))'
Tramp: Sending command ‘exec ssh   -o ControlMaster=auto -o ControlPath='tramp.%C' -o ControlPersist=no -e none HOST’
Tramp: Found remote shell prompt on ‘HOST’

1

Note that the return value of `process-file' is 1 instead of 42.

The relevant debug output is

20:46:42.302070 tramp-send-command (6) # ( cd / && bash -c exit\ 42 </dev/null; echo tramp_exit_status $? )
20:46:42.331851 tramp-wait-for-regexp (6) # 
tramp_exit_status 42

i.e. TRAMP should have access to the correct exit status.


In GNU Emacs 28.0.50 (build 6, x86_64-pc-linux-gnu, GTK+ Version 3.24.13, cairo version 1.16.0)
 of 2020-05-05
Repository revision: daab2d3a62ac8fb1c74987e614cee93dc79fab74
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12004000
System Description: Debian GNU/Linux rodete

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.

Configured using:
 'configure --enable-gtk-deprecation-warnings --with-modules
 --without-pop --with-mailutils --enable-gcc-warnings=warn-only
 CFLAGS=-O3 LDFLAGS=-O3'

Configured features:
XPM JPEG TIFF GIF PNG CAIRO SOUND DBUS GSETTINGS GLIB NOTIFY INOTIFY
LIBSELINUX GNUTLS FREETYPE HARFBUZZ ZLIB TOOLKIT_SCROLL_BARS GTK3 X11
XDBE XIM MODULES THREADS PDUMPER GMP

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc dired dired-loaddefs
format-spec rfc822 mml easymenu mml-sec epa epg epg-config gnus-util
rmail rmail-loaddefs text-property-search time-date mm-decode mm-bodies
mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail
rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils phst skeleton
derived edmacro kmacro pcase ffap thingatpt url url-proxy url-privacy
url-expand url-methods url-history url-cookie url-domsuf url-util
url-parse auth-source cl-seq eieio eieio-core cl-macs eieio-loaddefs
password-cache json map url-vars mailcap subr-x rx gnutls puny seq
byte-opt gv bytecomp byte-compile cconv dbus xml cl-loaddefs cl-lib
tooltip eldoc electric uniquify ediff-hook vc-hooks lisp-float-type
mwheel term/x-win x-win term/common-win x-dnd tool-bar dnd fontset image
regexp-opt fringe tabulated-list replace newcomment text-mode elisp-mode
lisp-mode prog-mode register page tab-bar menu-bar rfn-eshadow isearch
timer select scroll-bar mouse jit-lock font-lock syntax facemenu
font-core term/tty-colors frame minibuffer cl-generic cham georgian
utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao korean
japanese eucjp-ms cp51932 hebrew greek romanian slovak czech european
ethiopic indian cyrillic chinese composite charscript charprop
case-table epa-hook jka-cmpr-hook help simple abbrev obarray
cl-preloaded nadvice loaddefs button faces cus-face macroexp files
text-properties overlay sha1 md5 base64 format env code-pages mule
custom widget hashtable-print-readable backquote threads dbusbind
inotify dynamic-setting system-font-setting font-render-setting cairo
move-toolbar gtk x-toolkit x multi-tty make-network-process emacs)

Memory information:
((conses 16 63581 4193)
 (symbols 48 8433 1)
 (strings 32 22340 1840)
 (string-bytes 1 709171)
 (vectors 16 12523)
 (vector-slots 8 175190 8492)
 (floats 8 25 29)
 (intervals 56 202 0)
 (buffers 992 11))

-- 
Google Germany GmbH
Erika-Mann-Straße 33
80636 München

Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

If you received this communication by mistake, please don’t forward it to
anyone else (it may contain confidential or privileged information), please
erase all copies of it, including all attachments, and please let the sender
know it went to the wrong person.  Thanks.

bug#41099: 28.0.50; TRAMP process-file ignores exit status of remote process

[Philipp Stephani]

Am Di., 5. Mai 2020 um 20:50 Uhr schrieb Philipp Stephani
<p.stephani2@gmail.com>:
>
>
> emacs -Q -batch -eval '(let ((default-directory "/ssh:HOST:/")) (print (process-file "bash" nil nil nil "-c" "exit 42")))'
> Tramp: Sending command ‘exec ssh   -o ControlMaster=auto -o ControlPath='tramp.%C' -o ControlPersist=no -e none HOST’
> Tramp: Found remote shell prompt on ‘HOST’
>
> 1
>
> Note that the return value of `process-file' is 1 instead of 42.
>
> The relevant debug output is
>
> 20:46:42.302070 tramp-send-command (6) # ( cd / && bash -c exit\ 42 </dev/null; echo tramp_exit_status $? )
> 20:46:42.331851 tramp-wait-for-regexp (6) #
> tramp_exit_status 42
>
> i.e. TRAMP should have access to the correct exit status.

It looks like `tramp-send-command-and-check' should not just search
for the exit code, but also return it.

bug#41099: 28.0.50; TRAMP process-file ignores exit status of remote process

[Michael Albinus]

Philipp Stephani <p.stephani2@gmail.com> writes:

> It looks like `tramp-send-command-and-check' should not just search
> for the exit code, but also return it.

No, most use cases a simple nil or t is appropriate. Otherwise, you would
need to check always (zerop (tramp-send-command-and-check ...)). But in
tramp-sh-handle-process-file, it shall return the exit code (indicated
by an optional argument).

Same for tramp-adb-send-command-and-check, called in tramp-adb-handle-process-file.

Best regards, Michael.

bug#41099: 28.0.50; TRAMP process-file ignores exit status of remote process

[Michael Albinus]

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

Hi Philipp,

>> It looks like `tramp-send-command-and-check' should not just search
>> for the exit code, but also return it.
>
> No, most use cases a simple nil or t is appropriate. Otherwise, you would
> need to check always (zerop (tramp-send-command-and-check ...)). But in
> tramp-sh-handle-process-file, it shall return the exit code (indicated
> by an optional argument).
>
> Same for tramp-adb-send-command-and-check, called in tramp-adb-handle-process-file.

I've fixed this in master, could you pls check?

Best regards, Michael.

bug#41099: 28.0.50; TRAMP process-file ignores exit status of remote process

[Philipp Stephani]

Am Mi., 6. Mai 2020 um 10:38 Uhr schrieb Michael Albinus
<michael.albinus@gmx.de>:
>
> Michael Albinus <michael.albinus@gmx.de> writes:
>
> Hi Philipp,
>
> >> It looks like `tramp-send-command-and-check' should not just search
> >> for the exit code, but also return it.
> >
> > No, most use cases a simple nil or t is appropriate. Otherwise, you would
> > need to check always (zerop (tramp-send-command-and-check ...)). But in
> > tramp-sh-handle-process-file, it shall return the exit code (indicated
> > by an optional argument).
> >
> > Same for tramp-adb-send-command-and-check, called in tramp-adb-handle-process-file.
>
> I've fixed this in master, could you pls check?

Looks like it's working fine, thanks for the quick fix!

bug#41099: 28.0.50; TRAMP process-file ignores exit status of remote process

[Philipp Stephani]

Am Mi., 6. Mai 2020 um 12:38 Uhr schrieb Philipp Stephani
<p.stephani2@gmail.com>:
>
> Am Mi., 6. Mai 2020 um 10:38 Uhr schrieb Michael Albinus
> <michael.albinus@gmx.de>:
> >
> > Michael Albinus <michael.albinus@gmx.de> writes:
> >
> > Hi Philipp,
> >
> > >> It looks like `tramp-send-command-and-check' should not just search
> > >> for the exit code, but also return it.
> > >
> > > No, most use cases a simple nil or t is appropriate. Otherwise, you would
> > > need to check always (zerop (tramp-send-command-and-check ...)). But in
> > > tramp-sh-handle-process-file, it shall return the exit code (indicated
> > > by an optional argument).
> > >
> > > Same for tramp-adb-send-command-and-check, called in tramp-adb-handle-process-file.
> >
> > I've fixed this in master, could you pls check?
>
> Looks like it's working fine, thanks for the quick fix!

As a second step, consider also translating signals: if the exit code
is > 128, subtract 128 and return an appropriate string, like
call-process does.

bug#41099: 28.0.50; TRAMP process-file ignores exit status of remote process

[Michael Albinus]

Philipp Stephani <p.stephani2@gmail.com> writes:

>> Looks like it's working fine, thanks for the quick fix!
>
> As a second step, consider also translating signals: if the exit code
> is > 128, subtract 128 and return an appropriate string, like
> call-process does.

Thanks for the hint, will check.

Best reghards, Michael.

bug#41099: 28.0.50; TRAMP process-file ignores exit status of remote process

[Michael Albinus]

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

> Philipp Stephani <p.stephani2@gmail.com> writes:
>>
>> As a second step, consider also translating signals: if the exit code
>> is > 128, subtract 128 and return an appropriate string, like
>> call-process does.
>
> Thanks for the hint, will check.

Well, I can't see what "call-process does":

--8<---------------cut here---------------start------------->8---
(call-process "bash" nil nil nil "-c" "exit 142")
=> 142
--8<---------------cut here---------------end--------------->8---

Best reghards, Michael.

bug#41099: 28.0.50; TRAMP process-file ignores exit status of remote process

[Philipp Stephani]

Am Mi., 6. Mai 2020 um 15:32 Uhr schrieb Michael Albinus
<michael.albinus@gmx.de>:
>
> Michael Albinus <michael.albinus@gmx.de> writes:
>
> > Philipp Stephani <p.stephani2@gmail.com> writes:
> >>
> >> As a second step, consider also translating signals: if the exit code
> >> is > 128, subtract 128 and return an appropriate string, like
> >> call-process does.
> >
> > Thanks for the hint, will check.
>
> Well, I can't see what "call-process does":
>
> --8<---------------cut here---------------start------------->8---
> (call-process "bash" nil nil nil "-c" "exit 142")
> => 142
> --8<---------------cut here---------------end--------------->8---

Try
(call-process "bash" nil nil nil "-c" "kill -SYS $$")
"Bad system call"

bug#41099: 28.0.50; TRAMP process-file ignores exit status of remote process

[Michael Albinus]

Philipp Stephani <p.stephani2@gmail.com> writes:

Hi Philipp,

>> As a second step, consider also translating signals: if the exit code
>> is > 128, subtract 128 and return an appropriate string, like
>> call-process does.
>
> Try
> (call-process "bash" nil nil nil "-c" "kill -SYS $$")
> "Bad system call"

I see. Hmm, this would require to install a trap handler in the remote
shell, and to add a mechanism transferring its result back to
Tramp. Don't know whether this is worth the effort.

Best regards, Michael.

bug#41099: 28.0.50; TRAMP process-file ignores exit status of remote process

[Andreas Schwab]

On Mai 06 2020, Philipp Stephani wrote:

> Try
> (call-process "bash" nil nil nil "-c" "kill -SYS $$")
> "Bad system call"

That doesn't translate any exit code, it just handles the signal that
the process dies from.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#41099: 28.0.50; TRAMP process-file ignores exit status of remote process

[Philipp Stephani]

Am Mi., 6. Mai 2020 um 19:41 Uhr schrieb Andreas Schwab <schwab@linux-m68k.org>:
>
> On Mai 06 2020, Philipp Stephani wrote:
>
> > Try
> > (call-process "bash" nil nil nil "-c" "kill -SYS $$")
> > "Bad system call"
>
> That doesn't translate any exit code, it just handles the signal that
> the process dies from.

Yes, I know. But Bash catches such signals and then exits with 128 +
signal number.

bug#41099: 28.0.50; TRAMP process-file ignores exit status of remote process

[Philipp Stephani]

Am Mi., 6. Mai 2020 um 19:30 Uhr schrieb Michael Albinus
<michael.albinus@gmx.de>:
>
> Philipp Stephani <p.stephani2@gmail.com> writes:
>
> Hi Philipp,
>
> >> As a second step, consider also translating signals: if the exit code
> >> is > 128, subtract 128 and return an appropriate string, like
> >> call-process does.
> >
> > Try
> > (call-process "bash" nil nil nil "-c" "kill -SYS $$")
> > "Bad system call"
>
> I see. Hmm, this would require to install a trap handler in the remote
> shell, and to add a mechanism transferring its result back to
> Tramp. Don't know whether this is worth the effort.

That wouldn't work because Bash translates signals from subprocesses:

$ bash -c 'kill -SYS $$'; echo $?
Bad system call (core dumped)
159

The 159 here is 128 + the signal number.

bug#41099: 28.0.50; TRAMP process-file ignores exit status of remote process

[Philipp Stephani]

Am Mi., 6. Mai 2020 um 19:56 Uhr schrieb Philipp Stephani
<p.stephani2@gmail.com>:
>
> Am Mi., 6. Mai 2020 um 19:30 Uhr schrieb Michael Albinus
> <michael.albinus@gmx.de>:
> >
> > Philipp Stephani <p.stephani2@gmail.com> writes:
> >
> > Hi Philipp,
> >
> > >> As a second step, consider also translating signals: if the exit code
> > >> is > 128, subtract 128 and return an appropriate string, like
> > >> call-process does.
> > >
> > > Try
> > > (call-process "bash" nil nil nil "-c" "kill -SYS $$")
> > > "Bad system call"
> >
> > I see. Hmm, this would require to install a trap handler in the remote
> > shell, and to add a mechanism transferring its result back to
> > Tramp. Don't know whether this is worth the effort.
>
> That wouldn't work because Bash translates signals from subprocesses:
>
> $ bash -c 'kill -SYS $$'; echo $?
> Bad system call (core dumped)
> 159
>
> The 159 here is 128 + the signal number.

What I suggest here would be something like the following:

(defun tramp-process-file (...)
  (let ((code (...original code...)))
    (if (> code 128)
       ;; Probably a signal
       (format "Signal %d" (- code 128))
    code))

bug#41099: 28.0.50; TRAMP process-file ignores exit status of remote process

[Andreas Schwab]

On Mai 06 2020, Philipp Stephani wrote:

> Am Mi., 6. Mai 2020 um 19:41 Uhr schrieb Andreas Schwab <schwab@linux-m68k.org>:
>>
>> On Mai 06 2020, Philipp Stephani wrote:
>>
>> > Try
>> > (call-process "bash" nil nil nil "-c" "kill -SYS $$")
>> > "Bad system call"
>>
>> That doesn't translate any exit code, it just handles the signal that
>> the process dies from.
>
> Yes, I know. But Bash catches such signals and then exits with 128 +
> signal number.

No, it doesn't.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#41099: 28.0.50; TRAMP process-file ignores exit status of remote process

[Michael Albinus]

Philipp Stephani <p.stephani2@gmail.com> writes:

> What I suggest here would be something like the following:
>
> (defun tramp-process-file (...)
>   (let ((code (...original code...)))
>     (if (> code 128)
>        ;; Probably a signal
>        (format "Signal %d" (- code 128))
>     code))

Yes. Or maybe there's a chance to access sys_siglist from sysdep.c via
Lisp.

Best regards, Michael.

bug#41099: 28.0.50; TRAMP process-file ignores exit status of remote process

[Michael Albinus]

Philipp Stephani <p.stephani2@gmail.com> writes:

Hi Philipp,

> (defun tramp-process-file (...)
>   (let ((code (...original code...)))
>     (if (> code 128)
>        ;; Probably a signal
>        (format "Signal %d" (- code 128))
>     code))

I've pushed a patch to master along these lines.

Best regards, Michael.

bug#41099: 28.0.50; TRAMP process-file ignores exit status of remote process

[Philipp Stephani]

Am Do., 7. Mai 2020 um 10:29 Uhr schrieb Michael Albinus
<michael.albinus@gmx.de>:
>
> Philipp Stephani <p.stephani2@gmail.com> writes:
>
> Hi Philipp,
>
> > (defun tramp-process-file (...)
> >   (let ((code (...original code...)))
> >     (if (> code 128)
> >        ;; Probably a signal
> >        (format "Signal %d" (- code 128))
> >     code))
>
> I've pushed a patch to master along these lines.

Thanks again.

bug#41099: 28.0.50; TRAMP process-file ignores exit status of remote process

[Noam Postavsky]

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

> Philipp Stephani <p.stephani2@gmail.com> writes:
>
>> (defun tramp-process-file (...)
>>   (let ((code (...original code...)))
>>     (if (> code 128)
>>        ;; Probably a signal
>>        (format "Signal %d" (- code 128))
>>     code))
>
> I've pushed a patch to master along these lines.

I don't think this is sufficiently reliable.  With current master:

    (let ((default-directory "/sudo::/home/npostavs/.emacs.d/"))
      (process-file "git" nil nil nil "merge-base"))
    ;=> "Signal 1"

    (let ((default-directory "/home/npostavs/.emacs.d/"))
      (process-file "git" nil nil nil "merge-base"))
    ;=> 129

bug#41099: 28.0.50; TRAMP process-file ignores exit status of remote process

[Michael Albinus]

Noam Postavsky <npostavs@gmail.com> writes:

Hi Noam,

>>> (defun tramp-process-file (...)
>>>   (let ((code (...original code...)))
>>>     (if (> code 128)
>>>        ;; Probably a signal
>>>        (format "Signal %d" (- code 128))
>>>     code))
>>
>> I've pushed a patch to master along these lines.
>
> I don't think this is sufficiently reliable.  With current master:
>
>     (let ((default-directory "/sudo::/home/npostavs/.emacs.d/"))
>       (process-file "git" nil nil nil "merge-base"))
>     ;=> "Signal 1"
>
>     (let ((default-directory "/home/npostavs/.emacs.d/"))
>       (process-file "git" nil nil nil "merge-base"))
>     ;=> 129

I see. A short test shows, that git is using exit code 129 in case of
error in invocation, although it isn't documented in the man pages.

Hmm, this seems to be a contradiction to the specification of reserved
exit codes, as described in <https://tldp.org/LDP/abs/html/exitcodes.html>.
We cannot change git, so either

- we keep Tramp's process-file implementation as it is,
- we don't return a string in case a signal has interrupted the process,
- we install trap handlers in the remote shell in order to let Tramp
  detect signals reliably.

The last alternative might be the best approach to keep the process-file
spec. But it sounds expensive to me, and people already complain about
Tramp performance. Do we want to go this way?

Best regards, Michael.

bug#41099: 28.0.50; TRAMP process-file ignores exit status of remote process

[Philipp Stephani]

Am Do., 14. Mai 2020 um 13:00 Uhr schrieb Michael Albinus
<michael.albinus@gmx.de>:
>
> Noam Postavsky <npostavs@gmail.com> writes:
>
> Hi Noam,
>
> >>> (defun tramp-process-file (...)
> >>>   (let ((code (...original code...)))
> >>>     (if (> code 128)
> >>>        ;; Probably a signal
> >>>        (format "Signal %d" (- code 128))
> >>>     code))
> >>
> >> I've pushed a patch to master along these lines.
> >
> > I don't think this is sufficiently reliable.  With current master:
> >
> >     (let ((default-directory "/sudo::/home/npostavs/.emacs.d/"))
> >       (process-file "git" nil nil nil "merge-base"))
> >     ;=> "Signal 1"
> >
> >     (let ((default-directory "/home/npostavs/.emacs.d/"))
> >       (process-file "git" nil nil nil "merge-base"))
> >     ;=> 129
>
> I see. A short test shows, that git is using exit code 129 in case of
> error in invocation, although it isn't documented in the man pages.
>
> Hmm, this seems to be a contradiction to the specification of reserved
> exit codes, as described in <https://tldp.org/LDP/abs/html/exitcodes.html>.
> We cannot change git

We can at least file a bug against Git.

> so either
>
> - we keep Tramp's process-file implementation as it is,

I'd (naturally) prefer that way. Exit codes > 128 are nonportable, as
they don't allow shells to detect signals.

> - we don't return a string in case a signal has interrupted the process,
> - we install trap handlers in the remote shell in order to let Tramp
>   detect signals reliably.
>

Maybe I'm missing something, but I don't understand how this could
work. Bash trap handlers only catch signals sent to the current
process, not to subprocesses:

$ trap 'echo SIGSYS caught' SYS
$ bash -c 'kill -SYS $$'
Bad system call: 12

Note that the trap handler isn't executed.

bug#41099: 28.0.50; TRAMP process-file ignores exit status of remote process

[Andreas Schwab]

On Mai 14 2020, Philipp Stephani wrote:

> I'd (naturally) prefer that way. Exit codes > 128 are nonportable, as
> they don't allow shells to detect signals.

This is not true.  An exit code can be up to 255.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#41099: 28.0.50; TRAMP process-file ignores exit status of remote process

[Noam Postavsky]

Philipp Stephani <p.stephani2@gmail.com> writes:

> Am Do., 14. Mai 2020 um 13:00 Uhr schrieb Michael Albinus
> <michael.albinus@gmx.de>:

>> I see. A short test shows, that git is using exit code 129 in case of
>> error in invocation, although it isn't documented in the man pages.
>>
>> Hmm, this seems to be a contradiction to the specification of reserved
>> exit codes, as described in <https://tldp.org/LDP/abs/html/exitcodes.html>.
>> We cannot change git
>
> We can at least file a bug against Git.
>
>> so either
>>
>> - we keep Tramp's process-file implementation as it is,
>
> I'd (naturally) prefer that way. Exit codes > 128 are nonportable, as
> they don't allow shells to detect signals.

I don't think this is a correct description.  Bash has the convention
that it uses codes > 128 to indicate commands terminated by signals.
But processes other than bash (like git) don't necessarily follow this
convention.  The shell can still detect the signals, it's shell
*scripts* that will have the problem (when running commands that use
exit codes > 128).

>> - we don't return a string in case a signal has interrupted the process,

Since we don't have a reliable way to detect signals, I think this is
the only viable option.

bug#41099: 28.0.50; TRAMP process-file ignores exit status of remote process

[Philipp Stephani]

Am Do., 14. Mai 2020 um 16:07 Uhr schrieb Noam Postavsky <npostavs@gmail.com>:
>
> Philipp Stephani <p.stephani2@gmail.com> writes:
>
> > Am Do., 14. Mai 2020 um 13:00 Uhr schrieb Michael Albinus
> > <michael.albinus@gmx.de>:
>
> >> I see. A short test shows, that git is using exit code 129 in case of
> >> error in invocation, although it isn't documented in the man pages.
> >>
> >> Hmm, this seems to be a contradiction to the specification of reserved
> >> exit codes, as described in <https://tldp.org/LDP/abs/html/exitcodes.html>.
> >> We cannot change git
> >
> > We can at least file a bug against Git.
> >
> >> so either
> >>
> >> - we keep Tramp's process-file implementation as it is,
> >
> > I'd (naturally) prefer that way. Exit codes > 128 are nonportable, as
> > they don't allow shells to detect signals.
>
> I don't think this is a correct description.  Bash has the convention
> that it uses codes > 128 to indicate commands terminated by signals.
> But processes other than bash (like git) don't necessarily follow this
> convention.  The shell can still detect the signals, it's shell
> *scripts* that will have the problem (when running commands that use
> exit codes > 128).

Yes, I mean scripts here. (TRAMP essentially runs a bunch of shell scripts.)
Since Unix binaries get invoked from shell scripts regularly, they
better behave in a predictable way. Bash scripts will regularly assume
that an exit code > 128 means termination by signal, so these binaries
are not portable in that sense.

>
> >> - we don't return a string in case a signal has interrupted the process,
>
> Since we don't have a reliable way to detect signals, I think this is
> the only viable option.

I'd expect the vast majority of programs to avoid such exit codes,
precisely because they would want to allow portable usage in shell
scripts. So I expect that the current behavior in master provides the
"correct" result in the majority of cases.

bug#41099: 28.0.50; TRAMP process-file ignores exit status of remote process

[Michael Albinus]

Philipp Stephani <p.stephani2@gmail.com> writes:

Hi Philipp & Noam,

>> >> - we don't return a string in case a signal has interrupted the process,
>>
>> Since we don't have a reliable way to detect signals, I think this is
>> the only viable option.
>
> I'd expect the vast majority of programs to avoid such exit codes,
> precisely because they would want to allow portable usage in shell
> scripts. So I expect that the current behavior in master provides the
> "correct" result in the majority of cases.

I understand (and sympathize) both positions. However, Tramp has
returned for decades no strings for process-file, so I don't expect any
code in the wild which expects this.

What about a user option, tramp-process-file-return-signal-string? If
non-nil, it returns a string when a signal is assumed for exit codes >
128. If nil (the default), the exit code is always returned as
natnum. This would also fit the principle of least surprise, because
Tramp hasn't returned strings since ever.

A better variable name would also be appreciated :-)

Best regards, Michael.

bug#41099: 28.0.50; TRAMP process-file ignores exit status of remote process

[Michael Albinus]

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

Hi,

> What about a user option, tramp-process-file-return-signal-string? If
> non-nil, it returns a string when a signal is assumed for exit codes >
> 128. If nil (the default), the exit code is always returned as
> natnum. This would also fit the principle of least surprise, because
> Tramp hasn't returned strings since ever.
>
> A better variable name would also be appreciated :-)

Finally, its name is process-file-return-signal-string. Pushed to master.

Best regards, Michael.

bug#41099: 28.0.50; TRAMP process-file ignores exit status of remote process

[Dmitry Gutov]

Hi Michael,

On 14.05.2020 18:49, Michael Albinus wrote:
> I understand (and sympathize) both positions. However, Tramp has
> returned for decades no strings for process-file, so I don't expect any
> code in the wild which expects this.

But is there code in the wild that expects the _current_ behavior?

It sounds rather odd to me, given that such code would only be intended 
to run on remote systems, but never on the local one. Is that about right?

bug#41099: 28.0.50; TRAMP process-file ignores exit status of remote process

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

> Hi Michael,

Hi Dmitry,

> On 14.05.2020 18:49, Michael Albinus wrote:
>> I understand (and sympathize) both positions. However, Tramp has
>> returned for decades no strings for process-file, so I don't expect any
>> code in the wild which expects this.
>
> But is there code in the wild that expects the _current_ behavior?

Don't know. But at least Philipp has reported this inconsistency, so
there are prople who care.

> It sounds rather odd to me, given that such code would only be
> intended to run on remote systems, but never on the local one. Is that
> about right?

Emacs has no problem to detect, whether a local process has been
interrupted by a signal. It does it on C level.

Best regards, Michael.

bug#41099: 28.0.50; TRAMP process-file ignores exit status of remote process

[Dmitry Gutov]

On 16.05.2020 15:19, Michael Albinus wrote:

>> On 14.05.2020 18:49, Michael Albinus wrote:
>>> I understand (and sympathize) both positions. However, Tramp has
>>> returned for decades no strings for process-file, so I don't expect any
>>> code in the wild which expects this.
>>
>> But is there code in the wild that expects the _current_ behavior?
> 
> Don't know. But at least Philipp has reported this inconsistency, so
> there are prople who care.

Care for the remote case to behave like the local one, right? Not the 
reverse?

>> It sounds rather odd to me, given that such code would only be
>> intended to run on remote systems, but never on the local one. Is that
>> about right?
> 
> Emacs has no problem to detect, whether a local process has been
> interrupted by a signal. It does it on C level.

OK, so if I understand you right, Tramp ends up doing some extra 
computations to get that info, and that makes it slower. I suppose this 
could be a reason to make the "correct" behavior disabled by default.

bug#41099: 28.0.50; TRAMP process-file ignores exit status of remote process

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

Hi Dmitry,

> OK, so if I understand you right, Tramp ends up doing some extra
> computations to get that info, and that makes it slower. I suppose
> this could be a reason to make the "correct" behavior disabled by
> default.

That's not the problem. Tramp cannot determine reliably, whether a
remote process has been interrupted by a signal. It uses a heuristic
(all exit codes greater than 128), but Noam has shown that this is not
bullet-proof. See the discussion in this bug.

Best regards, Michael.

bug#41099: 28.0.50; TRAMP process-file ignores exit status of remote process

[Philipp Stephani]

Am Sa., 16. Mai 2020 um 14:19 Uhr schrieb Michael Albinus
<michael.albinus@gmx.de>:
>
> Dmitry Gutov <dgutov@yandex.ru> writes:
>
> > Hi Michael,
>
> Hi Dmitry,
>
> > On 14.05.2020 18:49, Michael Albinus wrote:
> >> I understand (and sympathize) both positions. However, Tramp has
> >> returned for decades no strings for process-file, so I don't expect any
> >> code in the wild which expects this.
> >
> > But is there code in the wild that expects the _current_ behavior?
>
> Don't know. But at least Philipp has reported this inconsistency, so
> there are prople who care.

To be fair, I care far more about the initial bug report (correct
treatment of exit codes below 128). Many programs follow the
convention to treat small nonzero exit codes as "expected" errors
(e.g. grep: 1 means no match found) and higher exit codes as
"unexpected" errors. In that light a distinction between small exit
codes is more important than trying to achieve 100% correctness when
it comes to signals. So I'm definitely fine with adding a
customization option: anything better would likely be more complex
than is warranted.

bug#41099: 28.0.50; TRAMP process-file ignores exit status of remote process

[Michael Albinus]

Philipp Stephani <p.stephani2@gmail.com> writes:

Hi Philipp,

> To be fair, I care far more about the initial bug report (correct
> treatment of exit codes below 128). Many programs follow the
> convention to treat small nonzero exit codes as "expected" errors
> (e.g. grep: 1 means no match found) and higher exit codes as
> "unexpected" errors. In that light a distinction between small exit
> codes is more important than trying to achieve 100% correctness when
> it comes to signals. So I'm definitely fine with adding a
> customization option: anything better would likely be more complex
> than is warranted.

Thanks for the feedback. Since the user option has been added already, I
believe we could regard this bug as solved entirely.

Best regards, Michael.

bug#41099: 28.0.50; TRAMP process-file ignores exit status of remote process

[Philipp Stephani]

Am Sa., 23. Mai 2020 um 21:35 Uhr schrieb Michael Albinus
<michael.albinus@gmx.de>:
>
> Philipp Stephani <p.stephani2@gmail.com> writes:
>
> Hi Philipp,
>
> > To be fair, I care far more about the initial bug report (correct
> > treatment of exit codes below 128). Many programs follow the
> > convention to treat small nonzero exit codes as "expected" errors
> > (e.g. grep: 1 means no match found) and higher exit codes as
> > "unexpected" errors. In that light a distinction between small exit
> > codes is more important than trying to achieve 100% correctness when
> > it comes to signals. So I'm definitely fine with adding a
> > customization option: anything better would likely be more complex
> > than is warranted.
>
> Thanks for the feedback. Since the user option has been added already, I
> believe we could regard this bug as solved entirely.

Agreed.