You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Malcolm Rowe <ma...@farside.org.uk> on 2006/06/19 12:53:26 UTC
Re: [PATCH] Correct svn_wc_get_pristine_copy_path() comment.
On Mon, Jun 19, 2006 at 06:30:32PM +0530, Madan U Sreenivasan wrote:
> The comment for svn_wc_get_pristine_copy_path() API currently says that
> the function will return a NULL value for the pristine_path parameter when
> the text-base does not exist. However, this is not so.
>
> 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.
>
Or instead of guessing, you could have looked to see where it is
actually used. svn_wc_get_pristine_copy_path() is a public wrapper
around svn_wc__text_base_path(), and is used in only two places, both
in libsvn_client, supporting cat and export of a BASE revision.
> [[[
> Correct comment for svn_wc_get_pristine_copy_path().
>
> * subversion/include/svn_wc.h
> (svn_wc_get_pristine_copy_path): svn_wc_get_pristine_copy_path() doesnt
> return NULL for the pristine_path parameter, if passed a wc path, for
> which a text-base doesn't exist. Removed part of the comment that says
> so.
> ]]]
>
Too much detail, in my opinion. 'Remove incorrect statement in
doc-comments' or anything similar would be fine.
> /** 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.
> + * pristine version of the file. This is needed so clients can do diffs.
> */
I'd also remove the 'This is needed so clients can do diffs' statement,
since this doesn't appear to be true, and even if it was, we shouldn't
try to justify all the possible reasons a client might want access to the
BASE version of the file. Mentioning the word 'BASE' here might be a good
idea too, since 'pristine' isn't immediately obvious as the same thing.
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Correct svn_wc_get_pristine_copy_path() comment.
Posted by Julian Foad <ju...@btopenworld.com>.
Madan U Sreenivasan wrote:
> For the comment I would suggest,
>
> /* Given any @a path, return where svn would look for its BASE version. */
>
> No mention of whether the path exists/if the text-base dir exists/
> if the file has a text-base file/if it is a WC file/if it is a file or
> a dir - nothing. I think it conveys what exactly the function does. The
> function simply doesn't care about these factors.
That's OK with me. (The comment should say "Set @a pristine_path to ..."
rather than "return".)
> Another point I would like to discuss is:
> svn_wc_get_pristine_copy_path() doesn't even care if the path given is
> a file or a dir.
>
> --------------------8<----------------------------------------------8<--------------------------
>>>> svn.wc.get_pristine_copy_path("/tmp")
>
> '/.svn/text-base/tmp.svn-base'
> --------------------8<----------------------------------------------8<--------------------------
>
> Is this intentional? AFAIK, the wc dirs do not have a text-base file.
> Shouldn't this case return an error?
Either the function should validate that the path is a real WC path of a file
that has a text-base, or it should not care about any such things including
whether it is a file or a directory. I don't think this distinction between
file and dir is any more or less important than the other factors that you
mentioned above.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Correct svn_wc_get_pristine_copy_path() comment.
Posted by Madan U Sreenivasan <ma...@collab.net>.
On Wed, 21 Jun 2006 03:24:30 +0530, Julian Foad
<ju...@btopenworld.com> wrote:
> Julian Foad wrote:
[snip]
> 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.
The function does not insist on a a WC file... we can pass any path/file
to the function, and it would return
path/../.svn/text-base/file.svn-base
It is as simple as that.
--------------------8<----------------------------------------------8<--------------------------
>>> import svn.wc
>>> svn.wc.get_pristine_copy_path("/tmp/nonexistantfile")
'/tmp/.svn/text-base/nonexistantfile.svn-base'
>>>
--------------------8<----------------------------------------------8<--------------------------
And I think this implementation is correct.
>
> /** 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.
> */
For the comment I would suggest,
/* Given any @a path, return where svn would look for its BASE version. */
No mention of whether the path exists/if the text-base dir exists/ if
the file has a text-base file/if it is a WC file/if it is a file or a dir
- nothing. I think it conveys what exactly the function does. The function
simply doesn't care about these factors.
Another point I would like to discuss is:
svn_wc_get_pristine_copy_path() doesn't even care if the path given is a
file or a dir.
--------------------8<----------------------------------------------8<--------------------------
>>> svn.wc.get_pristine_copy_path("/tmp")
'/.svn/text-base/tmp.svn-base'
--------------------8<----------------------------------------------8<--------------------------
Is this intentional? AFAIK, the wc dirs do not have a text-base file.
Shouldn't this case return an error?
Regards,
Madan.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Correct svn_wc_get_pristine_copy_path() comment.
Posted by Julian Foad <ju...@btopenworld.com>.
Julian Foad wrote:
> /** 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.
> */
Sorry for multiple posts, but I should explain that the reason I chose that
level of specification is that I think it is important for callers to be able
to use this on items where the WC file is missing and/or the text base is
missing, whereas I do not think callers will need to be able to call this on a
non-WC path or a path within a non-existent directory so I left the behaviour
in those cases deliberately unspecified.
The wording could usefully be a bit more verbose.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Correct svn_wc_get_pristine_copy_path() comment.
Posted by Julian Foad <ju...@btopenworld.com>.
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
Re: [PATCH] Correct svn_wc_get_pristine_copy_path() comment.
Posted by Julian Foad <ju...@btopenworld.com>.
Madan U Sreenivasan wrote:
> On Mon, 19 Jun 2006 18:23:26 +0530, Malcolm Rowe
> <ma...@farside.org.uk> 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".
> [[[
> Correct comment for svn_wc_get_pristine_copy_path().
>
> * subversion/include/svn_wc.h
> (svn_wc_get_pristine_copy_path): Remove incorrect statements from the
> doc-comments.
> ]]]
> Index: subversion/include/svn_wc.h
> ===================================================================
> --- subversion/include/svn_wc.h (revision 20165)
> +++ subversion/include/svn_wc.h (working copy)
> @@ -3034,9 +3034,7 @@
>
>
> /** 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.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Correct svn_wc_get_pristine_copy_path() comment.
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Mon, Jun 19, 2006 at 08:24:07PM +0530, Madan U Sreenivasan wrote:
> Done too. Please find attached the fixed log and patch.
>
> [[[
> Correct comment for svn_wc_get_pristine_copy_path().
>
> * subversion/include/svn_wc.h
> (svn_wc_get_pristine_copy_path): Remove incorrect statements from the
> doc-comments.
> ]]]
>
Looks fine to me. +1 to commit.
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Correct svn_wc_get_pristine_copy_path() comment.
Posted by Madan U Sreenivasan <ma...@collab.net>.
On Mon, 19 Jun 2006 18:23:26 +0530, Malcolm Rowe
<ma...@farside.org.uk> wrote:
> On Mon, Jun 19, 2006 at 06:30:32PM +0530, Madan U Sreenivasan wrote:
[snip]
>> 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.
> Or instead of guessing, you could have looked to see where it is
> actually used.
Yeah... I just didn't feel the need to validate eh's comments. Will keep
this in mind henceforth. Thanks. :)
[snip]
>> [[[
>> Correct comment for svn_wc_get_pristine_copy_path().
>>
>> * subversion/include/svn_wc.h
>> (svn_wc_get_pristine_copy_path): svn_wc_get_pristine_copy_path()
>> doesnt
>> return NULL for the pristine_path parameter, if passed a wc path, for
>> which a text-base doesn't exist. Removed part of the comment that
>> says
>> so.
>> ]]]
>>
>
> Too much detail, in my opinion. 'Remove incorrect statement in
> doc-comments' or anything similar would be fine.
Done.
>> /** 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.
>> + * pristine version of the file. This is needed so clients can do
>> diffs.
>> */
>
> I'd also remove the 'This is needed so clients can do diffs' statement,
> since this doesn't appear to be true, and even if it was, we shouldn't
> try to justify all the possible reasons a client might want access to the
> BASE version of the file. Mentioning the word 'BASE' here might be a
> good
> idea too, since 'pristine' isn't immediately obvious as the same thing.
Done too. Please find attached the fixed log and patch.
[[[
Correct comment for svn_wc_get_pristine_copy_path().
* subversion/include/svn_wc.h
(svn_wc_get_pristine_copy_path): Remove incorrect statements from the
doc-comments.
]]]
Regards,
Madan.