Updated patch series for providing mechanism to list available repositories

Updated patch series for providing mechanism to list available repositories

[Greg Brockman]

I have updated the patch series sent to this list on July 13th.  Based
largely on feedback from this list, I have made the following changes:

- sample commands now live in contrib/git-shell-commands
- blank lines have been added to patch 1's commit message for readability
- commands are run with cwd of the user's $HOME directory
- run_command is used rather than writing a new function with the same functionality
- the shell's loop resides in its own function
- commands are now parsed with split_cmdline, so helper functions can be passed arguments

Thanks to those who provided feedback.  I look forward to further
comments!

Greg

[PATCH 1/4] Allow creation of arbitrary git-shell commands

[Greg Brockman]

This provides a mechanism for the server to expose custom
functionality to clients.  My particular use case is that I would like
a way of discovering all repositories available for cloning.  A
client that clones via

  git clone user@example.com

can invoke a command by

  ssh user@example.com $command

Signed-off-by: Greg Brockman <gdb@mit.edu>
---
 shell.c |   41 ++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 40 insertions(+), 1 deletions(-)

diff --git a/shell.c b/shell.c
index e4864e0..fe1fe73 100644
--- a/shell.c
+++ b/shell.c
@@ -3,6 +3,8 @@
 #include "exec_cmd.h"
 #include "strbuf.h"
 
+#define COMMAND_DIR "git-shell-commands"
+
 static int do_generic_cmd(const char *me, char *arg)
 {
 	const char *my_argv[4];
@@ -33,6 +35,20 @@ static int do_cvs_cmd(const char *me, char *arg)
 	return execv_git_cmd(cvsserver_argv);
 }
 
+static int is_valid_cmd_name(const char *cmd)
+{
+	/* Test command contains no . or / characters */
+	return cmd[strcspn(cmd, "./")] == '\0';
+}
+
+static char *make_cmd(const char *prog)
+{
+	char *prefix = xmalloc((strlen(prog) + strlen(COMMAND_DIR) + 2) * sizeof(char));
+	strcpy(prefix, COMMAND_DIR);
+	strcat(prefix, "/");
+	strcat(prefix, prog);
+	return prefix;
+}
 
 static struct commands {
 	const char *name;
@@ -48,6 +64,8 @@ static struct commands {
 int main(int argc, char **argv)
 {
 	char *prog;
+	char *prog_cpy;
+	const char **user_argv;
 	struct commands *cmd;
 	int devnull_fd;
 
@@ -77,6 +95,7 @@ int main(int argc, char **argv)
 		die("What do you think I am? A shell?");
 
 	prog = argv[2];
+	prog_cpy = xstrdup(prog);
 	if (!strncmp(prog, "git", 3) && isspace(prog[3]))
 		/* Accept "git foo" as if the caller said "git-foo". */
 		prog[3] = '-';
@@ -99,5 +118,25 @@ int main(int argc, char **argv)
 		}
 		exit(cmd->exec(cmd->name, arg));
 	}
-	die("unrecognized command '%s'", prog);
+
+	if (split_cmdline(prog, &user_argv) != -1) {
+		if (is_valid_cmd_name(user_argv[0])) {
+			prog  = make_cmd(user_argv[0]);
+			user_argv[0] = prog;
+			execv(user_argv[0], (char *const *) user_argv);
+			free(prog);
+		}
+		free(user_argv);
+		/*
+		 * split_cmdline modifies its argument in-place, so 'prog' now
+		 * holds the actual command name
+		 */
+		die("unrecognized command '%s'", prog_cpy);
+	} else {
+		/*
+		 * split_cmdline has clobbered prog and printed an
+		 * error message, so print the original
+		 */
+		die("invalid command format '%s'", prog_cpy);
+	}
 }
-- 
1.7.0.4

[PATCH 2/4] git-shell-commands: Add a command to list bare repos

[Greg Brockman]

Signed-off-by: Greg Brockman <gdb@mit.edu>
---
 contrib/git-shell-commands/list |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
 create mode 100755 contrib/git-shell-commands/list

diff --git a/contrib/git-shell-commands/list b/contrib/git-shell-commands/list
new file mode 100755
index 0000000..cd8b15a
--- /dev/null
+++ b/contrib/git-shell-commands/list
@@ -0,0 +1,5 @@
+#!/bin/sh
+set -eu
+
+print_if_bare_repo='[ "$(git --git-dir="$1" rev-parse --is-bare-repository)" = true ] && echo "${1#./}"'
+find -type d -name "*.git" -exec sh -c "$print_if_bare_repo" -- \{} \; -prune
-- 
1.7.0.4

