Re: What's cooking in git.git (Aug 2008, #07; Sat, 23)

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

Junio C Hamano <gitster@pobox.com> writes:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>>> * sb/daemon (Thu Aug 14 20:02:20 2008 +0200) 4 commits
>>>  - git-daemon: rewrite kindergarden, new option --max-connections
>>>  - git-daemon: Simplify dead-children reaping logic
>>>  - git-daemon: use LOG_PID, simplify logging code
>>>  - git-daemon: call logerror() instead of error()
>>> 
>>> Can somebody who actually runs the daemon standalone comment on this 
>>> one?
>>
>> I am somewhat uneasy about running my production machine with these 
>> changes, since the last commit (the one introducing a kindergarden with 
>> cradles) is too unobvious for me.
>
> Well, I didn't ask anybody to _run_ it.
>
> I asked people who care about their daemons to comment on the change, so
> that if there are any issues in the code that I didn't see, the breakage
> gets caught before it propagates to their daemons they build from 'next'
> or 'master'.
>
> Having said that, I do agree that the kindergarden change is very involved
> (it removes more code than it adds --- the reduction of lines is somewhat
> inflated because quite a lot of comments that have become stale gets
> removed by the patch).

Ok, so I looked at the patch again (sorry, Stephen, for keeping this in a
limbo for very long).

First of all, I have to say that I think this patch is reasonably well
done.  I do see an unnecessary check of xcalloc() return value and some
style differences from what git uses, but that is only an indication of
the author not very familiar with git code, and every other aspect of the
change shows that this is quite competently done.  The list traversal with
cradle/blanket/newborn, except using perhaps too cute names for some
people's tastes, makes sense.

I however do have one issue with the logic, though.

It seems that kill_some_child() will not kill anything if nobody else is
coming from the same address, while the old code did kill some.  Is this
intended?

By the way, add_child() compares the whole "struct sockaddr_storage" in
order to queue the newborn in front of an existing connection from the
same address, and kill_some_child() takes advanrage of this to kill the
newest connection ("We kill the newest" comment should probably be moved
to add_child() to describe why the queuing is done this way).  If you
simplify add_child() to queue the newborn always at the front of the list,
your kill_some_child() will continue to do so, so I do not see the point
of the loop in add_child().  Am I mistaken?