Re: [RFC/PATCH] shell: allow 'help' command to disable interactive shell

[Jeff King <peff@peff.net>]

On Sun, Feb 10, 2013 at 05:20:16PM -0800, Jonathan Nieder wrote:

> > This of course now means that the ~/git-shell-commands should not be
> > empty, since that is where this default command also will be present.
> 
> How about this?

I like the general direction this is going, but:

> +When run interactively (with no arguments), 'git-shell' will
> +automatically run `~/git-shell-commands/help` on startup, provided it
> +exists.  If the 'help' command fails then the interactive shell is
> +aborted.

Doesn't that mean that people who currently do allow interactive access
and have a ~/git-shell-commands/help (that returns zero) will get
spammed by its as a motd each time they connect?

To be honest, I am not really clear on what interactive git-shell is
used for. AFAIK, it does nothing unless you have set up custom commands,
and I have never actually seen them in the wild. So maybe it is not a
big deal.

If I understand correctly, calling it "check-interactive", "greeting",
or something instead of "help" would be sufficient, and then you
wouldn't have to worry about backwards compatibility.

> diff --git a/shell.c b/shell.c
> index 84b237fe..3abc2b84 100644
> --- a/shell.c
> +++ b/shell.c
> @@ -63,10 +63,16 @@ static void cd_to_homedir(void)
>  
>  static void run_shell(void)
>  {
> -	int done = 0;
> +	int done = 0, status;
>  	static const char *help_argv[] = { HELP_COMMAND, NULL };
>  	/* Print help if enabled */
> -	run_command_v_opt(help_argv, RUN_SILENT_EXEC_FAILURE);
> +	status = run_command_v_opt(help_argv, RUN_SILENT_EXEC_FAILURE);
> +	if (!status)
> +		; /* success */
> +	else if (status == -1 && errno == ENOENT)
> +		; /* help disabled */
> +	else
> +		exit(status);

This kicks in only when there is no command given, right? So if I ran
"ssh example.com", it would give me the help message rather than (or in
addition) giving me interactive access.

What about "ssh example.com foo"? Do we want to allow a custom message
there, too (it might be different there; e.g., an allowed list of
commands might make more sense)?

-Peff