Re: New struct for holding (url, rev, repo-root) coordinates in client merge code

Greg Stein wrote:

> Julian Foad wrote:
>> ...
>> Sometimes we have more than two variables because we also want the repository root URL, either to check it's the same repo as some other location or to derive the repo-root-relative path (or fspath).  It's reached the point where it's definitely worth having a struct that contains <url, rev, repo_root>.  Now we can write things like:
>
> Please spell it like "repos_root_url" to be similar to precedent [...]

Done.  Currently:

{
  const char *repos_root_url;
  const char *repos_uuid;
  svn_revnum_t rev;
  const char *url;
}

> If you're going to have the repos_root_url in there, then I would
> recommend switching to repos_relpath rather than keeping a full URL.
> The relpath is being used within Ev2, and I'd like to see RA move over
> to <root, relpath> pairs for clarity/consistency (RA's arbitrary root
> and reparenting is very annoying and brittle).

Yes, I'd support switching to repos_relpath.  I think the use of this struct makes that easier to achieve because now we carry around enough information to switch between one and the other at any point without relying on external data (such as an RA session) that may be carried around to some but not all of the functions that may want to do the conversion.

Because libsvn_client predominantly uses the full URL, we'll be best off sticking with URL until the point at which most of the client APIs and code are using this struct, and then we'll find that there are not many places left that actually care about the URL.  Some time around that point we can gainfully switch to storing the relpath and creating the URL on demand.

>> It's not ideal as it is.  The pointer to a url_uuid_t sub-structure
>> is unnecessary at this stage (that is, unless and until we might
>> make the url_uuid_t structure into some bigger "repository"
>> abstraction); it would be simpler and therefore better right now to
>> embed the repo root URL and UUID directly.
>
> Yeah. The substructure doesn't add any value, it seems.

Gone.

>>  Also, suggestions for a better name are welcome -- preferably
>> shorter, as it will have a 'svn_client__' prefix.
>
> How about svn_client__pathrev_t, since it represents PATH_at_REV ?

That's not really lovely but it's better than what I've got.

I'll roll this out to libsvn_client private name space now.

- Julian
Received on 2012-04-03 10:27:55 CEST