On Tue, Jun 23, 2009 at 09:51:16AM -0500, Hyrum K. Wright wrote:
> Huihuang,
> Thanks for the patched and your continued work as part of Summer of
> Code. In the future, could you make sure your patches are attached with
> the correct mime-type, as requested by HACKING? Using a mime-type of
> text/x-diff, text/x-patch, or text/plain makes it possible to view and
> review the patch inline in a mail reader, and increases the likelihood it
> will get good review.
>
> A few comments:
> I see from the patch you are adding a field to
> svn_client_commit_item3_t. It wasn't obvious before r38160, but adding
> new members of a public structure should be done at the end of the
> structure. You should also update any constructors or copy functions to
> account for the new member.
Yes.
The constructors and copy functions are svn_client_commit_item3_create()
and svn_client_commit_item3_dup().
Note that making a copy of and adm_access_t may not make sense.
So I think the _dup() function should *not* make a newly allocated
copy of the adm_access_t. Rather, it should just copy the pointer
value, and the docstring needs to be changed from:
* Return a duplicate of @a item, allocated in @a pool. No part of the
* new structure will be shared with @a item.
to this:
* Return a duplicate of @a item, allocated in @a pool. No part of the
* new structure will be shared with @a item, except for the adm_access
* member.
> I also noticed that you used svn_wc_adm_access_t. While that's the
> traditional approach, we're in the midst of reworking the working copy
> library and deprecating the use of svn_wc_adm_access_t in favor of
> svn_wc_context_t. Since a svn_wc_context_t isn't tied to a particular
> working copy or working copy directory, it won't work as a way to find
> your way back home to the source working copy.
>
> (Given this information, part of me wonders how the split of
> functionality between libsvn_wc and libsvn_client may change with wc-ng,
> but that's a conversation for another day.)
So let's just use an adm_access_t for now. If we need to change
this, we can still do so.
Stefan
Received on 2009-06-23 17:00:33 CEST