threads / patch / 10160

patchgit-shell and git-cvsserver

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

## tl;dr

10 messages between Oct 5, 2007 and Oct 10, 2007. Diffs are folded; open one to read it.

replies: 9people: 4as markdown or json

Jan Wielemaker· Oct 5, 2007, 12:53 UTC · lore
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

Show changes to diff +58 −5
@@ -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;
Miklos Vajna· Oct 5, 2007, 14:31 UTC · re: Jan Wielemaker · lore

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

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
Frank Lichtenheld· Oct 5, 2007, 15:07 UTC · re: Jan Wielemaker · lore

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

On Fri, Oct 05, 2007 at 02:53:47PM +0200, Jan Wielemaker wrote:
Show 30 quoted lines
> +#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/
Johannes Schindelin· Oct 8, 2007, 04:51 UTC · re: Jan Wielemaker · lore

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

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

Jan Wielemaker· Oct 8, 2007, 14:22 UTC · re: Johannes Schindelin · lore

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

Dscho,
On Monday 08 October 2007 06:51, Johannes Schindelin wrote:
Show 20 quoted lines
> 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 :-)
Show 12 quoted lines
> 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 ...
Show 6 quoted lines
> 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
Johannes Schindelin· Oct 9, 2007, 11:51 UTC · re: Jan Wielemaker · lore

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

Hi,
On Mon, 8 Oct 2007, Jan Wielemaker wrote:
Show 7 quoted lines
> 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

Johannes Schindelin· Oct 9, 2007, 14:33 UTC · re: Jan Wielemaker · lore

[PATCH] Support cvs via git-shell

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(-)
Show changes to shell.c +26 −1
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
Frank Lichtenheld· Oct 9, 2007, 22:35 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Support cvs via git-shell

On Tue, Oct 09, 2007 at 03:33:25PM +0100, Johannes Schindelin wrote:
Show 8 quoted lines
>  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/
Johannes Schindelin· Oct 10, 2007, 13:29 UTC · re: Frank Lichtenheld · lore

Re: [PATCH] Support cvs via git-shell

Hi,
On Wed, 10 Oct 2007, Frank Lichtenheld wrote:
Show 11 quoted lines
> 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

Frank Lichtenheld· Oct 10, 2007, 19:10 UTC · re: Johannes Schindelin · lore

Re: [PATCH] Support cvs via git-shell

On Wed, Oct 10, 2007 at 02:29:00PM +0100, Johannes Schindelin wrote:
Show 14 quoted lines
> 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/

← back to recent threads