On Wed, Nov 07, 2012 at 07:54:33PM +0530, vijay wrote:
> Thanks stefan for your detailed review.
>
> I will keep in mind all your review comments. I will correct my
> mistakes in future patches.
>
> Attached the updated patch and log message.
This patch looks fine to me overall.
I have one more additional suggestion though.
Your patch uses boolean parameters to notify start end end of an
external item listing. This forces function implementors to reason
about all of these possibilities:
"What do I do if...":
1) notify_external_start == TRUE
notify_external_end == TRUE
2) notify_external_start == TRUE
notify_external_start == FALSE
3) notify_external_end == FALSE
notify_external_end == TRUE
4) notify_external_end == FALSE
notify_external_end == FALSE
All this combined with the possible values for external_parent_url and
external_target gives quite a few possibilities for implementors to reason
about.
I wonder if this interface can be simplified somehow, maybe by getting rid
of these two boolean parameters. Instead, we could tell implementors that
if external_parent_url and external_target are defined, the item being
listed is part of the external described by external_parent_url and
external_target. Else, the item is not part of any external.
It would then be the implementor's responsibility to, for instance,
properly open and close XML tags. To help with this, we can tell the
implementor that we'll never mix items which are part of separate
externals, and will always finish listing an external before listing
the next one. I think your code already guarantees this anyway, so we
can just make this guarantee explicit in the docstring of the
svn_client_list_func2_t interface.
Then we could make the callback implementation track state to detect
by itself whether or not it is entering or leaving an external.
The callback could keep track of the last seen external_parent_url and
external_target pair, and open and close XML tags if they change.
For an example of this approach, see the code in subversion/svn/notify.c
which uses an 'in_external' boolean parameter in the notify_baton for
a similar purpose.
Does this suggestion make sense?
Received on 2012-11-07 15:50:34 CET