bug#62093: [PATCH] Let processes read nothing from stdin in tramp

[Michael Albinus <michael.albinus@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 2399 bytes --]

Aleksander Trofimowicz <trof@n90.eu> writes:

> Hi Michael,

Hi Alexander,

>> The failed test runs have in common that there is a complex Tramp target
>> for the test using multi-hops, like above. I suspect a problem with
>> timing due to the several hops, but I don't know exactly what's up.
>>
> Triggered by your multi-hop tests (host B via host A) I run git-apply
> actions over Tramp with the patch I submitted. I settled with 3
> different test environment configurations; all of them should provide
> the same functionality:
>
> 1. An explicit multi-hop target "/ssh:host_A|/ssh:host_B:..."
>
> 2. The SSH client option ProxyCommand set to "ssh host_A nc host_B 22" and
> simple "/ssh:host_B"
>
> 3. The SSH client option ProxyJump set to "host_A" and simple "/ssh:host_B"
>
> tramp-use-connection-share was set to nil in each case.
>
> It worked as expected only in the first two cases. As far as the last one
> is concerned, the workflow wasn't stalled, but it turned out to be no-op
> after all.
>
> It seems the last test environment enables the most responsive data
> stream: no additional userland process is forked on a middle box and
> Nagle's algorithm is disabled for all TCP connections involved (which is
> not true for the case no 2.). In the end such results might corroborate
> your theory.

Well, I've tried deeper debugging with this. But for whatever values
I've taken for stty's min and time in tramp-sh-handle-make-process,
there were always cases it didn't work. Rare cases, and not always
reproducible due to race conditions, but they exist.

Going a step back to magit, I see the following comment in magit-start-process:

--8<---------------cut here---------------start------------->8---
               ;; Don't use a pty, because it would set icrnl
               ;; which would modify the input (issue #20).
--8<---------------cut here---------------end--------------->8---

I cannot speak for the local case. But in Tramp, we need to set "stty
-icanon ..." for the pipe connection type in order avoid blocking
situations with larger hunks of data, see the comment there. And the use
case of magit-start-process would be better with the connection type
pty, at least when calling Tramp.

The appended (rather naïve) patch fixes the reported use case in my
local enviroment.

> at

Best regards, Michael.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 1146 bytes --]

*** /home/albinus/.emacs.d/elpa/magit-20231103.1516/magit-process.el~	2023-11-04 11:43:11.848351502 +0100
--- /home/albinus/.emacs.d/elpa/magit-20231103.1516/magit-process.el	2023-11-11 13:26:23.212027014 +0100
***************
*** 583,589 ****
          (let ((process-connection-type
                 ;; Don't use a pty, because it would set icrnl
                 ;; which would modify the input (issue #20).
!                (and (not input) magit-process-connection-type))
                (process-environment (magit-process-environment))
                (default-process-coding-system (magit--process-coding-system)))
            (apply #'start-file-process
--- 583,590 ----
          (let ((process-connection-type
                 ;; Don't use a pty, because it would set icrnl
                 ;; which would modify the input (issue #20).
! 	       (or (and (file-remote-p default-directory) t)
! 		   (and (not input) magit-process-connection-type)))
                (process-environment (magit-process-environment))
                (default-process-coding-system (magit--process-coding-system)))
            (apply #'start-file-process