On Mon, Nov 5, 2012 at 5:24 PM, vijay <vi...@collab.net> wrote: > Hi, > > This patch implements '--include-externals' option to 'svn list' [Issue > #4225] [1]. > > All tests pass with 'make check' & 'make davautocheck'. > > Attached the patch and log message. > > Please review this patch and share your thoughts. > > Thanks in advance for your time. > > Thanks & Regards, > Vijayaguru > > [1] > http://subversion.tigris.org/**issues/show_bug.cgi?id=4225<http://subversion.tigris.org/issues/show_bug.cgi?id=4225> >
Hi Vijay, Not sure whether these points have already been discussed in previous threads, but the following two points caught my attention: > +typedef svn_error_t *(*svn_client_list_func2_t)( > + void *baton, > + const char *path, > + const svn_dirent_t *dirent, > + const svn_lock_t *lock, > + const char *abs_path, > > o.k. > + svn_boolean_t notify_external_start, > + svn_boolean_t notify_external_end, > + const char *external_parent_url, > + const char *external_target, > > Maybe, this should be modeled to create a more "seamless" appearance. Only keep the external_parent_url member. If it is NULL, this entry has not been pulled in here by some external. Otherwise it contains the parent URL as defined by your patch. I don't see the see the need to expose more information. Why would you need to group externals? The external_target member should be given implicitly by path / dirent. Am I missing something here? + apr_pool_t *pool); > + > > My second point is how do you process externals defined higher up in the tree? If you don't include them at all: why? -- Stefan^2. -- Certified & Supported Apache Subversion Downloads: * http://www.wandisco.com/subversion/download *
