[PATCH] git-shell and git-cvsserver

[PATCH] git-shell and git-cvsserver

[Jan Wielemaker]

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


--- shell.c.org	2007-10-05 13:08:47.000000000 +0200
+++ shell.c	2007-10-05 14:24:11.000000000 +0200
@@ -18,27 +18,80 @@
 	return execv_git_cmd(my_argv);
 }
 
+#define EXEC_PATH "/usr/local/bin"
+
+static int do_cvs_cmd(const char *me, char *arg)
+{
+	const char *my_argv[4];
+	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")) ) {
+		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);
+	}
+
+	return execv_git_cmd(my_argv);
+}
+
+
 static struct commands {
 	const char *name;
 	int (*exec)(const char *me, char *arg);
 } cmd_list[] = {
 	{ "git-receive-pack", do_generic_cmd },
 	{ "git-upload-pack", do_generic_cmd },
+	{ "cvs", do_cvs_cmd },
 	{ NULL },
 };
 
 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;
+	}
 
-	prog = argv[2];
-	argv += 2;
-	argc -= 2;
 	for (cmd = cmd_list ; cmd->name ; cmd++) {
 		int len = strlen(cmd->name);
 		char *arg;

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

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 187 bytes --]

On Fri, Oct 05, 2007 at 02:53:47PM +0200, Jan Wielemaker <wielemak@science.uva.nl> wrote:
> +#define EXEC_PATH "/usr/local/bin"

why don't you use $(prefix) from the Makefile?

- VMiklos

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

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

[Frank Lichtenheld]

On Fri, Oct 05, 2007 at 02:53:47PM +0200, Jan Wielemaker wrote:
> +#define EXEC_PATH "/usr/local/bin"
> +
> +static int do_cvs_cmd(const char *me, char *arg)
> +{
> +	const char *my_argv[4];
> +	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")) ) {
> +		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);
> +	}
> +
> +	return execv_git_cmd(my_argv);
> +}

This seems to be mostly a duplication of prepend_to_path from git.c

Gruesse,
-- 
Frank Lichtenheld <frank@lichtenheld.de>
www: http://www.djpig.de/

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

[Johannes Schindelin]

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

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

[Jan Wielemaker]

Dscho,

On Monday 08 October 2007 06:51, Johannes Schindelin wrote:
> 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...

Great :-)

> 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 buy that.  I'm still trying to get used to all the features ...

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

I stated in my comments I was not happy about that. Before I start
submitting a new patch that may or may not be accepted, I'd like to have
some things clear. I manage an Open Source project for a long time (the
term not even invented in 1985 :-) Users come up with problems and
report on this. Most often with just a statement they don't like it,
sometimes with a detailed description of what is wrong and how to fix
it, sometimes with patches.

Generally, patches are not really how I like them, precisely the kind
of things that are wrong with my patch.  Style issues, fixed A where
I consider the patch must be in B after a conflict between A and B
was detected, missing opportunities for code reuse, etc.

