Re: Review of lock-many API

Hi Philip.

Thanks for the other fixes and doc tweaks you committed related to this.

Philip Martin wrote:

> Julian Foad writes:
>> Hi Philip. I hope you don't mind, I've made a few more review comments
>> and suggestions, in the form of a patch.
>
> It would be better if your mail program didn't convert tabs to hard
> spaces, as it is I can't apply the patch.

Oops, sorry about that. (I copied it from a terminal screen, forgetting that the diff headers should contain tabs.)

>> +/** ### Document here not what the callers will do but what this function
>> + *     itself is promised and what it is allowed/required to do.
>
> I'm not really sure what you want.

What I mean is, as a matter of documentation style, to document this more in terms of itself and depend less on its current callers and their environments.

>> - * @a path and @a lock are allocated in the result_pool passed to
>> - * svn_fs_lock_many/svn_fs_unlock_many and so will persist beyond the
>> - * callback invocation. @a fs_err will be cleared after the callback
>> - * returns, use svn_error_dup() to preserve the error.

For example: The reader may not be thinking immediately about the two callers named here, having come here from one of the (at the time of writing) six other direct callers. Two of those don't have a result_pool as such and don't clear the error as such. What is written is still true, it just could be more independent. Perhaps like this:

 * @a lock contains the new (as opposed to the previous) lock on @a path
 * if successfully locked, or is NULL if successfully unlocked. In these
 * cases, @a fs_err is NULL.
 *
 * If there was an error in trying to lock or unlock the @a path, @a fs_err
 * is the error and @a lock is NULL. The error is owned by the caller; this
 * function should not modify or clear it but can use svn_error_dup() to
 * preserve a copy of it. The error will not be reported by the caller. This
 * function should report it if required.

Describing the error that way additionally gives me the idea that it should be passed by a 'const' pointer, as this function is not supposed to modify it.

And it's not necessarily a FS-layer error: it could be a repos-layer error from the pre-lock or pre-unlock hook. So maybe rename it to just 'err'?

> That documents the what the callback gets given, the callback can do
> anything it likes with the data.
>
>> + * ###
>> + * ### If the callback returns an error no further callbacks will be made
>> + * ### and svn_fs_lock_many/svn_fs_unlock_many will return an error.
>
> That documents the only thing the callback can do that the caller cares
> about.

This is a promise about what those particular callers
will do, and may not apply to other callers of this type of function, so better to move this to the doc strings of those callers.

Of course sometimes it's expediant to document at a place like this the common behaviours that should be
shared by all uses -- it's not possible to make a clear-cut
distinction.

- Julian
Received on 2014-04-08 13:22:32 CEST