bug#45357: [PATCH] * lisp/man.el (Man-getpage-in-background): always use shell-file-name

bug#45357: [PATCH] * lisp/man.el (Man-getpage-in-background): always use shell-file-name

[Nika Otiashvili]

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

Hello,
First time sending a patch to bug-gnu-emacs, not sure if this is the
correct format.
Please indicate any errors I made in my formatting of the email or the
attached patch file.

Regards,
Nika Otiashvili

[-- Attachment #2: 0001-lisp-man.el-Man-getpage-in-background-always-use-she.patch --]
[-- Type: text/x-patch, Size: 1885 bytes --]

From e27cc04bc2a1ca10d5e89ad585fc59eb0d462b8d Mon Sep 17 00:00:00 2001
From: Nika Otiashvili <nikaoto@gmail.com>
Date: Mon, 21 Dec 2020 23:14:28 +0400
Subject: [PATCH] * lisp/man.el (Man-getpage-in-background): always use
 shell-file-name

Copyright-paperwork-exempt: yes
; Reasons for change:
;
; * M-m man invokes Man-getpage-in-background, which launches
;   shell-file-name only when system-type is cygwin or windows-nt,
;   otherwise it launches /bin/sh. Some users may not have /bin/sh.
;
; * /bin/sh, when launched this way, runs through the user's
;   runtime configurations (.bashrc, .zshrc ...), however it does
;   not launch subshells even if there are appropriate shebangs at
;   the top of the rc files. Neither "#! /bin/bash", nor
;   "#! /usr/bin/env bash" launches bash. This causes the process
;   to crash if the rc files are incompatible with the user's
;   /bin/sh and instead of the rendered man page, it gives an error
;   message in the new buffer.
;
; * It is reasonable for the user to expect that any function that
;   launches a subshell under emacs should respect the
;   shell-file-name variable that they have configured.
---
 lisp/man.el | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/lisp/man.el b/lisp/man.el
index c914ec34b97..c257536aeca 100644
--- a/lisp/man.el
+++ b/lisp/man.el
@@ -1123,11 +1123,7 @@ Man-getpage-in-background
 	(Man-start-calling
 	 (if (fboundp 'make-process)
 	     (let ((proc (start-process
-			  manual-program buffer
-			  (if (memq system-type '(cygwin windows-nt))
-			      shell-file-name
-			    "sh")
-			  shell-command-switch
+                          manual-program buffer shell-file-name shell-command-switch
 			  (format (Man-build-man-command) man-args))))
 	       (set-process-sentinel proc 'Man-bgproc-sentinel)
 	       (set-process-filter proc 'Man-bgproc-filter))
-- 
2.20.1

bug#45357: [PATCH] * lisp/man.el (Man-getpage-in-background): always use shell-file-name

[Eli Zaretskii]

> From: Nika Otiashvili <nikaoto@gmail.com>
> Date: Mon, 21 Dec 2020 23:24:13 +0400
> 
> First time sending a patch to bug-gnu-emacs, not sure if this is the
> correct format.
> Please indicate any errors I made in my formatting of the email or the
> attached patch file.

In terms of formatting the patch, I see no problems with your
submission.

My problem is more a conceptual one: it is not trivial to replace
/bin/sh with any other shell, because they work differently.  The
command in question builds a complex shell command to run, and it
isn't obvious that any user shell will be able to run it.

OTOH, every Posix system I know about does have /bin/sh, so I'm
curious what kind of system did you see where /bin/sh is absent.

And finally, I don't think I understand the issue with .bashrc etc.:
if you launch /bin/sh, it is not supposed to process these init files,
they are processed by bash, zsh, and other shells.

Could you please elaborate on the specific problems you had and tell
more about your system configuration where you bumped into these
problems?

Thanks.

bug#45357: [PATCH] * lisp/man.el (Man-getpage-in-background): always use shell-file-name

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi,

> My problem is more a conceptual one: it is not trivial to replace
> /bin/sh with any other shell, because they work differently.  The
> command in question builds a complex shell command to run, and it
> isn't obvious that any user shell will be able to run it.

That's true. But we could require that shell-file-name points to a Posix
shell, if used in man.el.

Btw, if we were to change man.el, I'd rather propose to use
start-process-shell-command, instead of composing the command manually.

> Could you please elaborate on the specific problems you had and tell
> more about your system configuration where you bumped into these
> problems?

This question still stands, yes.

> Thanks.

Best regards, Michael.

bug#45357: [PATCH] * lisp/man.el (Man-getpage-in-background): always use shell-file-name

[Nika Otiashvili]

Hello,

Thanks for looking at the patch, Eli.

Regarding the rc files, it seems there was an error on my part.
I have (setq shell-command-switch "-cl") in my .emacs. The "-l" flag
runs sh as a login shell, thus it loads my .profile. From there I source my
.bashrc. Since I have bashisms in my .bashrc, sh simply sees them
as syntax errors and exits. I should have inserted a check of $0 inside my
.profile to prevent this.

> OTOH, every Posix system I know about does have /bin/sh, so I'm
> curious what kind of system did you see where /bin/sh is absent.

Initially, I thought Windows users wouldn't have /bin/sh, but the cygwin
and windows-nt checks I removed accounted for that and used
shell-file-name instead of sh.

So, I've concluded that the first two reasons described in my patch
are irrelevant and the last one is indeed very weak. I believe my
patch would not benefit anyone and it only solves a specific problem I
had with my
configuration.

Regards,
Nika Otiashvili

On Tue, Dec 22, 2020 at 10:50 PM Michael Albinus <michael.albinus@gmx.de> wrote:
>
> Eli Zaretskii <eliz@gnu.org> writes:
>
> Hi,
>
> > My problem is more a conceptual one: it is not trivial to replace
> > /bin/sh with any other shell, because they work differently.  The
> > command in question builds a complex shell command to run, and it
> > isn't obvious that any user shell will be able to run it.
>
> That's true. But we could require that shell-file-name points to a Posix
> shell, if used in man.el.
>
> Btw, if we were to change man.el, I'd rather propose to use
> start-process-shell-command, instead of composing the command manually.
>
> > Could you please elaborate on the specific problems you had and tell
> > more about your system configuration where you bumped into these
> > problems?
>
> This question still stands, yes.
>
> > Thanks.
>
> Best regards, Michael.

bug#45357: [PATCH] * lisp/man.el (Man-getpage-in-background): always use shell-file-name

[Michael Albinus]

Nika Otiashvili <nikaoto@gmail.com> writes:

> Hello,

Hi,

> So, I've concluded that the first two reasons described in my patch
> are irrelevant and the last one is indeed very weak. I believe my
> patch would not benefit anyone and it only solves a specific problem I
> had with my
> configuration.

Closing the bug.

> Regards,
> Nika Otiashvili

Best regards, Michael.