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

[Junio C Hamano <gitster@pobox.com>]

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?

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

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

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

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