On 11.11.2016 15:30, Branko Čibej wrote:
> 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.
I think having NULL and an empty list describing the
same thing is not a good idea. So, if we allow NULL,
then the empty list should become the degenerate
case of "nothing could possibly match".
Since API users say they prefer NULL over an empty
array, I changed the code accordingly in r1769485.
-- Stefan^2.
Received on 2016-11-13 11:44:58 CET