Hi,
On Fri, 5 Oct 2007, Jan Wielemaker wrote:
> Hi,
>
> I know, I shouldn't be using git-cvsserver :-( Anyway, I patched
> git-shell to start git-cvsserver if it is started interactively and the
> one and only line given to it is "cvs server".
>
> The patch to shell.c is below. The trick with the EXEC_PATH is needed
> because git-cvsserver doesn't appear to be working if you do not include
> the git bindir in $PATH. I think that should be fixed in git-cvsserver
> and otherwise we should at least make the value come from the prefix
> make variable. With this patch I was able to use both Unix and Windows
> cvs clients using git-shell as login shell.
>
> Note that you must provide ~/.gitconfig with user and email in the
> restricted environment.
>
> Enjoy --- Jan
I think this is a valuable contribution. That's why I comment...
Please put a useful commit message (less like an email, more like
something you want to read in git-log) at the beginning of the email, then
a line containing _just_ "---", and after that some comments that are not
meant to be stored in the history, like (I know this does not belong
to...)
After that, there should be a diffstat, and then the patch.
The easiest to have this layout is to do a proper commit in git, use "git
format-patch" to produce the patch, and then insert what you want to say
in addition to the commit message between the "---" marker and the
diffstat.
I strongly disagree (as you yourself, probably) with the notion that this
does not belong into git-shell.
> +#define EXEC_PATH "/usr/local/bin"
This is definitely wrong. Use git_exec_path() instead.
> +static int do_cvs_cmd(const char *me, char *arg)
> +{
> + const char *my_argv[4];
Maybe rename this to cvsserver_args?
> + const char *oldpath;
> +
> + if ( !arg )
> + die("no argument");
> + if ( strcmp(arg, "server") )
> + die("only allows git-cvsserver server: %s", arg);
> +
> + my_argv[0] = "cvsserver";
> + my_argv[1] = "server";
> + my_argv[2] = NULL;
> +
> + if ( (oldpath=getenv("PATH")) ) {
Please lose the spaces after the opening and before the closing brackets.
And put spaces around the "=" sign.
It is really distracting to read different styles of code in the same
project, and that's why we're pretty anal about coding styles. Just have
a look (in the same file) how we write things, and imitate it as closely
as possible.
> + char *newpath = malloc(strlen(oldpath)+strlen(EXEC_PATH)+5+1+1); > +
> + sprintf(newpath, "PATH=%s:%s", EXEC_PATH, oldpath);
> + putenv(newpath);
> + } else {
> + char *newpath = malloc(strlen(EXEC_PATH)+5+1);
> +
> + sprintf(newpath, "PATH=%s", EXEC_PATH);
> + putenv(newpath);
> + }
You have redundant "putenv(newpath);" in both clauses. AFAICT putenv() is
deprecated, too, and we use setenv() elsewhere.
In addition, I strongly suggest using strbuf:
struct strbuf newpath = STRBUF_INIT;
strbuf_addstr(&newpath, git_exec_path());
if ((oldpath = getenv("PATH"))) {
strbuf_addch(&newpath, ':');
strbuf_addstr(&newpath, oldpath);
}
setenv("PATH", strbuf_detach(&newpath, NULL), 1);
> + return execv_git_cmd(my_argv);
... and then you call execv_git_cmd(), which already does all the details
of setting up the exec dir correctly AFAIR.
> int main(int argc, char **argv)
> {
> char *prog;
> + char buf[256];
> struct commands *cmd;
>
> /* We want to see "-c cmd args", and nothing else */
> - if (argc != 3 || strcmp(argv[1], "-c"))
> - die("What do you think I am? A shell?");
> + if (argc == 1) {
> + if (fgets(buf, sizeof(buf)-1, stdin)) {
> + char *end;
> +
> + if ( (end=strchr(buf, '\n')) )
> + { while(end>buf && end[-1] <= ' ')
> + end--;
> + *end = '\0';
> + } else {
> + die("Bad command");
> + }
> +
> + prog = buf;
> + } else {
> + die("No command");
> + }
> + } else {
> + if (argc != 3 || strcmp(argv[1], "-c"))
> + die("What do you think I am? A shell?");
> +
> + prog = argv[2];
> + argv += 2;
> + argc -= 2;
> + }
And this is ugly. If you want to support "cvs server", then just check
for that string, and if it matches, return execl_git_cmd("cvsserver");
Otherwise proceed as in the original code.
Ciao,
Dscho
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
