On 11.11.2016 15:04, Patrick Steinhardt wrote:
> On Wed, Nov 09, 2016 at 02:05:38PM +0100, Stefan Sperling wrote:
>> On Wed, Nov 09, 2016 at 11:27:47AM +0100, Patrick Steinhardt wrote:
>>> Hi,
>>>
>>> while continuing my work on the obstructed checkout patch I just
>>> updated to the new `svn_client_list4` call, which introduced a
>>> new `patterns` parameter. I initially run into a `NULL` pointer
>>> derefeernce, as I expected that the patterns parameter may also
>>> be `NULL`, as it is often the case for parameters which are not
>>> in use.
>>>
>>> The attached patch changes the function to accept a `NULL`
>>> argument for `parameters` in addition to an empty array, which is
>>> mostly a convenience/consistency thing for callers of the new
>>> function.
>>>
>>> Regards
>>> Patrick
>> I agree with this semantic API change.
>>
>> Should this patch not also adjust the docstring for svn_client_list4()
>> in the file subversion/include/svn_client.h?
> Probably, yes. Thinking about this some more, I'm not that happy
> with this patch. I still think it should be possible for callers
> to pass `NULL`, meaning that no filtering should be done. But I'm
> not particularly happy about an empty array meaning that no
> filtering should be done, as well.
>
> A use case is that first, the array is filled dynamically with
> items to filter by. But in case that the array is not filled with
> anything, I'd expect the function to return nothing instead of
> everything. So maybe the behavior should even be:
>
> - NULL pointer: no filtering at all
> - empty array: return nothing
> - filled array: return only items matching at least one of the
> items
>
> For me as a developer, this behavior would match my expectations
> much better (even though one could argue that one should simply
> read the docstring).
I agree; we have other instances of such behaviour, e.g., for returned
revision properties in svn_client_log5() where NULL array means return
all propertes and empty array means return none.
Since svn_client_list4() is new on trunk, I suggest you just send in a
patch that fixes this behaviour as you described.
-- Brane
Received on 2016-11-11 15:30:16 CET