[PATCH] Introduce the translate_paths parameter to svn_ra_get_merge_info()

[Madan U Sreenivasan <ma...@collab.net>]

Hi,

    Further to a discussion culminating in the mail at  
http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=117981, we  
decided to implement the translate_paths parameter for  
svn_ra_get_merge_info(). Actually dberlin mentioned thus:
we really need to split include_parents into two arguments:
use_parent_info
and
translate_paths

     This could be interpreted as: "give the use_parent_info param to more  
than just bool values and interpret the meaning at the fs-layer." But I  
decided to leave the include_parents param alone and use a new parameter  
because multiple values of include_parents will confuse the reader and  
hence would be difficult to maintain.

     Pl. find attached a patch and the corresponding logfile. Its almost  
wake-up time now. So, request you to pl. bear with me if I have made any  
minor mistakes. :)

[[[
Introduce the translate_paths parameter, to conditionally append
the basename of the target to the mergefrom source paths.

(In branches/merge-tracking)

* subversion/libsvn_ra/ra_loader.c (svn_ra_get_merge_info):
* subversion/libsvn_ra/ra_loader.h (svn_ra__vtable_t.get_merge_info):
* subversion/include/svn_fs.h (svn_fs_get_merge_info):
* subversion/include/svn_repos.h (svn_repos_fs_get_merge_info):
* subversion/include/svn_ra.h (svn_ra_get_merge_info):
* subversion/libsvn_fs/fs-loader.h (root_vtable_t.get_merge_info):
* subversion/libsvn_fs/fs-loader.c (svn_fs_get_merge_info):
* subversion/libsvn_ra_local/ra_plugin.c (svn_ra_local__get_merge_info):
* subversion/libsvn_repos/fs-wrap.c (svn_repos_fs_get_merge_info):
* subversion/libsvn_ra_dav/ra_dav.h (svn_ra_dav__get_merge_info):
* subversion/libsvn_fs_fs/tree.c (fs_get_merge_info):
    Introduce the translate_paths parameter.

* subversion/mod_dav_svn/mergeinfo.c
   (dav_svn__get_merge_info_report): Pass on the translate_paths value
    using the 'S:translate-paths' namespace.

* subversion/libsvn_ra_dav/mergeinfo.c
   (svn_ra_dav__get_merge_info): Read translate_paths value from the
    'S:translate-paths' namespace.

* subversion/libsvn_ra_svn/client.c
   (ra_svn_get_merge_info): Add the translate-paths value to the  
get-merge-info
    tuple.

* subversion/svnserve/serve.c
   (get_merge_info): Retreive the translate-paths value from the  
get-merge-info
    tuple.

* subversion/libsvn_ra_svn/protocol
   (get-merge-info): Document the translate-path param.

* subversion/tests/libsvn_fs/fs-test.c
   (get_merge_info) : Modify to use the new signature of
    svn_fs_get_merge_info()

* subversion/libsvn_fs_fs/tree.c
   (get_merge_info_for_path): Introduce the translate_paths parameter, and
    execute the translate paths logic only if this variable is TRUE.
]]]

Regards,
Madan.

Re: [PATCH] Introduce the translate_paths parameter to svn_ra_get_merge_info()

[Madan U Sreenivasan <ma...@collab.net>]

On Thu, 20 Jul 2006 09:41:09 +0530, Daniel Rall <dl...@collab.net> wrote:

> On Wed, 19 Jul 2006, Madan S. wrote:

[snip]

>> Introduce the translate_paths parameter, to conditionally append
>> the basename of the target to the mergefrom source paths.
> ...
>
> Would you describe the use case for this behavior?  I'd like to
> understand why it's needed (I assume in libsvn_client/diff.c).

Okay currently, if svn_ra_get_merge_info() is invoked on  
'/branches/branch1/cc' (target),

If /branches/branch1/cc has mergeinfo
    returned_merge_info = mergeinfo_of_/branches/branch1/cc
else
    returned_merge_info = mergeinfo_of_/branches/branch1 (or the earliest  
parent in the hierarchy which has mergeinfo)

Now, in addition to that, all the mergefrom paths ( the /trunk in  
'/trunk:1-10' mergeinfo format), are appended with the basename of the  
target path - in this case cc.

So, the mergeinfo I would get on invokation of svn_ra_get_merge_info() for  
/branches/branch1/cc would look something like:
/trunk/cc:1-5

