On Wednesday 07 November 2012 08:19 PM, Stefan Sperling wrote:
> 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?
>
Yes, stefan. It makes sense.
Initially, I was struggling hard to implement this option without the
two boolean parameters, notify_external_start and notify_external_end.
I couldn't find any other way to do it. So I added the two parameters
reluctantly.
Now, I have an interesting suggestion. Let me dig through svn/notify.c
and come up with an implementation.
Thanks & Regards,
Vijayaguru
Received on 2012-11-08 11:41:20 CET