Re: [PATCH] Implement '--include-externals' option to 'svn list'

On Thursday 08 November 2012 04:10 PM, vijay wrote:
> 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.
>

I have removed the two boolean parameters notify_external_start and
notify_external_end from svn_client_list_func2 implementation. Now, the
callback keeps track of last seen external information and print details
based on the change. I have updated the docstring of
svn_client_list_func2_t also.

Attached the updated patch and log message.

Thanks & Regards,
Vijayaguru