Oh, and if it hasn't already grown one yet, svn_info2_t needs a
constructor. svn_wc_info_t doesn't, since it's always created by
libsvn_wc.
-Hyrum
On Tue, Jun 7, 2011 at 2:12 PM, Hyrum K Wright <hy...@hyrumwright.org> wrote:
> 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.f...@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
>>>
>>>
>>
>>
>>
>
