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

From: Jan Wielemaker <wielemak@science.uva.nl>
Date: Oct 8, 2007, 14:22 UTC
Message-ID: <200710081622.33222.wielemak@science.uva.nl>
In-Reply-To: <Pine.LNX.4.64.0710080534270.4174@racer.site>

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