I think it is correct and better to spawn login shells by default, these
are not child shells of some shell process, they are entirely new
shells.
There is already an option to change it, default-command. I don't see a
problem spawning two shells, shells are running, started, exited all the
time and fork is not expensive anymore. What exactly is the issue with
this? Do you have any boxes where this is noticeably slower than one
shell? How much?
If you can convince me it isn't a good idea and we need an option, I
would suggest making some new prefix to default-shell like "+", so
"+/bin/sh" doesn't run a login shell.
On Thu, Jun 21, 2012 at 01:38:02AM +0200, J??r??mie Courr??ges-Anglas wrote:
> Hi folks.
>
> As many of us, I'm a happy tmux user.
> There's a small glitch, though, that I have "fixed" in my local tree,
> and I was thinking about sharing it.
>
> I know that tmux spawns login shells by default, and I've read about
> people asking how to disable that. Sadly, the solutions that can be
> tried, currently, are:
> - set default-shell to something appropriate. I don't know which options
> are really available here.
> - set default-command to "ksh" / "/bin/ksh". This leads to exec'ing
> $SHELL to fire default-command. default-command might be set to "ksh",
> or "/bin/ksh". If set to "exec ksh", you should save a call to
> fork(), and an entry in the process table. The problem, imho, is that
> it leads to exec'ing two shells instead of one. Which is (*imho*) not
> elegant.
> - modify the source.
>
> What I propose is that tmux doesn't fire a login shell unless
> explicitely told to. tmux already has an internal login_shell variable,
> which is used in the tmux.c/shell_exec() function, but not in
> window.c/window_pane_spawn(). This variable seems to be properly
> modified when tmux is called as a login shell. Shells shouldn't
> (*imho*) be spawned as login shells by default. This could be a benefit
> for slow machines users (as /etc/profile and ~/.profile might be
> expensive), and matches (still imho) the least astonishment principle.
>
> So, I've used the following patch since a long time, but haven't tested
> it much with tmux set as login shell. I might take a bit of time to
> check it, but I'd prefer people that *really care* to do it. I've also
> updated the manpage to match the behaviour change.
>
> This might very well break some people's setup relying on tmux
> implicitely spawning login shells, but I honestly don't care much about
> that. :P
> The change could be documented in current.html and release notes for the
> portable version.
>
> The following diff introduces a new auto variable and makes xasprintf()
> unable to check the format string (with WARNINGS=Yes), but since the
> format string it trivial, I thought it wasn't such a big problem.
>
> Please don't hesitate to bash me if I've missed something important. :P
>
> Regards.
> --
> J??r??mie Courr??ges-Anglas
> GPG fingerprint: 61DB D9A0 00A4 67CF 2A90 8961 6191 8FBF 06A1 1494
>
> (Replaces last use of literal "tmux" by .Nm, btw)
>
> Index: names.c
> ===================================================================
> RCS file: /home/cvs/src/usr.bin/tmux/names.c,v
> retrieving revision 1.15
> diff -u -p -r1.15 names.c
> --- names.c 11 Apr 2012 07:45:30 -0000 1.15
> +++ names.c 20 Jun 2012 22:25:13 -0000
> @@ -64,8 +64,8 @@ window_name_callback(unused int fd, unus
> wname = default_window_name(w);
> else {
> /*
> - * If tmux is using the default command, it will be a login
> - * shell and argv[0] may have a - prefix. Remove this if it is
> + * If tmux is using the default command, it may be a login
> + * shell thus argv[0] could have a - prefix. Remove this if it
> is
> * present. Ick.
> */
> if (w->active->cmd != NULL && *w->active->cmd == '\0' &&
> Index: tmux.1
> ===================================================================
> RCS file: /home/cvs/src/usr.bin/tmux/tmux.1,v
> retrieving revision 1.294
> diff -u -p -r1.294 tmux.1
> --- tmux.1 22 May 2012 11:35:37 -0000 1.294
> +++ tmux.1 20 Jun 2012 18:36:25 -0000
> @@ -156,8 +156,11 @@ signal may be sent to the
> server process to recreate it.
> .It Fl l
> Behave as a login shell.
> -This flag currently has no effect and is for compatibility with other shells
> -when using tmux as a login shell.
> +Needed for compatibility with other shells when using
> +.Nm
> +as a login shell. See the
> +.Ic default-shell
> +option.
> .It Fl q
> Set the
> .Ic quiet
> @@ -1961,7 +1964,7 @@ which may be any
> command.
> The default is an empty string, which instructs
> .Nm
> -to create a login shell using the value of the
> +to create a shell using the value of the
> .Ic default-shell
> option.
> .It Ic default-path Ar path
> @@ -1975,7 +1978,7 @@ flag to
> .Ic new-window .
> .It Ic default-shell Ar path
> Specify the default shell.
> -This is used as the login shell for new windows when the
> +This is used as the shell for new windows when the
> .Ic default-command
> option is set to empty, and must be the full path of the executable.
> When started
> @@ -1989,6 +1992,10 @@ or
> This option should be configured when
> .Nm
> is used as a login shell.
> +Note that if
> +.Nm
> +is used as a login shell, then child shell processes are also launched as
> +login shells.
> .It Ic default-terminal Ar terminal
> Set the default terminal for new windows created in this session - the
> default value of the
> Index: window.c
> ===================================================================
> RCS file: /home/cvs/src/usr.bin/tmux/window.c,v
> retrieving revision 1.80
> diff -u -p -r1.80 window.c
> --- window.c 28 May 2012 08:55:43 -0000 1.80
> +++ window.c 20 Jun 2012 21:36:17 -0000
> @@ -686,7 +686,7 @@ window_pane_spawn(struct window_pane *wp
> {
> struct winsize ws;
> char *argv0, paneid[16];
> - const char *ptr;
> + const char *ptr, *shell_fmt;
> struct termios tio2;
>
> if (wp->fd != -1) {
> @@ -752,11 +752,12 @@ window_pane_spawn(struct window_pane *wp
> fatal("execl failed");
> }
>
> - /* No command; fork a login shell. */
> + /* No command; fork a shell. */
> + shell_fmt = login_shell ? "-%s" : "%s";
> if (ptr != NULL && *(ptr + 1) != '\0')
> - xasprintf(&argv0, "-%s", ptr + 1);
> + xasprintf(&argv0, shell_fmt, ptr + 1);
> else
> - xasprintf(&argv0, "-%s", wp->shell);
> + xasprintf(&argv0, shell_fmt, wp->shell);
> execl(wp->shell, argv0, (char *) NULL);
> fatal("execl failed");
> }