I try to talk frequent contributors into making things as `ready-to-use'
as possible. For incidental contributors I generally don't care. I just
rewrite the patch. Its less work for me than trying to explain all
details (as you kindly did and I agree to most of it, even learn some
new things) and it is too much work for someone who wishes an incidental
patch in the main distribution.

I don't want to become a GIT comitter, and I don't want to learn all of
its internals. Just a happy user for the SWI-Prolog project and an
internal project with some CVS addicts.

	Cheers --- Jan

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

[Johannes Schindelin]

Hi,

On Mon, 8 Oct 2007, Jan Wielemaker wrote:

> On Monday 08 October 2007 06:51, Johannes Schindelin wrote:
> 
> > > +#define EXEC_PATH "/usr/local/bin"
> >
> > This is definitely wrong.  Use git_exec_path() instead.
> 
> I stated in my comments I was not happy about that.

That's why I suggested a fix.

> [explaining -- using a lot of words -- no intention to fix the patch]

Heh.  I think that you could have fixed the patch in less time than it 
took you to write your answer ;-)

Ciao,
Dscho

[PATCH] Support cvs via git-shell

[Johannes Schindelin]

This adds cvs support to the git-shell; You can now give new users
a restricted git-shell and they still can commit via git's cvs
emulator.

Note that either the gecos information must be accurate, or you must
provide a $HOME/.gitconfig with the appropriate user credentials.
Since the git-shell is too restricted to allow the user to do it
(on purpose!), it is up to the administrator to take care of that.

Based on an idea by Jan Wielemaker.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	Since Jan seems to be too busy to do it (in contrast to everybody 
	else), I ended up implementing my advices myself.  At least that 
	way, I can take the credit, too, since not many things are left 
	from the original patch.

 shell.c |   27 ++++++++++++++++++++++++++-
 1 files changed, 26 insertions(+), 1 deletions(-)

diff --git a/shell.c b/shell.c
index c983fc7..cfe372b 100644
--- a/shell.c
+++ b/shell.c
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "quote.h"
 #include "exec_cmd.h"
+#include "strbuf.h"
 
 static int do_generic_cmd(const char *me, char *arg)
 {
@@ -18,12 +19,34 @@ static int do_generic_cmd(const char *me, char *arg)
 	return execv_git_cmd(my_argv);
 }
 
+static int do_cvs_cmd(const char *me, char *arg)
+{
+	const char *cvsserver_argv[3] = {
+		"cvsserver", "server", NULL
+	};
+	const char *oldpath = getenv("PATH");
+	struct strbuf newpath = STRBUF_INIT;
+
+	if (!arg || strcmp(arg, "server"))
+		die("git-cvsserver only handles server: %s", arg);
+
+	strbuf_addstr(&newpath, git_exec_path());
+	strbuf_addch(&newpath, ':');
+	strbuf_addstr(&newpath, oldpath);
+
+	setenv("PATH", strbuf_detach(&newpath, NULL), 1);
+
+	return execv_git_cmd(cvsserver_argv);
+}
+
+
 static struct commands {
 	const char *name;
 	int (*exec)(const char *me, char *arg);
 } cmd_list[] = {
 	{ "git-receive-pack", do_generic_cmd },
 	{ "git-upload-pack", do_generic_cmd },
+	{ "cvs", do_cvs_cmd },
 	{ NULL },
 };
 
@@ -32,8 +55,10 @@ int main(int argc, char **argv)
 	char *prog;
 	struct commands *cmd;
 
+	if (argc == 2 && !strcmp(argv[1], "cvs server"))
+		argv--;
 	/* We want to see "-c cmd args", and nothing else */
-	if (argc != 3 || strcmp(argv[1], "-c"))
+	else if (argc != 3 || strcmp(argv[1], "-c"))
 		die("What do you think I am? A shell?");
 
 	prog = argv[2];
-- 
1.5.3.4.1169.g5fb8d

Re: [PATCH] Support cvs via git-shell

[Frank Lichtenheld]

On Tue, Oct 09, 2007 at 03:33:25PM +0100, Johannes Schindelin wrote:
>  static struct commands {
>  	const char *name;
>  	int (*exec)(const char *me, char *arg);
>  } cmd_list[] = {
>  	{ "git-receive-pack", do_generic_cmd },
>  	{ "git-upload-pack", do_generic_cmd },
> +	{ "cvs", do_cvs_cmd },
>  	{ NULL },

Maybe this should also allow git-cvsserver for completeness' sake?

Gruesse,
-- 
Frank Lichtenheld <frank@lichtenheld.de>
www: http://www.djpig.de/

Re: [PATCH] Support cvs via git-shell

[Johannes Schindelin]

Hi,

On Wed, 10 Oct 2007, Frank Lichtenheld wrote:

> On Tue, Oct 09, 2007 at 03:33:25PM +0100, Johannes Schindelin wrote:
> >  static struct commands {
> >  	const char *name;
> >  	int (*exec)(const char *me, char *arg);
> >  } cmd_list[] = {
> >  	{ "git-receive-pack", do_generic_cmd },
> >  	{ "git-upload-pack", do_generic_cmd },
> > +	{ "cvs", do_cvs_cmd },
> >  	{ NULL },
> 
> Maybe this should also allow git-cvsserver for completeness' sake?

Umm.  Why?

If you use a cvs client with :ext: protocol, it will call ssh (or rsh!) 
with the command "cvs server", not "git-cvsserver".

Only in setups where you set (IIRC) the environment variable "CVSSERVER" 
on the client to "git-cvsserver" will it call something different, but 
these setups exist already, and never used git-shell to begin with.

Ciao,
Dscho

Re: [PATCH] Support cvs via git-shell

[Frank Lichtenheld]

On Wed, Oct 10, 2007 at 02:29:00PM +0100, Johannes Schindelin wrote:
> On Wed, 10 Oct 2007, Frank Lichtenheld wrote:
> > On Tue, Oct 09, 2007 at 03:33:25PM +0100, Johannes Schindelin wrote:
> > >  static struct commands {
> > >  	const char *name;
> > >  	int (*exec)(const char *me, char *arg);
> > >  } cmd_list[] = {
> > >  	{ "git-receive-pack", do_generic_cmd },
> > >  	{ "git-upload-pack", do_generic_cmd },
> > > +	{ "cvs", do_cvs_cmd },
> > >  	{ NULL },
> > 
> > Maybe this should also allow git-cvsserver for completeness' sake?
> 
> Umm.  Why?

Symmetry? ;)
The "for completeness' sake" was as good a reason as I could think of.

Gruesse,
-- 
Frank Lichtenheld <frank@lichtenheld.de>
www: http://www.djpig.de/