Re: Patch which adds syslog support to git-shell

From: Erik Faye-Lund <kusmabite@googlemail.com>
Date: Dec 24, 2009, 11:38 UTC
Message-ID: <40aa078e0912240338g7765d17cyf948650deab2aa01@mail.gmail.com>
In-Reply-To: <200912231832.03541.gerhard.gappmeier@ascolab.com>

Please read Documentation/SubmittingPatches. We prefer inline patches, as they are easier to review.

On Wed, Dec 23, 2009 at 6:32 PM, Gerhard Gappmeier <gerhard.gappmeier@ascolab.com> wrote:

> Hi
>
> I'm not sure if this is the right list, but here is my first GIT patch.
>

It's the right list :)

Show 6 quoted lines

> I had a problem with git-shell and wanted to analyze it.
> Unfortunately it does not contain any trace capabilities.
> So I cloned git and added some basic syslog support.
> After that I recognized that the current git version just works ;-)
> but the syslog functionality is always a nice thing I think.
> So here is the patch.

Looking at your patch, I see there's a lot of white-space changes. Stuff like:

Show 6 quoted lines

> -	const char *cvsserver_argv[3] = {
> -		"cvsserver", "server", NULL
> -	};
> +    const char *cvsserver_argv[3] = {
> +        "cvsserver", "server", NULL
> +    };

just makes this harder to review. Besides, we use tabs for indentation in git.

Also, I think it would be better to use set_die_routine() from usage.h than to change all the die call-sites. This is what git-daemon does:

--->8---
	if (log_syslog) {
		openlog("git-daemon", LOG_PID, LOG_DAEMON);
		set_die_routine(daemon_die);
	}
--->8---
Look at daemon.c for the implementation of daemon_die().

+/* Syslog defines */ +#define GIT_SYSLOG_IDENT "git-shell" +#define GIT_SYSLOG_OPTION 0 +#define GIT_SYSLOG_FACILITY LOG_LOCAL0 +

Is this really needed? These are only used at one place. Just doing
+    openlog("git-shell", 0, LOG_LOCAL0);
would IMO be cleaner.

Anyway, this is all I bother to point out before I see an inlined, white-space fixed patch.

> Merry X-Mas.

Happy holidays to you too :)

-- 
Erik "kusma" Faye-Lund