Re: Patch which adds syslog support to git-shell

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
[off-list ref] 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