Re: [PATCH] Implement '--include-externals' option to 'svn list'

On Mon, Nov 5, 2012 at 5:24 PM, vijay <vijay_at_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
*