[PATCH/RFC 0/4] Providing mechanism to list available repositories

I'm working on a project that has separate Git repositories for
different components.  Repositories are cloneable via
  git clone git@xvm.mit.edu:path/to/repo.git,
where the 'git' user's shell is git-shell.

We have been seeking a simple and maintainable way for users to
discover the set of available repositories.  E.g. posting the list on
our website would add extra steps for users to find and retrieve the
list as well as require extra effort from our end.  Since we already give
users ssh access to git@xvm.mit.edu, we would like to multiplex the
functionality to allow discovery of available repositories.

Our solution is to expose a 'list' command to the end user, invocable
as
  ssh git@xvm.mit.edu list,
which displays the available repositories.

We find this mechanism useful in that it requires no extra
infrastructure on either our end or the user's end.  Our
implementation is extensible, allowing the system administrator to
place arbitrary commands in ~/git-shell-commands (if the directory is
omitted, no extra functionality is exposed), and also supports an
interactive mode.

What do people think of this approach?  I'd love to get this
functionality merged in some form.

Thank you!

Greg Brockman

Signed-off-by: Greg Brockman <redacted>
---
 git-shell-commands/list |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)
 create mode 100755 git-shell-commands/list

diff --git a/git-shell-commands/list b/git-shell-commands/list
new file mode 100755
index 0000000..dca2472
--- /dev/null
+++ b/git-shell-commands/list

@@ -0,0 +1,9 @@
+#!/bin/bash
+
+cd "..";
+# TODO: make safe for spaces
+for dir in $(find -type d -name '*.git'); do
+    if [ "$(git --git-dir="$dir" rev-parse --is-bare-repository)" = "true" ]; then
+	echo "${dir#./}"
+    fi
+done

-- 
1.7.0.4

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 <redacted>
---
 shell.c |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/shell.c b/shell.c
index e4864e0..3fee0ed 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,12 @@ 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 struct commands {
 	const char *name;

@@ -99,5 +107,13 @@ int main(int argc, char **argv)
 		}
 		exit(cmd->exec(cmd->name, arg));
 	}
+
+	/* Shell should be spawned with cwd in the git user's home directory */
+	if (chdir(COMMAND_DIR))
+		die("unrecognized command '%s'", prog);
+
+	if (is_valid_cmd_name(prog))
+		execl(prog, prog, (char *) NULL);
+
 	die("unrecognized command '%s'", prog);
 }

-- 
1.7.0.4

Signed-off-by: Greg Brockman <redacted>
---
 git-shell-commands/help |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)
 create mode 100755 git-shell-commands/help

diff --git a/git-shell-commands/help b/git-shell-commands/help
new file mode 100755
index 0000000..a6b1a68
--- /dev/null
+++ b/git-shell-commands/help

@@ -0,0 +1,7 @@
+#!/bin/sh
+
+echo "Commands you may want to run by hand:"
+ls
+if tty -s; then
+    echo "You can leave by running 'exit'"
+fi

-- 
1.7.0.4

Signed-off-by: Greg Brockman <redacted>
---
 shell.c |   50 ++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/shell.c b/shell.c
index 3fee0ed..9f80226 100644
--- a/shell.c
+++ b/shell.c

@@ -1,8 +1,11 @@
+#include <stdio.h>
+
 #include "cache.h"
 #include "quote.h"
 #include "exec_cmd.h"
 #include "strbuf.h"
 
+#define MAX_LINE_LEN 128
 #define COMMAND_DIR "git-shell-commands"
 
 static int do_generic_cmd(const char *me, char *arg)

@@ -41,6 +44,26 @@ static int is_valid_cmd_name(const char *cmd)
 	return cmd[strcspn(cmd, "./")] == '\0';
 }
 
