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
