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

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

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

> In the current use instances in http-push and http-walker,
> run_active_slot and finish_all_active_slots (which calls
> run_active_slot) will always be called, because they are the functions
> that actually start the http requests[1], and that means the fill
> functions will be called repeated, because
>
> run_active_slot, in its while loop, repeatedly calls
>   step_active_slots calls
>     fill_active_slots calls
>       our fill functions.

That is not what I was worried about.  There are two callsites to
add_fill_function().

http-push.c:2439:		add_fill_function(NULL, fill_active_slot);
http-walker.c:936:	add_fill_function(walker, (int (*)(void *)) fill_active_slot);

The caller in http-push.c always passes NULL as the callback data for the
fill_active_slot callback, which means that the callback function, as long
as it does not depend on the state other than the callback function
parameter (which is always NULL), will do the same thing whether you queue
it only once or as many times as add_fill_function() was called, which the
improvement your patch brings in.

But it is not immediately obvious in http-walker.c callchain if multiple
calls to add_fill_function(), if they were ever made, give the same
"walker" as the callback data.  Your patch only looks at the function but
not the callback data for its filtering.  Doesn't it mean that if a caller
made two calls to add_fill_function() with walker#1 and then walker#2 as
the callback data, you would discard the request for walker#2 and call the
callback function fill_active_slot() with walker#1 inside run_active_slot
repeatedly, without ever calling it for walker#2?

I do not know if that actually happens in the current http-walker.c
codepath, or the caller _happens to_ call the add_fill_function() only
once and making my worry a non-issue, but as a patch submitter, you would
know the answer to the question.

If it is the former, then your change is introducing a bug. If it is the
latter, your change won't break anything immediately, but still introduces
a potential bug waiting to happen, as the API looks as if you can call
add_fill_function() to ask for callback functions to be called with
different callback data and the caller can rely on both of them to be
called at appropriate times, but in reality that guarantee does not hold.