[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] add adm_access to svn_client_commit_item3_t

From: Stefan Sperling <stsp_at_elego.de>
Date: Tue, 23 Jun 2009 15:59:53 +0100

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.


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.

Received on 2009-06-23 17:00:33 CEST

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.