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

From: Jan Engelhardt <jengelh@medozas.de>
Date: Jan 14, 2009, 13:03 UTC
Message-ID: <alpine.LSU.2.00.0901141342060.23212@fbirervta.pbzchgretzou.qr>
In-Reply-To: <7vy6xe2kbx.fsf@gitster.siamese.dyndns.org>

On Wednesday 2009-01-14 12:33, Junio C Hamano wrote:

Show 7 quoted lines

>
>> parent v1.6.1
>>
>> git-daemon: single-line logs
>
>Please drop these two needless lines when/if you are submitting patches
>for inclusion..

The patches are produced by my git-export-patch script (or in this specific case it was quilt); so they usually look like this. Especially when I do make mental notes that do not go into the commit log, the patch is tacked on, as in, for example, http://marc.info/?l=netfilter-devel&m=123191159015731&w=2

Show 6 quoted lines

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

Well, why did no one do it like that then? My guess is that it was not a "real" logging infrastructure but more of a debug aid. Especially when it required --syslog --verbose to pass to git-daemon this seems like --debug=yesPlease.

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

Probably. Which just shows that git-daemon is in need of some configuration .. thing so that each user can choose his own if desired.

Show 13 quoted lines

>> @@ -295,12 +295,13 @@ static int git_daemon_config(const char
>> -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..

Not quite sure what you mean by losing.

But in 1.6.0.x, run_service had a variable interp of type itable or so and it was possible to use interp[SLOT_DIR].val without someone raising a hand declaring I lost them elsewhere ;-)

Show 7 quoted lines

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

Right, I did not see a need for it, and it in itself just stood in the way of getting 1-line-output.

Show 10 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.

I only ever saw the hostname XA, and this is still logged.

Show 19 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?

No. Which means someone succeeded at obfuscating daemon.c. It seems that parse_extra_args() sets hostname again after it has been NULLified just moments ago.