My current plan comes in two variants.
Minimal solution: move the whole type to libsvn_wc, putting it in the
svn_wc_ name space. A bit ugly in terms of how the svn_client_info3()
API would look, returning a svn_wc data type from a svn_client function,
but that's by no means unusual for us.
Better solution: make two of these info structures, one for libsvn_wc
which can be in the private svn_wc__ name space with its retrieval
function svn_wc__get_info(), and one for libsvn_client which would be in
the svn_client_ name space with svn_client_info3(). Both would contain
the same data, at first pass, and thus might well be identical, but
serve logically distinct purposes.
- Julian
On Tue, 2011-06-07 at 12:34 +0100, Julian Foad wrote:
> Two things about the new svn_info2_t structure.
>
> svn_info2_t contains the following fields, which I'll group into four
> sections:
>
> repos node coordinates:
> repos_root_URL
> repos_UUID
> rev
> URL
>
> repos node basic info:
> kind
> size
> last_changed_rev
> last_changed_date
> last_changed_author
>
> repository path lock:
> lock
>
> WC info:
> wc_info
>
>
> 1) I think we need to change the "shape" of this structure so that WC
> info is not "inside" it.
>
> The idea behind this current structure, as I understand it, is that it
> should hold repos info and/or WC info about a node, and should be shared
> between libraries, such that libsvn_wc can fill in both parts whereas
> libsvn_ra (if it were directly used there, which it isn't currently)
> would fill in only the repos part and leave the WC part null.
>
> Currently the repos into fields are immediate fields, and there is a
> pointer to a WC info struct, so that effectively it "contains" an
> (optional) WC info struct, like this:
>
> svn_info2_t:
> +---------------------------------------------+
> | |
> | svn_wc_info_t: |
> | +------------------+ |
> | | | |
> | (repos node info) | (wc node info) | |
> | +------------------+ |
> +---------------------------------------------+
>
> The definition of svn_wc_info_t is in libsvn_wc, whereas the definition
> of the overall svn_info2_t is global, in svn_types.h, with the WC part
> being declared there as a pointer to an "incomplete" structure type.
>
> However, there is a problem with this, which is exposed by asking "how
> would we write an svn_info2_dup() function?" Any structure like this
> should have a 'dup' function. The function should be global (so
> implemented in the lowest layer, libsvn_subr), as any code using an
> svn_info2_t might need it. Yet it needs to access libsvn_wc data and
> use libsvn_wc functions in order to duplicate the WC part. That creates
> a cyclic dependency - a layering violation.
>
> The interesting thing to note is that dependencies between "repos info"
> and "WC info" are not symmetrical. The repos layer does not know
> anything about WC info, but the WC layer *does* already know repos info
> about each node. Therefore the shape we need, in order to cleanly
> express info about a WC node, is rather the other way around, something
> like this:
>
> svn_wc_info2_t:
> +-----------------------------------------------+
> | |
> | svn_node_info_t: |
> | +------------------+ |
> | | | |
> | |(repos node info) | (wc node info) |
> | +------------------+ |
> +-----------------------------------------------+
>
> Then to report a WC node we would use svn_wc_info2_t, whereas to report
> a repos node (nothing to do with a WC) we would use this putative
> 'svn_node_info_t' alone.
>
> So why do we currently have the WC info innermost? I think it is
> because the only thing we do with the info is pass it to a generic info
> receiver function that prints whatever info is there (the WC info and/or
> the repos node info), and the repos node info is the common part of what
> it receives, so it feels Wrong to declare the receiver as taking a "WC
> info" structure and more Right to declare it as taking a "generic info
> structure that might include WC info".
>
> Now, I think we have to analyze that last thought more carefully. If we
> think about a replaced WC node, for example, we can see that a WC node
> is not logically "a repository node plus some WC-specific info"; it
> might in fact refer to two different repository nodes, or none at all in
> some cases. So the feeling was misleading. The receiver's input should
> instead be structured differently. Basically, in pseudo-code it should
> be:
>
> info_receiver(repos-info OR wc-info)
>
> in some concrete form or other.
>
> I'm working on how exactly to res-structure this now. Let me know any
> thoughts.
>
>
> 2) An independent thing. Note that the "repos node basic info" fields
> in svn_info2_t are exactly what the existing 'svn_dirent_t' structure is
> defined to hold:
>
> /** A general subversion directory entry. */
> typedef struct svn_dirent_t
> {
> /** node kind */
> svn_node_kind_t kind;
>
> /** length of file text, or 0 for directories */
> svn_filesize_t size;
>
> /** does the node have props? */
> svn_boolean_t has_props;
>
> /** last rev in which this node changed */
> svn_revnum_t created_rev;
>
> /** time of created_rev (mod-time) */
> apr_time_t time;
>
> /** author of created_rev */
> const char *last_author;
>
> /* IMPORTANT: If you extend this struct, check svn_dirent_dup(). */
> } svn_dirent_t;
>
> The only differences are different names for the fields and one extra
> field, 'has_props'.
>
> I would prefer to re-use the svn_dirent_t structure instead of putting
> separate fields in an info structure. (If a given info reporting
> function doesn't fill in the 'has_props' field, we will simply say so.)
> That would simplify existing code such as libsvn_client's
> build_info_from_dirent() by allowing us to pass that single
> sub-structure around directly instead of unpacking and re-packing its
> fields. With this change, the current svn_info2_t definition would look
> like:
>
> repos node coordinates:
> repos_root_URL
> repos_UUID
> rev
> URL
>
> repos node basic info (svn_dirent_t):
> node_info
>
> repository path lock (svn_lock_t):
> lock
>
> WC info (svn_wc_info_t):
> wc_info
>
> (And then we can observe that the set of repos node coordinates is
> another group of fields that we should some time create a structure for.
> But that's a separate issue.)
>
> - Julian
>
>
Received on 2011-06-07 19:35:00 CEST