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

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

[Greg Brockman]

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

[PATCH/RFC 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 |   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

Re: [PATCH/RFC 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

Please have a blank line above and below sample command display like these
for readability.

> Signed-off-by: Greg Brockman <gdb@mit.edu>
> ---
>  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);

Hmm, could you justify "should be" above please?

An example would be "All of the custom commands I wrote to give added
features to users at my installation wanted to be in that directory, not
at the user's home directory, as they mostly operated on files in that
directory", but please do not make me (or other reviewers) guess why.

What I am getting at is that it may be more natural and useful to run
these custom commands in the user's $HOME directory---you would need to
make sure that execl() finds the command you get from the request, perhaps
by prefixing COMMAND_DIR / to the command name, though.

> +	if (is_valid_cmd_name(prog))
> +		execl(prog, prog, (char *) NULL);
> +
>  	die("unrecognized command '%s'", prog);
>  }

Re: [PATCH/RFC 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
>
> Please have a blank line above and below sample command display like these
> for readability.
Sounds good.

>> +     /* Shell should be spawned with cwd in the git user's home directory */
>> +     if (chdir(COMMAND_DIR))
>> +             die("unrecognized command '%s'", prog);
>
> Hmm, could you justify "should be" above please?
>
> An example would be "All of the custom commands I wrote to give added
> features to users at my installation wanted to be in that directory, not
> at the user's home directory, as they mostly operated on files in that
> directory", but please do not make me (or other reviewers) guess why.
>
> What I am getting at is that it may be more natural and useful to run
> these custom commands in the user's $HOME directory---you would need to
> make sure that execl() finds the command you get from the request, perhaps
> by prefixing COMMAND_DIR / to the command name, though.
Err, good point.  The commands I wrote end up running 'cd ..' anyway
:).  Instead just running these commands in the user's $HOME does make
a lot more sense.

Thanks everyone for the comments thus far.

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

[Greg Brockman]

Signed-off-by: Greg Brockman <gdb@mit.edu>
---
 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

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

[Greg Brockman]

Signed-off-by: Greg Brockman <gdb@mit.edu>
---
 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

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

[Greg Brockman]

Signed-off-by: Greg Brockman <gdb@mit.edu>
---
 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

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

[Ævar Arnfjörð Bjarmason]

On Wed, Jul 14, 2010 at 03:01, Greg Brockman <gdb@mit.edu> 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?

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

[Kevin P. Fleming]

On 07/14/2010 04:04 AM, Ævar Arnfjörð Bjarmason wrote:
> On Wed, Jul 14, 2010 at 03:01, Greg Brockman <gdb@mit.edu> 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?

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

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

[Bernhard R. Link]

* Kevin P. Fleming <kpfleming@digium.com> [100714 15:59]:
> On 07/14/2010 04:04 AM, Ævar Arnfjörð Bjarmason wrote:
> > On Wed, Jul 14, 2010 at 03:01, Greg Brockman <gdb@mit.edu> 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?
>
> 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

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

[Thomas Rast]

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

Bernhard R. Link wrote:
> * Kevin P. Fleming <kpfleming@digium.com> [100714 15:59]:
> > On 07/14/2010 04:04 AM, Ævar Arnfjörð Bjarmason wrote:
> > > On Wed, Jul 14, 2010 at 03:01, Greg Brockman <gdb@mit.edu> 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?
> >
> > 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

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

[Greg Brockman]

It looks like git@ got dropped from the CC at some point, but I had
written a few days ago:

>> Is there a reason that you duplicate functionality offered by run_command()?
> No--I hadn't realized that existed.  I'll switch over to that in V2 of
> this patch series.

Today, I inspected run_command() in more detail.  Unfortunately, I'm
not sure of the best way to use it in this situation.  In particular,
run_command() uses execvp, meaning that PATH is invoked.  However, the
user should only be able to run commands in the git-shell-commands
directory.   I do have a few ideas for approaches here; maybe others
see more?  Anyway:
- Set PATH to just $HOME/git-shell-commands.  But then the helper
scripts have to restore PATH to a sane value themselves, and it's not
really clear to me what that value should be.
- Under the hood, exec a different script, which processes the user's
command on its own.  (So if the user types 'help' at the git shell
prompt, actually exec 'git-shell-wrapper help'.)  The
git-shell-wrapper could be a dumb wrapper that just execs
$HOME/git-shell-commands/help or similar.
- Extend run_command to optionally use execv.  Would any other code
actually want this functionality though?  If not, it's probably an
excessively large code change for little benefit.
- Continue using the one-off run() method that I wrote here.

Do people have opinions on the most elegant way to handle this?

Thanks!

Greg

> On Wed, Jul 14, 2010 at 12:07 PM, Bernhard R. Link <brlink@debian.org> wrote:
>> * Thomas Rast <trast@student.ethz.ch> [100714 17:41]:
>>> [Please don't trim the Cc list without good reason.]
>>
>> The mail I answered to had only git@vger.kernel.org in CC and some
>> syntax errors in To.
>>
>>> Bernhard R. Link wrote:
>>> > 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.
>>
>> There is a difference between ugly operating systems where everything
>> else works and you need to cast it and things too exotic to have any
>> chance to get the rest of the code to work without big changes.
>>
>> Machines where you do not get a NULL pointer by a memset(,0,), calloc
>> or the like will have bigger problems anyway. (have not looked at git,
>> but I'd be suprised if at every place there is an explicit assignment
>> for the pointers).
>> Note that in the other examples, char * and void * are the same anyway.
>>
>>        Bernhard R. Link
>>
>

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

[Jonathan Nieder]

Hi Greg,

Greg Brockman wrote:

> - Extend run_command to optionally use execv.  Would any other code
> actually want this functionality though?  If not, it's probably an
> excessively large code change for little benefit.

Of the options you presented, this is the best one.  It doesn’t matter
whether any other code would use it; even if you are the only caller,
it is still good because

 - if someone else needs the facility, it will be obvious where
   to find it

 - you can share the existing logic to portably run a command
   (i.e., near free portability to msys).

run_command() already takes an argument for options like
RUN_USING_SHELL; your new facility would fit right in.

But first a more basic question: why not just add “./” to the start of
the command name?

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

[Greg Brockman]

> But first a more basic question: why not just add “./” to the start of
> the command name?
Wow, of course... that's the obvious solution.

Thanks for the comments!

[PATCH] Cast execl*() NULL sentinels to (char *)

[Thomas Rast]

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 <trast@student.ethz.ch>
---

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

Re: [PATCH] Cast execl*() NULL sentinels to (char *)

[Ævar Arnfjörð Bjarmason]

On Sat, Jul 24, 2010 at 15:20, Thomas Rast <trast@student.ethz.ch> 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 <trast@student.ethz.ch>

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

Acked.

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

[Johannes Sixt]

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:
> --- a/shell.c
> +++ b/shell.c
> @@ -1,8 +1,11 @@
> +#include <stdio.h>

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()?

> @@ -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);
> +			}
> +		};
> +	}

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

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

[Junio C Hamano]

Greg Brockman <gdb@MIT.EDU> 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?

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

[Greg Brockman]

>> 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.