[PATCH] shell: Rewrite documentation and improve error message

[PATCH] shell: Rewrite documentation and improve error message

[Ramkumar Ramachandra]

Update the documentation of 'git shell' to mention the interactive
mode and COMMAND_DIR. Also provide a hint when interactive mode is not
available in the shell.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Cc: Greg Brockman <gdb@MIT.EDU>
Cc: Petr Baudis <pasky@suse.cz>
---
 Documentation/git-receive-pack.txt |    2 +-
 Documentation/git-shell.txt        |   27 ++++++++++++++++-----------
 shell.c                            |    7 +++++--
 3 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/Documentation/git-receive-pack.txt b/Documentation/git-receive-pack.txt
index 2790eeb..2b07e44 100644
--- a/Documentation/git-receive-pack.txt
+++ b/Documentation/git-receive-pack.txt
@@ -8,7 +8,7 @@ git-receive-pack - Receive what is pushed into the repository
 
 SYNOPSIS
 --------
-'git-receive-pack' <directory>
+'git receive-pack' <directory>
 
 DESCRIPTION
 -----------
diff --git a/Documentation/git-shell.txt b/Documentation/git-shell.txt
index 0f3ad81..c9f1dc9 100644
--- a/Documentation/git-shell.txt
+++ b/Documentation/git-shell.txt
@@ -3,24 +3,29 @@ git-shell(1)
 
 NAME
 ----
-git-shell - Restricted login shell for GIT-only SSH access
+git-shell - Restricted login shell for Git-only SSH access
 
 
 SYNOPSIS
 --------
-'$(git --exec-path)/git-shell' -c <command> <argument>
+'git shell' [-c <command> <argument>]
 
 DESCRIPTION
 -----------
-This is meant to be used as a login shell for SSH accounts you want
-to restrict to GIT pull/push access only. It permits execution only
-of server-side GIT commands implementing the pull/push functionality.
-The commands can be executed only by the '-c' option; the shell is not
-interactive.
-
-Currently, only four commands are permitted to be called, 'git-receive-pack'
-'git-upload-pack' and 'git-upload-archive' with a single required argument, or
-'cvs server' (to invoke 'git-cvsserver').
+
+A login shell for SSH accounts to provide restricted Git access. When
+'-c' is given, the program executes <command> non-interactively;
+<command> can be one of 'git receive-pack', 'git upload-pack', 'git
+upload-archive', 'cvs server', or a command in COMMAND_DIR. The shell
+is started in interactive mode when no arguments are given; in this
+case, COMMAND_DIR must exist, and any of the executables in them can
+be invoked.
+
+'cvs server' is a special command which executes git-cvsserver.
+
+COMMAND_DIR is the path 'git-shell-commands' in the user's home
+directory. The user must have read and execute permissions to the
+directory for it to be useful.
 
 Author
 ------
