RFC patch, 4 partsgit-shell-commands: Add a command to list bare repos

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

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

>> +     /* 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.

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

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

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

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

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

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

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

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

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