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

All,

Subversion contains some pretty high level code that's pretty inscrutable.  I keep asking myself how we would do <whatever> if we were writing in a higher level or more object-oriented language.

In the merge code we work with <url,rev> coordinates a lot of the time.  Passing these as two separate variables works OK as long as there are only two and not used too many times.  In this code, however, we end up passing them around very many times.  We end up with code like this:
    SVN_ERR(svn_client__get_youngest_common_ancestor(
              NULL /* yc_relpath */, &yc_url, &yc_rev,
              source1_url,
              source1_rev,
              source2_url,
              source2_rev,
              ctx, scratch_pool));

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:

    SVN_ERR(svn_client__get_youngest_common_ancestor(
              &yca, source1_loc, source2_loc,
              ctx, scratch_pool));

It's not just a case of one parameter
instead of two.  Packaging the two related components of the location
together also helps us write code that correctly pairs them up, by making it obvious whenever we deliberately separate them.  In a lot of existing code we find the the two bits of data being separated and passed them
through different code paths.  Often we end up implicitly or explicitly
joining them up again in the end, and often we do this correctly, but often it is hard to follow and very hard to verify.  And every now and then of course we do it wrongly, and may not notice for a long time.

So, I introduced this structure in libsvn_client/merge.c recently:

/* A location in a repository. */
typedef struct repo_location_t
{
  url_uuid_t *repo;
  svn_revnum_t rev;
  const char *url;
} repo_location_t;

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.  Also, suggestions for a better name are welcome -- preferably shorter, as it will have a 'svn_client__' prefix.

For small bits of code, use of a structure such as this -- with its own 'create' and 'dup' functions -- would be over-complex.  However, for the large amounts of code in relatively high level client-library functions, use of it makes a significant contribution to writability and readability.

I'm planning to:

  * Replace the 'repo' field with a simple 'const char *repo_root_url' (and maybe a uuid as well), thus simplifying initial allocation and the 'dup' function.

  * Extend the visibility to make it an "svn_client__" prefixed module-scope type in libsvn_client, and use it to simplify the interfaces of other private svn_client__ functions.

If it works well there, we can consider making it more global.

- Julian
Received on 2012-03-25 18:18:17 CEST