diff --git a/shell.c b/shell.c
index ffed615..3fb804e 100644
--- a/shell.c
+++ b/shell.c
@@ -152,8 +152,11 @@ int main(int argc, char **argv)
 	} else if (argc == 1) {
 		/* Allow the user to run an interactive shell */
 		cd_to_homedir();
-		if (access(COMMAND_DIR, R_OK | X_OK) == -1)
-			die("Sorry, the interactive git-shell is not enabled");
+		if (access(COMMAND_DIR, R_OK | X_OK) == -1) {
+			die("Inteactive git shell is not enabled.\n"
+			    "hint: " COMMAND_DIR " should exist "
+			    "and have read and execute access.");
+		}
 		run_shell();
 		exit(0);
 	} else if (argc != 3 || strcmp(argv[1], "-c")) {
-- 
1.7.2.2.409.gdbb11.dirty

Re: [PATCH] shell: Rewrite documentation and improve error message

[Ramkumar Ramachandra]

Hi,

Ramkumar Ramachandra writes:
> Update the documentation of 'git shell' to mention the interactive
> mode and COMMAND_DIR. Also provide a hint when interactive mode is not
> available in the shell.
> 
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> Cc: Greg Brockman <gdb@MIT.EDU>
> Cc: Petr Baudis <pasky@suse.cz>
> ---
>  Documentation/git-receive-pack.txt |    2 +-

I'm sorry this stray change crept in. It's not relevant to this patch.

> diff --git a/Documentation/git-receive-pack.txt b/Documentation/git-receive-pack.txt
> index 2790eeb..2b07e44 100644
> --- a/Documentation/git-receive-pack.txt
> +++ b/Documentation/git-receive-pack.txt
> @@ -8,7 +8,7 @@ git-receive-pack - Receive what is pushed into the repository
>  
>  SYNOPSIS
>  --------
> -'git-receive-pack' <directory>
> +'git receive-pack' <directory>

Quick fixup:
-- 8< --
diff --git a/Documentation/git-receive-pack.txt b/Documentation/git-receive-pack.txt
index 2b07e44..2790eeb 100644
--- a/Documentation/git-receive-pack.txt
+++ b/Documentation/git-receive-pack.txt
@@ -8,7 +8,7 @@ git-receive-pack - Receive what is pushed into the repository
 
 SYNOPSIS
 --------
-'git receive-pack' <directory>
+'git-receive-pack' <directory>
 
 DESCRIPTION
 -----------

Re: [PATCH] shell: Rewrite documentation and improve error message

[Greg Brockman]

Warning: the following email contains lots of nitpicks.  Proceed at
your own risk.

> +A login shell for SSH accounts to provide restricted Git access. When
> +'-c' is given, the program executes <command> non-interactively;
> +<command> can be one of 'git receive-pack', 'git upload-pack', 'git
> +upload-archive', 'cvs server', or a command in COMMAND_DIR. The shell
> +is started in interactive mode when no arguments are given; in this
> +case, COMMAND_DIR must exist, and any of the executables in them can
s/in them/in it/

> +COMMAND_DIR is the path 'git-shell-commands' in the user's home
> +directory. The user must have read and execute permissions to the
- Maybe instead 'COMMAND_DIR is the path "$HOME/git-shell-commands"'?
> +directory for it to be useful.
- I would be more specific here, and instead change this insertion to
something like: "directory in order to execute commands in it."
- What about adding something like 'Commands in COMMAND_DIR are run
with a cwd of $HOME'
- Perhaps note that if a COMMAND_DIR program is invoked, 'argument' is
parsed as a command-line?

>
>  Author
>  ------
> diff --git a/shell.c b/shell.c
> index ffed615..3fb804e 100644
> --- a/shell.c
> +++ b/shell.c
> @@ -152,8 +152,11 @@ int main(int argc, char **argv)
>        } else if (argc == 1) {
>                /* Allow the user to run an interactive shell */
>                cd_to_homedir();
> -               if (access(COMMAND_DIR, R_OK | X_OK) == -1)
> -                       die("Sorry, the interactive git-shell is not enabled");
> +               if (access(COMMAND_DIR, R_OK | X_OK) == -1) {
> +                       die("Inteactive git shell is not enabled.\n"
s/Inteactive/Interactive/

> +                           "hint: " COMMAND_DIR " should exist "
I would recommend displaying ~/$COMMAND_DIR instead, or
$HOME/COMMAND_DIR.  The latter could be considered an information leak
though.

> +                           "and have read and execute access.");

Sounds fine to me otherwise.  Thanks for starting on this.

[PATCH v2] shell: Rewrite documentation and improve error message

[Ramkumar Ramachandra]

Hi Greg,

Thanks for the review. Here's the inter-diff.

diff --git a/Documentation/git-shell.txt b/Documentation/git-shell.txt
index c9f1dc9..e7c226e 100644
--- a/Documentation/git-shell.txt
+++ b/Documentation/git-shell.txt
@@ -18,14 +18,15 @@ A login shell for SSH accounts to provide restricted Git access. When
 <command> can be one of 'git receive-pack', 'git upload-pack', 'git
 upload-archive', 'cvs server', or a command in COMMAND_DIR. The shell
 is started in interactive mode when no arguments are given; in this
-case, COMMAND_DIR must exist, and any of the executables in them can
-be invoked.
+case, COMMAND_DIR must exist, and any of the executables in it can be
+invoked.
 
 'cvs server' is a special command which executes git-cvsserver.
 
-COMMAND_DIR is the path 'git-shell-commands' in the user's home
-directory. The user must have read and execute permissions to the
-directory for it to be useful.
+COMMAND_DIR is the path "$HOME/git-shell-commands". The user must have
+read and execute permissions to the directory in order to execute the
+programs in it. The programs are executed with a cwd of $HOME, and
+<argument> is passed to them as a command-line argument.
 
 Author
 ------

diff --git a/shell.c b/shell.c
index 3fb804e..0903ac8 100644
--- a/shell.c
+++ b/shell.c
@@ -153,8 +153,8 @@ int main(int argc, char **argv)
 		/* Allow the user to run an interactive shell */
 		cd_to_homedir();
 		if (access(COMMAND_DIR, R_OK | X_OK) == -1) {
-			die("Inteactive git shell is not enabled.\n"
-			    "hint: " COMMAND_DIR " should exist "
+			die("Interactive git shell is not enabled.\n"
+			    "hint: ~/$COMMAND_DIR should exist "
 			    "and have read and execute access.");
 		}
 		run_shell();

-- >8 --
From 01ff9085867ae6be558927cb4d905c441ffedb41 Mon Sep 17 00:00:00 2001
From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Fri, 20 Aug 2010 14:30:06 +0530
Subject: [PATCH v2] shell: Rewrite documentation and improve error message

Update the documentation of 'git shell' to mention the interactive
mode and COMMAND_DIR. Also provide a hint when interactive mode is not
available in the shell.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Cc: Greg Brockman <gdb@MIT.EDU>
---
 Documentation/git-shell.txt |   28 +++++++++++++++++-----------
 shell.c                     |    7 +++++--
 2 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/Documentation/git-shell.txt b/Documentation/git-shell.txt
index 0f3ad81..1e38c70 100644
--- a/Documentation/git-shell.txt
+++ b/Documentation/git-shell.txt
@@ -3,24 +3,30 @@ git-shell(1)
 
 NAME
 ----
-git-shell - Restricted login shell for GIT-only SSH access
+git-shell - Restricted login shell for Git-only SSH access
 
 
 SYNOPSIS
 --------
-'$(git --exec-path)/git-shell' -c <command> <argument>
+'git shell' [-c <command> <argument>]
 
 DESCRIPTION
 -----------
-This is meant to be used as a login shell for SSH accounts you want
-to restrict to GIT pull/push access only. It permits execution only
-of server-side GIT commands implementing the pull/push functionality.
-The commands can be executed only by the '-c' option; the shell is not
-interactive.
-
-Currently, only four commands are permitted to be called, 'git-receive-pack'
-'git-upload-pack' and 'git-upload-archive' with a single required argument, or
-'cvs server' (to invoke 'git-cvsserver').
+
+A login shell for SSH accounts to provide restricted Git access. When
+'-c' is given, the program executes <command> non-interactively;
+<command> can be one of 'git receive-pack', 'git upload-pack', 'git
+upload-archive', 'cvs server', or a command in COMMAND_DIR. The shell
+is started in interactive mode when no arguments are given; in this
+case, COMMAND_DIR must exist, and any of the executables in it can be
+invoked.
+
+'cvs server' is a special command which executes git-cvsserver.
+
+COMMAND_DIR is the path "$HOME/git-shell-commands". The user must have
+read and execute permissions to the directory in order to execute the
+programs in it. The programs are executed with a cwd of $HOME, and
+<argument> is passed to them as a command-line argument.
 
 Author
 ------
diff --git a/shell.c b/shell.c
index ffed615..0903ac8 100644
--- a/shell.c
+++ b/shell.c
@@ -152,8 +152,11 @@ int main(int argc, char **argv)
 	} else if (argc == 1) {
 		/* Allow the user to run an interactive shell */
 		cd_to_homedir();
-		if (access(COMMAND_DIR, R_OK | X_OK) == -1)
-			die("Sorry, the interactive git-shell is not enabled");
+		if (access(COMMAND_DIR, R_OK | X_OK) == -1) {
+			die("Interactive git shell is not enabled.\n"
+			    "hint: ~/$COMMAND_DIR should exist "
+			    "and have read and execute access.");
+		}
 		run_shell();
 		exit(0);
 	} else if (argc != 3 || strcmp(argv[1], "-c")) {
-- 
1.7.2.2.409.gdbb11.dirty

Re: [PATCH v2] shell: Rewrite documentation and improve error message

[Greg Brockman]

> -COMMAND_DIR is the path 'git-shell-commands' in the user's home
> -directory. The user must have read and execute permissions to the
> -directory for it to be useful.
> +COMMAND_DIR is the path "$HOME/git-shell-commands". The user must have
> +read and execute permissions to the directory in order to execute the
> +programs in it. The programs are executed with a cwd of $HOME, and
> +<argument> is passed to them as a command-line argument.
Sorry, I guess I wasn't clear enough with what I meant here.  The
<argument> is actually parsed as a command-line string (so split on
spaces respecting quotes), not just passed as a raw command-line
argument.

>                if (access(COMMAND_DIR, R_OK | X_OK) == -1) {
> -                       die("Inteactive git shell is not enabled.\n"
> -                           "hint: " COMMAND_DIR " should exist "
> +                       die("Interactive git shell is not enabled.\n"
> +                           "hint: ~/$COMMAND_DIR should exist "
>                            "and have read and execute access.");
>                }
>                run_shell();
Whoops, I'm not sure why I put a $ here.  "hint: ~/" COMMAND_DIR "
should exist " is definitely what you want.

Other than that, looks fine to me.

[PATCH v3] shell: Rewrite documentation and improve error message

[Ramkumar Ramachandra]

Update the documentation of 'git shell' to mention the interactive
mode and COMMAND_DIR. Also provide a hint when interactive mode is not
available in the shell.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Cc: Greg Brockman <gdb@MIT.EDU>
---
 Interdiff:
 diff --git a/Documentation/git-shell.txt b/Documentation/git-shell.txt
 index 1e38c70..6403126 100644
 --- a/Documentation/git-shell.txt
 +++ b/Documentation/git-shell.txt
 @@ -26,7 +26,7 @@ invoked.
  COMMAND_DIR is the path "$HOME/git-shell-commands". The user must have
  read and execute permissions to the directory in order to execute the
  programs in it. The programs are executed with a cwd of $HOME, and
 -<argument> is passed to them as a command-line argument.
 +<argument> is parsed as a command-line string.
  
  Author
  ------
 	Modified shell.c
 diff --git a/shell.c b/shell.c
 index 0903ac8..6aade3d 100644
 --- a/shell.c
 +++ b/shell.c
 @@ -154,7 +154,7 @@ int main(int argc, char **argv)
  		cd_to_homedir();
  		if (access(COMMAND_DIR, R_OK | X_OK) == -1) {
  			die("Interactive git shell is not enabled.\n"
 -			    "hint: ~/$COMMAND_DIR should exist "
 +			    "hint: ~/" $COMMAND_DIR " should exist "
  			    "and have read and execute access.");
  		}
  		run_shell();

 Documentation/git-shell.txt |   28 +++++++++++++++++-----------
 shell.c                     |    7 +++++--
 2 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/Documentation/git-shell.txt b/Documentation/git-shell.txt
index 0f3ad81..6403126 100644
--- a/Documentation/git-shell.txt
+++ b/Documentation/git-shell.txt
@@ -3,24 +3,30 @@ git-shell(1)
 
 NAME
 ----
-git-shell - Restricted login shell for GIT-only SSH access
+git-shell - Restricted login shell for Git-only SSH access
 
 
 SYNOPSIS
 --------
-'$(git --exec-path)/git-shell' -c <command> <argument>
+'git shell' [-c <command> <argument>]
 
 DESCRIPTION
 -----------
-This is meant to be used as a login shell for SSH accounts you want
-to restrict to GIT pull/push access only. It permits execution only
-of server-side GIT commands implementing the pull/push functionality.
-The commands can be executed only by the '-c' option; the shell is not
-interactive.
-
-Currently, only four commands are permitted to be called, 'git-receive-pack'
-'git-upload-pack' and 'git-upload-archive' with a single required argument, or
-'cvs server' (to invoke 'git-cvsserver').
+
+A login shell for SSH accounts to provide restricted Git access. When
+'-c' is given, the program executes <command> non-interactively;
+<command> can be one of 'git receive-pack', 'git upload-pack', 'git
+upload-archive', 'cvs server', or a command in COMMAND_DIR. The shell
+is started in interactive mode when no arguments are given; in this
+case, COMMAND_DIR must exist, and any of the executables in it can be
+invoked.
+
+'cvs server' is a special command which executes git-cvsserver.
+
+COMMAND_DIR is the path "$HOME/git-shell-commands". The user must have
+read and execute permissions to the directory in order to execute the
+programs in it. The programs are executed with a cwd of $HOME, and
+<argument> is parsed as a command-line string.
 
 Author
 ------
diff --git a/shell.c b/shell.c
index ffed615..6aade3d 100644
--- a/shell.c
+++ b/shell.c
@@ -152,8 +152,11 @@ int main(int argc, char **argv)
 	} else if (argc == 1) {
 		/* Allow the user to run an interactive shell */
 		cd_to_homedir();
-		if (access(COMMAND_DIR, R_OK | X_OK) == -1)
-			die("Sorry, the interactive git-shell is not enabled");
+		if (access(COMMAND_DIR, R_OK | X_OK) == -1) {
+			die("Interactive git shell is not enabled.\n"
+			    "hint: ~/" $COMMAND_DIR " should exist "
+			    "and have read and execute access.");
+		}
 		run_shell();
 		exit(0);
 	} else if (argc != 3 || strcmp(argv[1], "-c")) {
-- 
1.7.2.2.409.gdbb11.dirty

Re: [PATCH v3] shell: Rewrite documentation and improve error message

[Greg Brockman]

>  -                          "hint: ~/$COMMAND_DIR should exist "
>  +                          "hint: ~/" $COMMAND_DIR " should exist "
There's still a floating $.  (Recall, you're in C land here, not shell.)

Re: [PATCH v3] shell: Rewrite documentation and improve error message

[Ramkumar Ramachandra]

Hi Greg,

Greg Brockman writes:
> >  -                          "hint: ~/$COMMAND_DIR should exist "
> >  +                          "hint: ~/" $COMMAND_DIR " should exist "
> There's still a floating $.  (Recall, you're in C land here, not shell.)

Ugh. So sorry about this- I've been suffering from insomnia over the
last few days.

-- 8< --
diff --git a/shell.c b/shell.c
index 6aade3d..b539cdf 100644
--- a/shell.c
+++ b/shell.c
@@ -154,7 +154,7 @@ int main(int argc, char **argv)
 		cd_to_homedir();
 		if (access(COMMAND_DIR, R_OK | X_OK) == -1) {
 			die("Interactive git shell is not enabled.\n"
-			    "hint: ~/" $COMMAND_DIR " should exist "
+			    "hint: ~/" COMMAND_DIR " should exist "
 			    "and have read and execute access.");
 		}
 		run_shell();

Re: [PATCH v3] shell: Rewrite documentation and improve error message

[Greg Brockman]

>> >  -                          "hint: ~/$COMMAND_DIR should exist "
>> >  +                          "hint: ~/" $COMMAND_DIR " should exist "
>> There's still a floating $.  (Recall, you're in C land here, not shell.)
>
> Ugh. So sorry about this- I've been suffering from insomnia over the
> last few days.
No worries :).

> -- 8< --
> diff --git a/shell.c b/shell.c
> index 6aade3d..b539cdf 100644
> --- a/shell.c
> +++ b/shell.c
> @@ -154,7 +154,7 @@ int main(int argc, char **argv)
>                cd_to_homedir();
>                if (access(COMMAND_DIR, R_OK | X_OK) == -1) {
>                        die("Interactive git shell is not enabled.\n"
> -                           "hint: ~/" $COMMAND_DIR " should exist "
> +                           "hint: ~/" COMMAND_DIR " should exist "
>                            "and have read and execute access.");
>                }
>                run_shell();
LGTM.

Reviewed-by: Greg Brockman <gdb@mit.edu>

Re: [PATCH v3] shell: Rewrite documentation and improve error message

[Junio C Hamano]

Greg Brockman <gdb@MIT.EDU> writes:

>> Ugh. So sorry about this- I've been suffering from insomnia over the
>> last few days.
> No worries :).
> ...
>> -- 8< --
> LGTM.
>
> Reviewed-by: Greg Brockman <gdb@mit.edu>

Thanks, both.  Will queue.