+static int run(const char *prog)
+{
+	pid_t pid, res;
+	int w;
+	pid = fork();
+	if (pid == -1) {
+		perror("fork");
+		exit(-1);
+	} else if ( pid == 0 ) {
+		execl(prog, prog, (char *) NULL);
+		if (prog[0] != '\0')
+			fprintf(stderr, "unrecognized command '%s'\n", prog);
+		exit(127);
+	} else {
+		do {
+			res = waitpid (pid, &w, 0);
+		} while (res == -1 && errno == EINTR);
+	}
+}
+
 
 static struct commands {
 	const char *name;

@@ -56,6 +79,7 @@ static struct commands {
 int main(int argc, char **argv)
 {
 	char *prog;
+	char line[MAX_LINE_LEN];
 	struct commands *cmd;
 	int devnull_fd;
 

@@ -81,8 +105,30 @@ int main(int argc, char **argv)
 	 * We do not accept anything but "-c" followed by "cmd arg",
 	 * where "cmd" is a very limited subset of git commands.
 	 */
-	else if (argc != 3 || strcmp(argv[1], "-c"))
-		die("What do you think I am? A shell?");
+	else if (argc != 3 || strcmp(argv[1], "-c")) {
+		if (chdir(COMMAND_DIR))
+			die("Sorry, the interactive git-shell is not enabled");
+		for (;;) {
+			printf("git> ");
+			if (fgets(line, MAX_LINE_LEN, stdin) == NULL) {
+				printf("\n");
+				exit(0);
+			}
+
+			if (line[strlen(line) - 1] == '\n')
+				line[strlen(line) - 1] = '\0';
+
+			if (!strcmp(line, "quit") || !strcmp(line, "logout") ||
+				   !strcmp(line, "exit")) {
+				exit(0);
+			} else if (!strcmp(line, "")) {
+			} else if (is_valid_cmd_name(line)) {
+				run(line);
+			} else {
+				fprintf(stderr, "invalid command format '%s'\n", line);
+			}
+		};
+	}
 
 	prog = argv[2];
 	if (!strncmp(prog, "git", 3) && isspace(prog[3]))

-- 
1.7.0.4

On Wed, Jul 14, 2010 at 03:01, Greg Brockman [off-list ref] wrote:

+               execl(prog, prog, (char *) NULL);

Why the casting of NULL? It's not done in the builtin/help.c code.

Anyway, if it was cast it should be to (const char *), shouldn't it?

I don't have an immediate need for features implemented by this series,
but I think they can be useful occasionally.

Am 7/14/2010 5:01, schrieb Greg Brockman:

Is it really needed? Doesn't cache.h pull it in already?

+
 #include "cache.h"

...

+static int run(const char *prog)
+{
+	pid_t pid, res;
+	int w;
+	pid = fork();
+	if (pid == -1) {
+		perror("fork");
+		exit(-1);
+	} else if ( pid == 0 ) {
+		execl(prog, prog, (char *) NULL);
+		if (prog[0] != '\0')
+			fprintf(stderr, "unrecognized command '%s'\n", prog);
+		exit(127);
+	} else {
+		do {
+			res = waitpid (pid, &w, 0);
+		} while (res == -1 && errno == EINTR);
+	}
+}

Is there a reason that you duplicate functionality offered by run_command()?

I can imagine that this loop grows in the future, so I suggest to move it
to a separate function right from the beginning.

I think it would make sense to print a help message before the first prompt.

-- Hannes

On 07/14/2010 04:04 AM, Ævar Arnfjörð Bjarmason wrote:

On Wed, Jul 14, 2010 at 03:01, Greg Brockman [off-list ref] wrote:

quoted

+               execl(prog, prog, (char *) NULL);

Why the casting of NULL? It's not done in the builtin/help.c code.

Anyway, if it was cast it should be to (const char *), shouldn't it?

When a NULL sentinel is passed to a varargs function that only
understands 'char *' arguments, the NULL must be cast specifically,
otherwise it will appear in the varargs array as an int or a long.
execl() is an example of a varargs function that only uses varargs
functionality to accept a variable *number* of arguments, it does not
allow for arguments of differing types, so it does not check the types
of its arguments at all. On any platform where an int and a pointer are
not the same size, this can cause a serious problem. When we came across
this problem in Asterisk, we added a macro called SENTINEL (that just
expands to the proper type for the target platform) that is used in
these cases, so that it is clear to the reader of the code what is going on.

-- 
Kevin P. Fleming
Digium, Inc. | Director of Software Technologies
445 Jan Davis Drive NW - Huntsville, AL 35806 - USA
skype: kpfleming | jabber: kfleming@digium.com
Check us out at www.digium.com & www.asterisk.org

* Kevin P. Fleming [off-list ref] [100714 15:59]:

On 07/14/2010 04:04 AM, Ævar Arnfjörð Bjarmason wrote:

quoted

On Wed, Jul 14, 2010 at 03:01, Greg Brockman [off-list ref] wrote:

quoted

+               execl(prog, prog, (char *) NULL);

Why the casting of NULL? It's not done in the builtin/help.c code.

Anyway, if it was cast it should be to (const char *), shouldn't it?

When a NULL sentinel is passed to a varargs function that only
understands 'char *' arguments, the NULL must be cast specifically,
otherwise it will appear in the varargs array as an int or a long.

To be more specific: If NULL is (void *)0 then it does not need to be
cast. Sadly the standard allows to define it as 0, and so it is on
some systems. So to be portable it needs to be cast to be a pointer,
otherwise the varargs argument is assumed to be an int.

	Bernhard R. Link

[Please don't trim the Cc list without good reason.]

Bernhard R. Link wrote:

* Kevin P. Fleming [off-list ref] [100714 15:59]:

quoted

On 07/14/2010 04:04 AM, Ævar Arnfjörð Bjarmason wrote:

quoted

On Wed, Jul 14, 2010 at 03:01, Greg Brockman [off-list ref] wrote:

quoted

+               execl(prog, prog, (char *) NULL);

Why the casting of NULL? It's not done in the builtin/help.c code.

Anyway, if it was cast it should be to (const char *), shouldn't it?

When a NULL sentinel is passed to a varargs function that only
understands 'char *' arguments, the NULL must be cast specifically,
otherwise it will appear in the varargs array as an int or a long.

To be more specific: If NULL is (void *)0 then it does not need to be
cast. Sadly the standard allows to define it as 0, and so it is on
some systems. So to be portable it needs to be cast to be a pointer,
otherwise the varargs argument is assumed to be an int.

Worse, the pointer representations need not be the same between types,
even though that is a fairly exotic idea:

  http://c-faq.com/null/machexamp.html

So it seems execl() must always have an explicitly-cast (char*)NULL
sentinel.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Greg Brockman [off-list ref] writes:

We find this mechanism useful in that it requires no extra
infrastructure on either our end or the user's end.  Our
implementation is extensible, allowing the system administrator to
place arbitrary commands in ~/git-shell-commands (if the directory is
omitted, no extra functionality is exposed), and also supports an
interactive mode.

What do people think of this approach?  I'd love to get this
functionality merged in some form.

It seems to me that any time you need to add a new helper command, the
administrator needs to make sure that appears in ~$user/git-shell-commands
of all the users who need it.  When adding a new user, a similar
management action needs to happen.  Perhaps that is done by making a
symlink from all the users' home directories to one shared place.  Is that
the general idea?

In any case, I'd prefer that the sample command implementations like list
and help to live in contrib/ somewhere.  They are not part of what the
main Makefile needs to know about, right?

quoted

We find this mechanism useful in that it requires no extra
infrastructure on either our end or the user's end.  Our
implementation is extensible, allowing the system administrator to
place arbitrary commands in ~/git-shell-commands (if the directory is
omitted, no extra functionality is exposed), and also supports an
interactive mode.

What do people think of this approach?  I'd love to get this
functionality merged in some form.

It seems to me that any time you need to add a new helper command, the
administrator needs to make sure that appears in ~$user/git-shell-commands
of all the users who need it.  When adding a new user, a similar
management action needs to happen.  Perhaps that is done by making a
symlink from all the users' home directories to one shared place.  Is that
the general idea?

That's correct.  Our particular environment only has a single git
user, but if we were to add more we would probably make
git-shell-commands a symlink as you suggest.

In any case, I'd prefer that the sample command implementations like list
and help to live in contrib/ somewhere.  They are not part of what the
main Makefile needs to know about, right?

Also correct.  I'll look for a reasonable place within contrib/ to put them.

The NULL sentinel argument to the execl*() family of calls must be
cast to (char *), as otherwise:

- platforms where NULL is just 0 (not (void *)) would pass an int

- (admittedly esoteric) platforms where NULL is (void *)0 and (void *)
  and (char *) have different memory layouts would pass the wrong kind
  of pointer

Signed-off-by: Thomas Rast <redacted>
---

Let's not forget about this.

 builtin/help.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/builtin/help.c b/builtin/help.c
index a9836b0..61ff798 100644
--- a/builtin/help.c
+++ b/builtin/help.c

@@ -120,7 +120,7 @@ static void exec_woman_emacs(const char *path, const char *page)
 		if (!path)
 			path = "emacsclient";
 		strbuf_addf(&man_page, "(woman \"%s\")", page);
-		execlp(path, "emacsclient", "-e", man_page.buf, NULL);
+		execlp(path, "emacsclient", "-e", man_page.buf, (char *)NULL);
 		warning("failed to exec '%s': %s", path, strerror(errno));
 	}
 }

@@ -148,7 +148,7 @@ static void exec_man_konqueror(const char *path, const char *page)
 		} else
 			path = "kfmclient";
 		strbuf_addf(&man_page, "man:%s(1)", page);
-		execlp(path, filename, "newTab", man_page.buf, NULL);
+		execlp(path, filename, "newTab", man_page.buf, (char *)NULL);
 		warning("failed to exec '%s': %s", path, strerror(errno));
 	}
 }

