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?