Julian Foad wrote:
> Madan U Sreenivasan wrote:
>> On Mon, 19 Jun 2006 18:23:26 +0530, Malcolm Rowe wrote:
>>> On Mon, Jun 19, 2006 at 06:30:32PM +0530, Madan U Sreenivasan wrote:
>>
>>>> I spoke with eh on irc sometime back, and eh felt that this API might
>>>> be used in places where the text-base does not exist, but has to be
>>>> created. For example in case of a cp/mv.
>
> I'm not sure exactly what you mean by "has to be created".
Never mind those particular words; I didn't see how this statement related to
your patch. "Might be in use currently (in third-party projects) ..." or
"Might be used in future if we define it in a suitable way ..."? And what if
it is or will be? Anyway, I think I have bypassed the need for answers to this
in my analysis below.
>> Index: subversion/include/svn_wc.h
>> ===================================================================
>> /** Given a @a path to a wc file, return a @a pristine_path which
>> points to a
>> - * pristine version of the file. This is needed so clients can do
>> - * diffs. If the WC has no text-base, return a @c NULL instead of a
>> - * path.
>> + * BASE version of the file.
>> */
>> svn_error_t *svn_wc_get_pristine_copy_path(const char *path,
>> const char **pristine_path,
>
> I don't see how this function can guarantee to return a path to a
> pristine version of the file if it is possible for a file not to have a
> text-base. Therefore I assume it returns an error in that case. If a
> caller wants to take special action in that case, rather than just
> aborting, the caller needs to know what error will be returned and
> therefore this interface needs to document it.
Well, I've just gone and looked at the implementation and the two callers in
our client code. What I find is:
* The function returns the path where the text base would be, even if the text
base does not exist, and regardless of whether the given path exists or is in a
WC. I was wrong in assuming it would return an error.
* The two callers in our client only call this on WC files that are known to
have text-bases, so they don't expect it to return NULL and don't check for
that. (They abort if it returns an svn_error_t error.)
This API was not doing what it said it did. Did it ever? If it never did,
there can't exist any (properly tested) programs that rely on it doing so, and
therefore we can change its definition without "revving" it.
Your new description still does not match the implementation. Given a path to
a WC file, the current implementation does not necessarily return a path which
points to a pristine version of the file.
We need to resolve this one way or another. Possible designs are:
1) Given a path of a WC file (which need not exist),
get the path at which its text base is or would be if it had one.
2) Given a path of a WC file (which need not exist),
get the path of its text base or NULL if it does not have one.
3) Given a path of a WC file (which need not exist),
get the path of its text base, or return an error if it does not have one.
(1) matches the current implementation, (2) is a more rigorous statement of the
currently documented behaviour, and (3) is another reasonable option. I don't
have a preference for any of these. I'm happy with your decision to keep the
implementation as it is, so let's make the documentation match it. I suggest:
/** Given a @a path to a WC file, set @a *pristine_path to the path of its
* pristine (base) copy. The file at @a path need not exist. The file at
* @a *pristine_path might not exist.
*/
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Jun 20 23:54:52 2006