Re: [PATCH] git-shell and git-cvsserver

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Oct 8, 2007, 04:51 UTC
Message-ID: <Pine.LNX.4.64.0710080534270.4174@racer.site>
In-Reply-To: <200710051453.47622.wielemak@science.uva.nl>

Hi,

On Fri, 5 Oct 2007, Jan Wielemaker wrote:

Show 17 quoted lines

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

Show 12 quoted lines

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

Show 9 quoted lines

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

Show 33 quoted lines

>  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