Re: Patch which adds syslog support to git-shell

[Erik Faye-Lund <kusmabite@googlemail.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 :)

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

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