As the original author of svn_info2_t, I'll share some thoughts which
might be of use.
The struct was originally defined as svn_info_t in svn_client.h, but
with wc-ng, it needed a face lift. At the same time, I recognized the
fact that info can be obtained via a URL, which returned a subset of
that information obtained by querying a working copy path. That
insight led to the current implementation, where svn_info2_t is
defined in svn_types.h, with an opaque reference to svn_wc_info_t
which is defined in svn_wc.h. (Or at least, I think that's the
current state of affairs.)
There is an email thread from when this was all happening in which
Greg and I debated whether or not svn_info2_t should be in svn_wc.h or
svn_client.h or svn_types.h. It basically boiled down to the fact
that: a) info on a URL is a subset of info on a WC; and b) a structure
which returns information solely coming from a URL shouldn't be
defined in svn_wc.h. (A client doesn't even need a working copy to
run 'svn info $URL'.)
In the end, I'm for whatever is more maintainable, but let's not try
to over-engineer things.
-Hyrum
PS - Thanks for doing the somewhat thankless task of API review. Lots
of this code need more eyes!
On Tue, Jun 7, 2011 at 12:34 PM, Julian Foad <julian.foad_at_wandisco.com> wrote:
> 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 21:13:07 CEST