@@ -157,7 +157,7 @@ static void exec_man_man(const char *path, const char *page)
 {
 	if (!path)
 		path = "man";
-	execlp(path, "man", page, NULL);
+	execlp(path, "man", page, (char *)NULL);
 	warning("failed to exec '%s': %s", path, strerror(errno));
 }
 

@@ -165,7 +165,7 @@ static void exec_man_cmd(const char *cmd, const char *page)
 {
 	struct strbuf shell_cmd = STRBUF_INIT;
 	strbuf_addf(&shell_cmd, "%s %s", cmd, page);
-	execl("/bin/sh", "sh", "-c", shell_cmd.buf, NULL);
+	execl("/bin/sh", "sh", "-c", shell_cmd.buf, (char *)NULL);
 	warning("failed to exec '%s': %s", cmd, strerror(errno));
 }
 

@@ -372,7 +372,7 @@ static void show_info_page(const char *git_cmd)
 {
 	const char *page = cmd_to_page(git_cmd);
 	setenv("INFOPATH", system_path(GIT_INFO_PATH), 1);
-	execlp("info", "info", "gitman", page, NULL);
+	execlp("info", "info", "gitman", page, (char *)NULL);
 	die("no info viewer handled the request");
 }
 

@@ -398,7 +398,7 @@ static void get_html_page_path(struct strbuf *page_path, const char *page)
 #ifndef open_html
 static void open_html(const char *path)
 {
-	execl_git_cmd("web--browse", "-c", "help.browser", path, NULL);
+	execl_git_cmd("web--browse", "-c", "help.browser", path, (char *)NULL);
 }
 #endif
 

-- 
1.7.2.278.g76edd.dirty

On Sat, Jul 24, 2010 at 15:20, Thomas Rast [off-list ref] wrote:

The NULL sentinel argument to the execl*() family of calls must be
cast to (char *), as otherwise:

- platforms where NULL is just 0 (not (void *)) would pass an int

- (admittedly esoteric) platforms where NULL is (void *)0 and (void *)
 and (char *) have different memory layouts would pass the wrong kind
 of pointer

Signed-off-by: Thomas Rast <redacted>

Nice that you got around to this after I inadvertently pointed it out
in another thread.

Acked.