Re: [PATCH 1/3] git-daemon: logging done right

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

"Stephen R. van den Berg" <srb@cuci.nl> writes:

> Make git-daemon a proper syslogging citizen with PID-info.

In general, please try to avoid using the wording like that in the commit
log message, _without_ defining what you think is the proper way and why
you think the current behaviour is improper.

Do you mean "if we use syslog(), we can ask it to add pid information to
the output and we do not have to prepend pid information ourselves"?

For the same reason, "logging done right" is somewhat a suboptimal title
for the patch.  If the title describes concisely the reason why the new
way was chosen by you over the old way, and the readers can judge for
themselves if they agree with your reasoning and if the new way is better.
For example, you could have said "git-daemon: let syslog() to add our pid
to the messages".

Yes, it is _not_ the only change you are making with this patch, and the
example message won't describe what you did fully.  It may be an
indication that you are doing too many things in one patch, unless other
changes are "well, they are not a big deal but while we are at it why not
fix them" kind of changes.

> Simplify the overzealous double buffering in the logroutine.

Is there any overzealous double buffering involved?  I thought it just
does s*printf() twice into the single buf[]?  Are you referring to the
trick of setting stderr to line-buffered output?  It does remove the need
for these two s*printf(), and is an improvement.

I am however not as sure as you seem to be that these two changes make the
difference between "done right" and "done wrong" --- at most I'd say that
these fall into "improve the way the log is done" category.

> Call logerror() instead of error().

Calls to die() are covered by setting die-routine to daemon_die(), but the
error() calls are lost to bitbucket.  This is a real fix to keep otherwise
lost error message to the log stream.

Don't you want to also send the "poll failed" error to the log stream as
well?

Thanks.