Re: [PATCH 1/3] git-daemon: single-line logs

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 14, 2009, 11:33 UTC
Message-ID: <7vy6xe2kbx.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <alpine.LSU.2.00.0901141147120.16109@fbirervta.pbzchgretzou.qr>

Jan Engelhardt <jengelh@medozas.de> writes:

> parent v1.6.1
>
> git-daemon: single-line logs

Please drop these two needless lines when/if you are submitting patches for inclusion..

> Having just a single line per connection attempt, much like Apache
> httpd2 access logs, makes log parsing much easier, especially when
> just glancing over it non-automated.

While I like the motivation, and I wish the log were as terse as possible from the day one, I think changing the output format unconditionally like this patch does is a horrible idea. I'd expect there are many people who already have their infrastructure set up to parse the current output; this patch actively breaks things for them, doesn't it?

Show 13 quoted lines

> @@ -295,12 +295,13 @@ static int git_daemon_config(const char
>  	return 0;
>  }
>  
> -static int run_service(char *dir, struct daemon_service *service)
> +static int run_service(char *dir, struct daemon_service *service,
> +    const char *origin, const char *vhost)
>  {
>  	const char *path;
>  	int enabled = service->enabled;
>  
> -	loginfo("Request %s for '%s'", service->name, dir);
> +	loginfo("%s->%s %s \"%s\"\n", origin, vhost, service->name, dir);

Mental note. You are adding origin and vhost probably because you are losing them from elsewhere..

Show 17 quoted lines

> @@ -507,10 +508,10 @@ static void parse_extra_args(char *extra
>  static int execute(struct sockaddr *addr)
>  {
>  	static char line[1000];
> +	char addrbuf[256] = "";
>  	int pktlen, len, i;
>  
>  	if (addr) {
> -		char addrbuf[256] = "";
>  		int port = -1;
>  
>  		if (addr->sa_family == AF_INET) {
> @@ -529,7 +530,6 @@ static int execute(struct sockaddr *addr
>  			port = ntohs(sin6_addr->sin6_port);
>  #endif
>  		}
> -		loginfo("Connection from %s:%d", addrbuf, port);

Mental note.  Port is not logged anymore here.

Show 8 quoted lines

> @@ -541,10 +541,6 @@ static int execute(struct sockaddr *addr
>  	alarm(0);
>  
>  	len = strlen(line);
> -	if (pktlen != len)
> -		loginfo("Extended attributes (%d bytes) exist <%.*s>",
> -			(int) pktlen - len,
> -			(int) pktlen - len, line + len + 1);

Mental note.  XA are not logged here anymore.

Show 9 quoted lines

> @@ -569,7 +565,8 @@ static int execute(struct sockaddr *addr
>  			 * Note: The directory here is probably context sensitive,
>  			 * and might depend on the actual service being performed.
>  			 */
> -			return run_service(line + namelen + 5, s);
> +			return run_service(line + namelen + 5, s,
> +			       addrbuf, hostname);
>  		}
>  	}

So not just you are changing the format, but you are losing information as well.

By the way, I think hostname has already been freed and NULLed at this call site. Aren't you getting entries like:

	192.168.0.1->(null) upload-pack "/pub/git.git"

in your log?