[PATCH 3/4] git-shell-commands: Add a help command

[Greg Brockman]

Signed-off-by: Greg Brockman <gdb@mit.edu>
---
 contrib/git-shell-commands/help |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)
 create mode 100755 contrib/git-shell-commands/help

diff --git a/contrib/git-shell-commands/help b/contrib/git-shell-commands/help
new file mode 100755
index 0000000..19d5277
--- /dev/null
+++ b/contrib/git-shell-commands/help
@@ -0,0 +1,18 @@
+#!/bin/sh
+
+set -eu
+
+if tty -s; then
+    echo "Run 'help' for help, or 'exit' to leave.  Available commands:"
+else
+    echo "Run 'help' for help.  Available commands:"
+fi
+
+cd "$(dirname "$0")"
+
+for cmd in *; do
+    case "$cmd" in
+	help) ;;
+	*) [ -f "$cmd" ] && [ -x "$cmd" ] && echo "$cmd" ;;
+    esac
+done
-- 
1.7.0.4

[PATCH 4/4] Add interactive mode to git-shell for user-friendliness

[Greg Brockman]

Signed-off-by: Greg Brockman <gdb@mit.edu>
---
 shell.c |   75 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 69 insertions(+), 6 deletions(-)

diff --git a/shell.c b/shell.c
index fe1fe73..142d201 100644
--- a/shell.c
+++ b/shell.c
@@ -2,8 +2,10 @@
 #include "quote.h"
 #include "exec_cmd.h"
 #include "strbuf.h"
+#include "run-command.h"
 
 #define COMMAND_DIR "git-shell-commands"
+#define HELP_COMMAND COMMAND_DIR "/help"
 
 static int do_generic_cmd(const char *me, char *arg)
 {
@@ -50,6 +52,56 @@ static char *make_cmd(const char *prog)
 	return prefix;
 }
 