(*) I opine that we only need to store
/trunk:1-5 as the mergeinfo in /trunk/branches/branch1/cc.

Now, that said, the term translate_paths might be a misnomer. I could be  
more like append_base_path or something like that.

> I don't grasp every last detail here, but the behavior doesn't seem
> valid to me.  How can the client do path translation when it doesn't
> have access to the repository FS?  I discussed the following two cases
> for translate_paths=false on IRC with DannyB:
>
> 1) When include_parents=false, we don't include inherited merge
> history for the parents of the requested paths.  Because of this, only
> the requested paths themselves will ever be returned by the API,

Did you mean 'Only *the mergeinfo of* the requested paths will ever be  
returned' ?

> meaning that the translate_paths parameter becomes irrelevant (other
> than the fact that translate_paths=true isn't even a valid value
> here).

Agree. But as I said earlier, as long as svn_ra_get_merge_info() appends  
the basename of the targetpath to the mergefrom sources, this portion of  
the client cannot use it. The reason being (*) I mentioned above.

> 2) Otherwise, when include_parents=true, you must make assumptions on
> the client-side about how your non-translated path relates to your WC.
> While most of the time these assumptions will turn out to be correct,
> aren't there edge cases which could bite us when we don't have access
> to the repos FS?
>
> For instance, if you request merge info for /branches/foo/bar, and the
> merge info for that path comes back "/trunk:1-5", but the path
> /trunk/bar does not represent the same object as /branches/foo/bar
> (e.g. if /trunk/bar was renamed to /trunk/baz, and a new /trunk/bar
> has been created), appending "bar" to "/trunk" on the client side
> isn't correct -- we need the merge info's source to be translated to
> "/trunk/baz:1-5" (or whatever).  I don't see how the client can do
> this efficiently.

Am sorry. I do not understand the need for such a translation of paths.  
Could you pl. explain?

Looking at your example, I have two points to make:

1) IMHO, The mergeinfo '/trunk/bar:1-5' should never change to  
'/trunk/baz:1-5', on any given target path. The reason is: between r1-5,  
the mergedfrom path *was* /trunk/bar and not /trunk/baz.

2) The so-called path translation mechanism (available in  
get_merge_info_for_path()) currently only appends the basename to the  
mergefrom source paths. It does not have the intelligent mechanism that  
you mention.

Regards,
Madan.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Introduce the translate_paths parameter to svn_ra_get_merge_info()

[Daniel Rall <dl...@collab.net>]

On Wed, 19 Jul 2006, Madan S. wrote:
...
> http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=117981, we  
> decided to implement the translate_paths parameter for  
> svn_ra_get_merge_info(). Actually dberlin mentioned thus:
> we really need to split include_parents into two arguments:
> use_parent_info
> and
> translate_paths
> 
>     This could be interpreted as: "give the use_parent_info param to more  
> than just bool values and interpret the meaning at the fs-layer." But I  
> decided to leave the include_parents param alone and use a new parameter  
> because multiple values of include_parents will confuse the reader and  
> hence would be difficult to maintain.
...
> [[[
> Introduce the translate_paths parameter, to conditionally append
> the basename of the target to the mergefrom source paths.
...

Would you describe the use case for this behavior?  I'd like to
understand why it's needed (I assume in libsvn_client/diff.c).




I don't grasp every last detail here, but the behavior doesn't seem
valid to me.  How can the client do path translation when it doesn't
have access to the repository FS?  I discussed the following two cases
for translate_paths=false on IRC with DannyB:

1) When include_parents=false, we don't include inherited merge
history for the parents of the requested paths.  Because of this, only
the requested paths themselves will ever be returned by the API,
meaning that the translate_paths parameter becomes irrelevant (other
than the fact that translate_paths=true isn't even a valid value
here).

2) Otherwise, when include_parents=true, you must make assumptions on
the client-side about how your non-translated path relates to your WC.
While most of the time these assumptions will turn out to be correct,
aren't there edge cases which could bite us when we don't have access
to the repos FS?

For instance, if you request merge info for /branches/foo/bar, and the
merge info for that path comes back "/trunk:1-5", but the path
/trunk/bar does not represent the same object as /branches/foo/bar
(e.g. if /trunk/bar was renamed to /trunk/baz, and a new /trunk/bar
has been created), appending "bar" to "/trunk" on the client side
isn't correct -- we need the merge info's source to be translated to
"/trunk/baz:1-5" (or whatever).  I don't see how the client can do
this efficiently.