Re: Review of lock-many API

Hi Philip.

Philip Martin wrote:

> Julian Foad writes:
>> Index: subversion/svnserve/serve.c
>> ===================================================================

>> -  /* The client expects results in the same order as paths were supplied. */
>> +  /* Send the results. The client expects results in the same order as
>> +     paths were supplied. */
>>    for (i = 0; i < path_revs->nelts; ++i)
>>      {
>>        const char *path, *full_path;
>> @@ -2816,6 +2825,16 @@ static svn_error_t *lock_many(svn_ra_svn
>>          result = svn_hash_gets(authz_results, full_path);
>>        if (!result)
>>          /* No result?  Should we return some sort of placeholder error? */
>> +        /* ### [JAF] Is that a question to reviewers? Certainly something
>> +           has gone wrong. Maybe svn_repos_fs_lock2 returned an error
>> +           overall, having processed none or only some paths -- is it
>> +           allowed to do so?
>
> It could run out of disk space half way through.

AFAICT, if I'm reading this right, then a valid reason for not finding a result here is if svn_repos_fs_lock_many returned an error overall, having locked some but not all of the paths. Doesn't really matter why.

Breaking here seems wrong. We're processing the targets in this loop in a different order from the hash order that svn_repos_fs_lock_many() was using to lock them. Therefore we are potentially stopping before we have given responses to all the paths that were successfully locked. We would stop the response list at this point, and terminate it with a successful "done" keyword, but the response list would have too few elements. This seems arbitrary and broken.

I can think of two alternatives. We could just not generate a response list in the case where svn_repos_fs_lock_many returns an error, but simply report the error. That's effectively no worse than the current behaviour. Or we could insert an error response or some sort of place-holder entry in the list for this target, and then continue to the next target, to give a full response.

Does that make sense and would either of those options work?

- Julian

>>                         Breaking here would signal to the client that
>> +           something went wrong, because they'll see a too-short response
>> +           list, but would also potentially hide information about further
>> +           targets that were processed, some of which perhaps were locked
>> +           successfully. (Note: we're scanning them here in a different
>> +           order from the order in which svn_repos_fs_lock2() processed
>> +           them.) */
>>          break;
Received on 2014-04-03 20:07:50 CEST