+static void run_shell(void)
+{
+	int done = 0;
+	static const char *help_argv[] = { HELP_COMMAND, NULL };
+	/* Print help if enabled */
+	run_command_v_opt(help_argv, RUN_SILENT_EXEC_FAILURE);
+
+	do {
+		struct strbuf line = STRBUF_INIT;
+		const char *prog;
+		char *full_cmd;
+		char *rawargs;
+		const char **argv;
+		int code;
+
+		fprintf(stderr, "git> ");
+		if (strbuf_getline(&line, stdin, '\n') == EOF) {
+			fprintf(stderr, "\n");
+			strbuf_release(&line);
+			break;
+		}
+		strbuf_trim(&line);
+		rawargs = strbuf_detach(&line, NULL);
+		if (split_cmdline(rawargs, &argv) == -1) {
+			free(rawargs);
+			continue;
+		}
+
+		prog = argv[0];
+		if (!strcmp(prog, "")) {
+		} else if (!strcmp(prog, "quit") || !strcmp(prog, "logout") ||
+			   !strcmp(prog, "exit") || !strcmp(prog, "bye")) {
+			done = 1;
+		} else if (is_valid_cmd_name(prog)) {
+			full_cmd = make_cmd(prog);
+			argv[0] = full_cmd;
+			code = run_command_v_opt(argv, RUN_SILENT_EXEC_FAILURE);
+			if (code == -1 && errno == ENOENT) {
+				fprintf(stderr, "unrecognized command '%s'\n", prog);
+			}
+			free(full_cmd);
+		} else {
+			fprintf(stderr, "invalid command format '%s'\n", prog);
+		}
+
+		free(argv);
+		free(rawargs);
+	} while (!done);
+}
+
 static struct commands {
 	const char *name;
 	int (*exec)(const char *me, char *arg);
@@ -84,15 +136,26 @@ int main(int argc, char **argv)
 	/*
 	 * Special hack to pretend to be a CVS server
 	 */
-	if (argc == 2 && !strcmp(argv[1], "cvs server"))
+	if (argc == 2 && !strcmp(argv[1], "cvs server")) {
 		argv--;
-
+	}
 	/*
-	 * We do not accept anything but "-c" followed by "cmd arg",
-	 * where "cmd" is a very limited subset of git commands.
+	 * Allow the user to run an interactive shell
 	 */
-	else if (argc != 3 || strcmp(argv[1], "-c"))
-		die("What do you think I am? A shell?");
+	else if (argc == 1) {
+		if (access(COMMAND_DIR, R_OK | X_OK) == -1)
+			die("Sorry, the interactive git-shell is not enabled");
+		run_shell();
+		exit(0);
+	}
+	/*
+	 * We do not accept any other modes except "-c" followed by
+	 * "cmd arg", where "cmd" is a very limited subset of git
+	 * commands or a command in the COMMAND_DIR
+	 */
+	else if (argc != 3 || strcmp(argv[1], "-c")) {
+		die("Run with no arguments or with -c cmd");
+	}
 
 	prog = argv[2];
 	prog_cpy = xstrdup(prog);
-- 
1.7.0.4

Re: [PATCH 1/4] Allow creation of arbitrary git-shell commands

[Junio C Hamano]

Greg Brockman <gdb@MIT.EDU> writes:

> This provides a mechanism for the server to expose custom
> functionality to clients.  My particular use case is that I would like
> a way of discovering all repositories available for cloning.  A
> client that clones via
>
>   git clone user@example.com
>
> can invoke a command by
>
>   ssh user@example.com $command
>
> Signed-off-by: Greg Brockman <gdb@mit.edu>
> ---

Just a few minor nits...

> +static char *make_cmd(const char *prog)
> +{
> +	char *prefix = xmalloc((strlen(prog) + strlen(COMMAND_DIR) + 2) * sizeof(char));

Since sizeof(char) by defintion is 1, I'd rather not to see the above
obfuscation.

> @@ -48,6 +64,8 @@ static struct commands {
>  int main(int argc, char **argv)
>  {
>  	char *prog;
> +	char *prog_cpy;
> +	const char **user_argv;
>  	struct commands *cmd;
>  	int devnull_fd;
>  
> @@ -77,6 +95,7 @@ int main(int argc, char **argv)
>  		die("What do you think I am? A shell?");
>  
>  	prog = argv[2];
> +	prog_cpy = xstrdup(prog);

Wouldn't changing "prog = argv[2]" to "prog = xstrdup(argv[2])" easier to
read?  Or name this new variable "prog_orig", have one comment here to say
"keep the original for reporting purposes, as prog will be munged", and
drop the two 4-line comment blocks below.

>  	if (!strncmp(prog, "git", 3) && isspace(prog[3]))
>  		/* Accept "git foo" as if the caller said "git-foo". */
>  		prog[3] = '-';
> @@ -99,5 +118,25 @@ int main(int argc, char **argv)
>  		}
>  		exit(cmd->exec(cmd->name, arg));
>  	}
> -	die("unrecognized command '%s'", prog);
> +
> +	if (split_cmdline(prog, &user_argv) != -1) {
> +		if (is_valid_cmd_name(user_argv[0])) {
> +			prog  = make_cmd(user_argv[0]);

Extra SP before "="?

> +			user_argv[0] = prog;
> +			execv(user_argv[0], (char *const *) user_argv);
> +			free(prog);
> +		}
> +		free(user_argv);
> +		/*
> +		 * split_cmdline modifies its argument in-place, so 'prog' now
> +		 * holds the actual command name
> +		 */
> +		die("unrecognized command '%s'", prog_cpy);
> +	} else {
> +		/*
> +		 * split_cmdline has clobbered prog and printed an
> +		 * error message, so print the original
> +		 */
> +		die("invalid command format '%s'", prog_cpy);

Hmm, we might want to fix split_cmdline to report the breakage better
then (perhaps not as part of this patch series).  Instead of seeing

    error: cmdline ends with \.
    fatal: invalid command format 'foo bar \'.

wouldn't it be nicer to see a single error message:

    fatal: invalid command 'foo bar\': cmdline ends with \.

Re: [PATCH 2/4] git-shell-commands: Add a command to list bare repos

[Junio C Hamano]

Greg Brockman <gdb@MIT.EDU> writes:

> Signed-off-by: Greg Brockman <gdb@mit.edu>
> ---
>  contrib/git-shell-commands/list |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
>  create mode 100755 contrib/git-shell-commands/list
>
> diff --git a/contrib/git-shell-commands/list b/contrib/git-shell-commands/list
> new file mode 100755
> index 0000000..cd8b15a
> --- /dev/null
> +++ b/contrib/git-shell-commands/list
> @@ -0,0 +1,5 @@
> +#!/bin/sh
> +set -eu
> +
> +print_if_bare_repo='[ "$(git --git-dir="$1" rev-parse --is-bare-repository)" = true ] && echo "${1#./}"'

That's a very long line you have here.  It might be better to do split the
line perhaps like this for readability:

print_if_bare_repo='
	if "$(git --git-dir="$1" rev-parse --is-bare-repository)" = true
        then
	        printf "%s\n" "${1#./}"
	fi
'

It is unclear why it limits its listing only to bare repositories.  "It's
my design decision" is a perfectly acceptable answer, but no matter what
the reasoning is, it needs to be documented as a part of "How to use this"
insn to the users.  A separate file README in contrib/git-shell-commands
that reads like:

	Any bare repository whose name ends with ".git" under your home
	directory is visible by "list" extended command (no other git
	repositories are visible).

would probably be a good start.

> +find -type d -name "*.git" -exec sh -c "$print_if_bare_repo" -- \{} \; -prune

Also do you need "set -eu" at the beginning?  I don't see it serving a
useful purpose (other than being a development aid, that is).

Re: [PATCH 1/4] Allow creation of arbitrary git-shell commands

[Greg Brockman]

>> +                     user_argv[0] = prog;
>> +                     execv(user_argv[0], (char *const *) user_argv);
>> +                     free(prog);
>> +             }
>> +             free(user_argv);
>> +             /*
>> +              * split_cmdline modifies its argument in-place, so 'prog' now
>> +              * holds the actual command name
>> +              */
>> +             die("unrecognized command '%s'", prog_cpy);
>> +     } else {
>> +             /*
>> +              * split_cmdline has clobbered prog and printed an
>> +              * error message, so print the original
>> +              */
>> +             die("invalid command format '%s'", prog_cpy);
>
> Hmm, we might want to fix split_cmdline to report the breakage better
> then (perhaps not as part of this patch series).  Instead of seeing
>
>    error: cmdline ends with \.
>    fatal: invalid command format 'foo bar \'.
>
> wouldn't it be nicer to see a single error message:
>
>    fatal: invalid command 'foo bar\': cmdline ends with \.
>
That would definitely be preferable.  If possible, I would rather not
do it as part of this patch series, but would be happy to work on
fixing split_cmdline afterwards.

Re: [PATCH 2/4] git-shell-commands: Add a command to list bare repos

[Greg Brockman]

>> @@ -0,0 +1,5 @@
>> +#!/bin/sh
>> +set -eu
>> +
>> +print_if_bare_repo='[ "$(git --git-dir="$1" rev-parse --is-bare-repository)" = true ] && echo "${1#./}"'
>
> That's a very long line you have here.  It might be better to do split the
> line perhaps like this for readability:
>
> print_if_bare_repo='
>        if "$(git --git-dir="$1" rev-parse --is-bare-repository)" = true
>        then
>                printf "%s\n" "${1#./}"
>        fi
> '
Sure, this seems reasonable.  Done in the latest version of my patch
series.  I also added a 2>/dev/null to the 'find' to discard
unnecessary error output.

> It is unclear why it limits its listing only to bare repositories.  "It's
> my design decision" is a perfectly acceptable answer, but no matter what
> the reasoning is, it needs to be documented as a part of "How to use this"
> insn to the users.  A separate file README in contrib/git-shell-commands
> that reads like:
>
>        Any bare repository whose name ends with ".git" under your home
>        directory is visible by "list" extended command (no other git
>        repositories are visible).
>
> would probably be a good start.
Yes, basically "it's my design decision" is the answer here.  I now
include a contrib/git-shell-commands/README, which contains the
following text regarding 'list':
"""
list: Displays any bare repository whose name ends with ".git" under
user's home directory.  No other git repositories are visible,
although they might be clonable through git-shell.  'list' is designed
to minimize the number of calls to git that must be made in finding
available repositories; if your setup has additional repositories that
should be user-discoverable, you may wish to modify 'list'
accordingly.
"""

>> +find -type d -name "*.git" -exec sh -c "$print_if_bare_repo" -- \{} \; -prune
>
> Also do you need "set -eu" at the beginning?  I don't see it serving a
> useful purpose (other than being a development aid, that is).
Yeah, it's was just a development aid.  I've removed the "set -eu" line.