Re: [PATCH 1/4] Allow creation of arbitrary git-shell commands
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 20, 2010, 17:01 UTC
- Message-ID
- <7vr5iym6jj.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <1279602972-1264-2-git-send-email-gdb@mit.edu>
Greg Brockman <gdb@MIT.EDU> writes:
Show 13 quoted lines
> 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.
Show 14 quoted lines
> @@ -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.
Show 12 quoted lines
> 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 "="?
Show 16 quoted lines
> + 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 \.