Re: [PATCH] http: add_fill_function checks if function has been added

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 7, 2009, 20:18 UTC
Message-ID: <7vy6vhm6it.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <49B266B0.4020304@gmail.com>

Tay Ray Chuan <rctay89@gmail.com> writes:

Show 10 quoted lines

> This patch ensures that the same fill function is called once so to
> prevent any possible issues.
>
> Nevertheless, calling a fill function repeatedly in
> ''fill_active_slots'' will not lead to any obvious change in existing
> behavior, though performance might be affected.
>
> ''add_fill_action'' checks if the function to be added has already been
> added. Allocation of memory for the list ''fill_chain*'' is postponed
> until the check passes, unlike previously.

Could you care to explain the following a bit better?

 - what "possible issues" you are addressing;

 - what changes in the behaviour that are not "obvious" we would be
   suffering from, if we apply this patch;

 - in what situation the performance _might_ be affected, in what way and
   to what extent.

If the patch author does not have clear answers to these questions, how can others decide if it is worth reading the patch to judge if it is worth applying?

In other words, I'd expect you to explain the issues like this:

    add_fill_function() adds the same fill function twice on the fill_cfg
    list; this causes THIS and THAT breakages when the fill function is
    called twice.

    Ignore add_fill_function() when fill_cfg list already has the function
    registered on it to fix this issue.  Note however that the new code may
    behave very inefficiently under this situation:

    - XYZZY happens, then
    - FROTZ happens, and then
    - NITFOL happens.

    In such a case we end up doing FROTZ repeatedly, and ...; we might
    want to later optimize this, but a correctly working code is more
    important than efficient code that works most of the time but silently
    breaks in some cases.

    We need to iterate over the existing entries in fill_cfg list to find
    duplicates, which may look like an overhead, but the existing function
    already needed to do so to queue the new entry at the end anyway, so
    this is nothing new.