You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by vijay <vi...@collab.net> on 2012/11/05 17:24:56 UTC
[PATCH] Implement '--include-externals' option to 'svn list'
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
Re: [PATCH] Implement '--include-externals' option to 'svn list'
Posted by vijay <vi...@collab.net>.
On Thursday 15 November 2012 08:10 PM, C. Michael Pilato wrote:
> On 11/15/2012 03:47 AM, Bert Huijben wrote:
>>>> Can't you just add the new argument to every call to list_func() that
>>>> applies to an external?
>>>
>>>
>>> There is no separate list_func() call to list external items. We are
>>> just calling svn_client_list3() recursively for each external, which in
>>> turn calls list_func() using get_dir_contents(). I am struggling a bit
>>> hard to carry forward external information via svn_client_list3(). Can
>>> we add two more arguments external_parent_url and external_target to
>>> svn_client_list3()?
>>
>> You could just create a static function with those extra arguments and make
>> the outer svn_client_list3() call into that function.
>> Please try to avoid adding implementation details to the public api, unless
>> absolutely necessary.
>> (We have such wrappers in almost every case where we use externals)
>>
>> This model would also allow future improvements like re-using the
>> ra-session, without updating the public function prototype.
>
> Right. The pattern (to the degree that there is one) would have you
> creating a function named something like svn_client__list_internal(), which
> has all the code from svn_client_list3() plus the extra (optional)
> parameters and functionality you need. svn_client_list3() then becomes a
> thin wrapper around that function.
>
Thanks Bert and C-Mike. I have created a new function
svn_client__list_internal() with the parameters I need and made
svn_client_list3() as a wrapper around that function.
Thanks & Regards,
Vijayaguru
Re: [PATCH] Implement '--include-externals' option to 'svn list'
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 11/15/2012 03:47 AM, Bert Huijben wrote:
>>> Can't you just add the new argument to every call to list_func() that
>>> applies to an external?
>>
>>
>> There is no separate list_func() call to list external items. We are
>> just calling svn_client_list3() recursively for each external, which in
>> turn calls list_func() using get_dir_contents(). I am struggling a bit
>> hard to carry forward external information via svn_client_list3(). Can
>> we add two more arguments external_parent_url and external_target to
>> svn_client_list3()?
>
> You could just create a static function with those extra arguments and make
> the outer svn_client_list3() call into that function.
> Please try to avoid adding implementation details to the public api, unless
> absolutely necessary.
> (We have such wrappers in almost every case where we use externals)
>
> This model would also allow future improvements like re-using the
> ra-session, without updating the public function prototype.
Right. The pattern (to the degree that there is one) would have you
creating a function named something like svn_client__list_internal(), which
has all the code from svn_client_list3() plus the extra (optional)
parameters and functionality you need. svn_client_list3() then becomes a
thin wrapper around that function.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
RE: [PATCH] Implement '--include-externals' option to 'svn list'
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: vijay [mailto:vijay@collab.net]
> Sent: donderdag 15 november 2012 08:53
> To: Bert Huijben
> Cc: 'Subversion Development'
> Subject: Re: [PATCH] Implement '--include-externals' option to 'svn list'
>
> On Tuesday 13 November 2012 06:45 PM, Bert Huijben wrote:
> >
> >
> >> -----Original Message-----
> >> From: vijay [mailto:vijay@collab.net]
> >> Sent: dinsdag 13 november 2012 14:06
> >> To: Subversion Development
> >> Subject: Re: [PATCH] Implement '--include-externals' option to 'svn
list'
> >>
> >> On Tuesday 13 November 2012 03:02 AM, Stefan Sperling wrote:
> >>>> Attached the updated patch and log message.
> >>>
> >>>> + /* Notify that we're about to handle an external. */
> >>>> + SVN_ERR(list_func(baton, NULL, NULL, NULL, NULL,
> >>>> + externals_parent_url,
> >>>> + item->target_dir, iterpool));
> >>>
> >>> The docstring of svn_client_list_func2_t doesn't say if it is valid
> >>> for path or dirent to be NULL.
> >>>
> >>> However, you're passing NULL for these parameters before listing the
> >>> external. Do we really need these two extra list_func() calls before
> >>> and after listing the external? I was expecting them to go away.
> >>
> >>
> >> If we are removing these list_func() calls, how can we pass the
> >> arguments external_parent_url and external_target to the callback
> >> function? Is there any way to pass those arguments via
svn_client_list3()?
> >
> > Can't you just add the new argument to every call to list_func() that
> > applies to an external?
>
>
> There is no separate list_func() call to list external items. We are
> just calling svn_client_list3() recursively for each external, which in
> turn calls list_func() using get_dir_contents(). I am struggling a bit
> hard to carry forward external information via svn_client_list3(). Can
> we add two more arguments external_parent_url and external_target to
> svn_client_list3()?
You could just create a static function with those extra arguments and make
the outer svn_client_list3() call into that function.
Please try to avoid adding implementation details to the public api, unless
absolutely necessary.
(We have such wrappers in almost every case where we use externals)
This model would also allow future improvements like re-using the
ra-session, without updating the public function prototype.
Bert
Re: [PATCH] Implement '--include-externals' option to 'svn list'
Posted by vijay <vi...@collab.net>.
On Tuesday 13 November 2012 06:45 PM, Bert Huijben wrote:
>
>
>> -----Original Message-----
>> From: vijay [mailto:vijay@collab.net]
>> Sent: dinsdag 13 november 2012 14:06
>> To: Subversion Development
>> Subject: Re: [PATCH] Implement '--include-externals' option to 'svn list'
>>
>> On Tuesday 13 November 2012 03:02 AM, Stefan Sperling wrote:
>>>> Attached the updated patch and log message.
>>>
>>>> + /* Notify that we're about to handle an external. */
>>>> + SVN_ERR(list_func(baton, NULL, NULL, NULL, NULL,
>>>> + externals_parent_url,
>>>> + item->target_dir, iterpool));
>>>
>>> The docstring of svn_client_list_func2_t doesn't say if it is valid
>>> for path or dirent to be NULL.
>>>
>>> However, you're passing NULL for these parameters before listing the
>>> external. Do we really need these two extra list_func() calls before
>>> and after listing the external? I was expecting them to go away.
>>
>>
>> If we are removing these list_func() calls, how can we pass the
>> arguments external_parent_url and external_target to the callback
>> function? Is there any way to pass those arguments via svn_client_list3()?
>
> Can't you just add the new argument to every call to list_func() that
> applies to an external?
There is no separate list_func() call to list external items. We are
just calling svn_client_list3() recursively for each external, which in
turn calls list_func() using get_dir_contents(). I am struggling a bit
hard to carry forward external information via svn_client_list3(). Can
we add two more arguments external_parent_url and external_target to
svn_client_list3()?
If you want to look at the patch, here[1] is the latest version. Any
suggestions are welcome.
[1] http://svn.haxx.se/dev/archive-2012-11/0313.shtml
Thanks & Regards,
Vijayaguru
>
> That would allow removing the initial call, while the callee always has the
> information it needs.
>
> Bert
>
RE: [PATCH] Implement '--include-externals' option to 'svn list'
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: vijay [mailto:vijay@collab.net]
> Sent: dinsdag 13 november 2012 14:06
> To: Subversion Development
> Subject: Re: [PATCH] Implement '--include-externals' option to 'svn list'
>
> On Tuesday 13 November 2012 03:02 AM, Stefan Sperling wrote:
> >> Attached the updated patch and log message.
> >
> >> + /* Notify that we're about to handle an external. */
> >> + SVN_ERR(list_func(baton, NULL, NULL, NULL, NULL,
> >> + externals_parent_url,
> >> + item->target_dir, iterpool));
> >
> > The docstring of svn_client_list_func2_t doesn't say if it is valid
> > for path or dirent to be NULL.
> >
> > However, you're passing NULL for these parameters before listing the
> > external. Do we really need these two extra list_func() calls before
> > and after listing the external? I was expecting them to go away.
>
>
> If we are removing these list_func() calls, how can we pass the
> arguments external_parent_url and external_target to the callback
> function? Is there any way to pass those arguments via svn_client_list3()?
Can't you just add the new argument to every call to list_func() that
applies to an external?
That would allow removing the initial call, while the callee always has the
information it needs.
Bert
Re: [PATCH] Implement '--include-externals' option to 'svn list'
Posted by vijay <vi...@collab.net>.
On Tuesday 13 November 2012 03:02 AM, Stefan Sperling wrote:
>> Attached the updated patch and log message.
>
>> + /* Notify that we're about to handle an external. */
>> + SVN_ERR(list_func(baton, NULL, NULL, NULL, NULL,
>> + externals_parent_url,
>> + item->target_dir, iterpool));
>
> The docstring of svn_client_list_func2_t doesn't say if it is valid
> for path or dirent to be NULL.
>
> However, you're passing NULL for these parameters before listing the
> external. Do we really need these two extra list_func() calls before
> and after listing the external? I was expecting them to go away.
If we are removing these list_func() calls, how can we pass the
arguments external_parent_url and external_target to the callback
function? Is there any way to pass those arguments via svn_client_list3()?
>
>> +
>> + /* List the external */
>> + SVN_ERR(wrap_external_error(ctx, item->target_dir,
>> + svn_client_list3(resolved_url,
>> + &item->peg_revision,
>> + &item->revision,
>> + depth, dirent_fields,
>> + fetch_locks,
>> + TRUE,
>> + list_func, baton, ctx,
>> + iterpool),
>> + iterpool));
>> +
>> + /* Notify that we are done with external handling. It is helpful
>> + when list is run in xml mode. */
>> + SVN_ERR(list_func(baton, NULL, NULL, NULL, NULL,
>> + externals_parent_url,
>> + item->target_dir, iterpool));
>
>
>> -/* This implements the svn_client_list_func_t API, printing a single dirent
>> +/* This implements the svn_client_list_func2_t API, printing a single dirent
>> in XML format. */
>> static svn_error_t *
>> print_dirent_xml(void *baton,
>> @@ -141,16 +174,46 @@
>> const svn_dirent_t *dirent,
>> const svn_lock_t *lock,
>> const char *abs_path,
>> - apr_pool_t *pool)
>> + const char *external_parent_url,
>> + const char *external_target,
>> + apr_pool_t *scratch_pool)
>> {
>
> Renaming 'pool' to 'scratch_pool' here is correct, but it causes
> too many unrelated changes in this patch for my taste.
> I'd prefer to apply this change in a separate patch.
>
I will send a separate patch for this change.
Thanks & Regards,
Vijayaguru
Re: [PATCH] Implement '--include-externals' option to 'svn list'
Posted by Neels Hofmeyr <ne...@elego.de>.
On Wed, 28 Nov 2012 11:27:54 +0530
vijay <vi...@collab.net> wrote:
> Thanks everyone! It was a great learning opportunity for me!
Come back anytime for more learning opportunities!
I've had committer access for four years now and am still on a
learning curve... :)
~Neels
Re: [PATCH] Implement '--include-externals' option to 'svn list'
Posted by vijay <vi...@collab.net>.
On Tuesday 27 November 2012 11:52 PM, Julian Foad wrote:
> vijay <vi...@collab.net> wrote:
>
>> This updated patch has all the changes.
>>
>> I have removed some redundant portion of code in print_dirent() and
>> print_dirent_xml() in svn/list-cmd.c.
>
> Thanks Vijay. That looks great. I've committed it in r1414304. Nice log message by the way.
>
Thanks Julian.
Thanks everyone! It was a great learning opportunity for me!
Thanks & Regards,
Vijayaguru
Re: [PATCH] Implement '--include-externals' option to 'svn list'
Posted by Julian Foad <ju...@btopenworld.com>.
vijay <vi...@collab.net> wrote:
> This updated patch has all the changes.
>
> I have removed some redundant portion of code in print_dirent() and
> print_dirent_xml() in svn/list-cmd.c.
Thanks Vijay. That looks great. I've committed it in r1414304. Nice log message by the way.
- Julian
Re: [PATCH] Implement '--include-externals' option to 'svn list'
Posted by vijay <vi...@collab.net>.
Thanks Julian.
This updated patch has all the changes.
I have removed some redundant portion of code in print_dirent() and
print_dirent_xml() in svn/list-cmd.c.
Thanks & Regards,
Vijayaguru
On Tuesday 27 November 2012 03:19 AM, Julian Foad wrote:
> vijay <vi...@collab.net> wrote:
>
>> On Tuesday 13 November 2012 03:02 AM, Stefan Sperling wrote:
>> The two extra list_func() calls has gone away!
>>
>> Attaching the updated patch that addresses your review comments.
>
> Hi Vijay. I read through the patch and it looks good, I just have a few minor comments:
>
>> Index: subversion/include/svn_client.h
>> ===================================================================
>> - * @since New in 1.4.
>> + * If svn_client_list3() was called with @a include_externals set to TRUE,
>> + * @a external_parent_url and @a external_target will be set.
>> + * @a external_parent_url is url of the directory which has the
>> + * externals definitions. @a external_target is the target subdirectory of
>> + * externals definitions.
>
> Is @a external_target an abspath, or a 'relpath' relative to @a path, or relative to the root of the entire 'list' operation, or what?
>
>> + *
>> + * If external_parent_url and external_target are defined, the item being
>> + * listed is part of the external described by external_parent_url and
>> + * external_target. Else, the item is not part of any external.
>> + * Moreover, we will never mix items which are part of separate
>> + * externals, and will always finish listing an external before listing
>> + * the next one.
>> +
>> + * @a pool may be used for temporary allocations.
>> + *
>> + * @since New in 1.8.
>> */
>> +typedef svn_error_t *(*svn_client_list_func2_t)(
>
>> Index: subversion/libsvn_client/deprecated.c
>> ===================================================================
>> +
>> +static void
>
> Please put a doc string on each new function. It can be brief for a simple function such as this.
>
>> +wrap_list_func(svn_client_list_func2_t *list_func2,
>> + void **list_func2_baton,
>> + svn_client_list_func_t list_func,
>> + void *baton,
>> + apr_pool_t *scratch_pool)
>> +{
>> + struct list_func_wrapper_baton *lfwb = apr_palloc(scratch_pool,
>> + sizeof(*lfwb));
>
> This is allocating memory that will be returned as the result of this function, so the pool shouldn't be 'scratch_pool' but 'result_pool'.
>
>> +
>> + /* Set the user provided old format callback in the baton. */
>> + lfwb->list_func1_baton = baton;
>> + lfwb->list_func1 = list_func;
>> +
>> + *list_func2_baton = lfwb;
>> + *list_func2 = list_func_wrapper;
>> +}
>
>> Index: subversion/libsvn_client/client.h
>> ===================================================================
>> +/* List external items defined on each external in EXTERNALS, a const char *
>> + externals_parent_url(url of the directory which has the externals
>> + definitions) of all externals mapping to the const char * externals_desc
>
> The implementation treats the hash values as 'svn_string_t *' not 'const char *'.
>
>> + (externals description text). All other options are the same as those
>> + passed to svn_client_list(). */
>> +svn_error_t *
>> +svn_client__list_externals(apr_hash_t *externals,
>> + svn_depth_t depth,
>> + apr_uint32_t dirent_fields,
>> + svn_boolean_t fetch_locks,
>> + svn_client_list_func2_t list_func,
>> + void *baton,
>> + svn_client_ctx_t *ctx,
>> + apr_pool_t *scratch_pool);
>
>> Index: subversion/libsvn_client/externals.c
>> ===================================================================
>> +svn_error_t *
>> +svn_client__list_externals(apr_hash_t *externals,
>> + svn_depth_t depth,
>> + apr_uint32_t dirent_fields,
>> + svn_boolean_t fetch_locks,
>> + svn_client_list_func2_t list_func,
>> + void *baton,
>> + svn_client_ctx_t *ctx,
>> + apr_pool_t *scratch_pool)
>> +{
>> + apr_pool_t *iterpool = svn_pool_create(scratch_pool);
>> + apr_hash_index_t *hi;
>> +
>> + for (hi = apr_hash_first(scratch_pool, externals);
>> + hi;
>> + hi = apr_hash_next(hi))
>> + {
>> + const char *externals_parent_url = svn__apr_hash_index_key(hi);
>> + svn_string_t *externals_desc = svn__apr_hash_index_val(hi);
>> + apr_array_header_t *external_items;
>> +
>> + svn_pool_clear(iterpool);
>> +
>> + external_items = apr_array_make(iterpool, 1,
>> + sizeof(svn_wc_external_item2_t*));
>
> There's no need to initialize 'external_items', as the first parameter of svn_wc_parse_externals_description3() is a pure output parameter. That is, the function creates a new array.
>
>> + SVN_ERR(svn_wc_parse_externals_description3(&external_items,
>> + externals_parent_url,
>> + externals_desc->data,
>> + FALSE, iterpool));
>> +
>> + if (! external_items->nelts)
>> + continue;
>> +
>> + SVN_ERR(list_external_items(external_items, externals_parent_url, depth,
>> + dirent_fields, fetch_locks, list_func,
>> + baton, ctx, iterpool));
>> +
>> + }
>> + svn_pool_destroy(iterpool);
>> +
>> + return SVN_NO_ERROR;
>> +}
>
>> Index: subversion/svn/main.c
>> ===================================================================
>> {"include-externals", opt_include_externals, 0,
>> - N_("Also commit file and dir externals reached by\n"
>> - " "
>> - "recursion. This does not include externals with a\n"
>> - " "
>> - "fixed revision. (See the svn:externals property)")},
>> + N_("include externals definitions")},
>
> That change loses information that was previously shown for the 'commit' command. We have a way to display a different description text for the same option for a specific subcommand. It's done by adding a bit at the end of the subcommand definition -- see the lines starting with '{{' in main.c.
>
> - Julian
>
Re: [PATCH] Implement '--include-externals' option to 'svn list'
Posted by Julian Foad <ju...@btopenworld.com>.
vijay <vi...@collab.net> wrote:
> On Tuesday 13 November 2012 03:02 AM, Stefan Sperling wrote:
> The two extra list_func() calls has gone away!
>
> Attaching the updated patch that addresses your review comments.
Hi Vijay. I read through the patch and it looks good, I just have a few minor comments:
> Index: subversion/include/svn_client.h
> ===================================================================
> - * @since New in 1.4.
> + * If svn_client_list3() was called with @a include_externals set to TRUE,
> + * @a external_parent_url and @a external_target will be set.
> + * @a external_parent_url is url of the directory which has the
> + * externals definitions. @a external_target is the target subdirectory of
> + * externals definitions.
Is @a external_target an abspath, or a 'relpath' relative to @a path, or relative to the root of the entire 'list' operation, or what?
> + *
> + * If external_parent_url and external_target are defined, the item being
> + * listed is part of the external described by external_parent_url and
> + * external_target. Else, the item is not part of any external.
> + * Moreover, we will never mix items which are part of separate
> + * externals, and will always finish listing an external before listing
> + * the next one.
> +
> + * @a pool may be used for temporary allocations.
> + *
> + * @since New in 1.8.
> */
> +typedef svn_error_t *(*svn_client_list_func2_t)(
> Index: subversion/libsvn_client/deprecated.c
> ===================================================================
> +
> +static void
Please put a doc string on each new function. It can be brief for a simple function such as this.
> +wrap_list_func(svn_client_list_func2_t *list_func2,
> + void **list_func2_baton,
> + svn_client_list_func_t list_func,
> + void *baton,
> + apr_pool_t *scratch_pool)
> +{
> + struct list_func_wrapper_baton *lfwb = apr_palloc(scratch_pool,
> + sizeof(*lfwb));
This is allocating memory that will be returned as the result of this function, so the pool shouldn't be 'scratch_pool' but 'result_pool'.
> +
> + /* Set the user provided old format callback in the baton. */
> + lfwb->list_func1_baton = baton;
> + lfwb->list_func1 = list_func;
> +
> + *list_func2_baton = lfwb;
> + *list_func2 = list_func_wrapper;
> +}
> Index: subversion/libsvn_client/client.h
> ===================================================================
> +/* List external items defined on each external in EXTERNALS, a const char *
> + externals_parent_url(url of the directory which has the externals
> + definitions) of all externals mapping to the const char * externals_desc
The implementation treats the hash values as 'svn_string_t *' not 'const char *'.
> + (externals description text). All other options are the same as those
> + passed to svn_client_list(). */
> +svn_error_t *
> +svn_client__list_externals(apr_hash_t *externals,
> + svn_depth_t depth,
> + apr_uint32_t dirent_fields,
> + svn_boolean_t fetch_locks,
> + svn_client_list_func2_t list_func,
> + void *baton,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *scratch_pool);
> Index: subversion/libsvn_client/externals.c
> ===================================================================
> +svn_error_t *
> +svn_client__list_externals(apr_hash_t *externals,
> + svn_depth_t depth,
> + apr_uint32_t dirent_fields,
> + svn_boolean_t fetch_locks,
> + svn_client_list_func2_t list_func,
> + void *baton,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *scratch_pool)
> +{
> + apr_pool_t *iterpool = svn_pool_create(scratch_pool);
> + apr_hash_index_t *hi;
> +
> + for (hi = apr_hash_first(scratch_pool, externals);
> + hi;
> + hi = apr_hash_next(hi))
> + {
> + const char *externals_parent_url = svn__apr_hash_index_key(hi);
> + svn_string_t *externals_desc = svn__apr_hash_index_val(hi);
> + apr_array_header_t *external_items;
> +
> + svn_pool_clear(iterpool);
> +
> + external_items = apr_array_make(iterpool, 1,
> + sizeof(svn_wc_external_item2_t*));
There's no need to initialize 'external_items', as the first parameter of svn_wc_parse_externals_description3() is a pure output parameter. That is, the function creates a new array.
> + SVN_ERR(svn_wc_parse_externals_description3(&external_items,
> + externals_parent_url,
> + externals_desc->data,
> + FALSE, iterpool));
> +
> + if (! external_items->nelts)
> + continue;
> +
> + SVN_ERR(list_external_items(external_items, externals_parent_url, depth,
> + dirent_fields, fetch_locks, list_func,
> + baton, ctx, iterpool));
> +
> + }
> + svn_pool_destroy(iterpool);
> +
> + return SVN_NO_ERROR;
> +}
> Index: subversion/svn/main.c
> ===================================================================
> {"include-externals", opt_include_externals, 0,
> - N_("Also commit file and dir externals reached by\n"
> - " "
> - "recursion. This does not include externals with a\n"
> - " "
> - "fixed revision. (See the svn:externals property)")},
> + N_("include externals definitions")},
That change loses information that was previously shown for the 'commit' command. We have a way to display a different description text for the same option for a specific subcommand. It's done by adding a bit at the end of the subcommand definition -- see the lines starting with '{{' in main.c.
- Julian
Re: [PATCH] Implement '--include-externals' option to 'svn list'
Posted by vijay <vi...@collab.net>.
On Tuesday 13 November 2012 03:02 AM, Stefan Sperling wrote:
>> Attached the updated patch and log message.
>
>> + /* Notify that we're about to handle an external. */
>> + SVN_ERR(list_func(baton, NULL, NULL, NULL, NULL,
>> + externals_parent_url,
>> + item->target_dir, iterpool));
>
> The docstring of svn_client_list_func2_t doesn't say if it is valid
> for path or dirent to be NULL.
>
> However, you're passing NULL for these parameters before listing the
> external. Do we really need these two extra list_func() calls before
> and after listing the external? I was expecting them to go away.
>
The two extra list_func() calls has gone away!
Attaching the updated patch that addresses your review comments.
Thanks & Regards,
Vijayaguru
Re: [PATCH] Implement '--include-externals' option to 'svn list'
Posted by Stefan Sperling <st...@elego.de>.
On Sat, Nov 10, 2012 at 07:11:02PM +0530, vijay wrote:
> I have removed the two boolean parameters notify_external_start and
> notify_external_end from svn_client_list_func2 implementation. Now,
> the callback keeps track of last seen external information and print
> details based on the change. I have updated the docstring of
> svn_client_list_func2_t also.
Thanks!
> Attached the updated patch and log message.
> + /* Notify that we're about to handle an external. */
> + SVN_ERR(list_func(baton, NULL, NULL, NULL, NULL,
> + externals_parent_url,
> + item->target_dir, iterpool));
The docstring of svn_client_list_func2_t doesn't say if it is valid
for path or dirent to be NULL.
However, you're passing NULL for these parameters before listing the
external. Do we really need these two extra list_func() calls before
and after listing the external? I was expecting them to go away.
> +
> + /* List the external */
> + SVN_ERR(wrap_external_error(ctx, item->target_dir,
> + svn_client_list3(resolved_url,
> + &item->peg_revision,
> + &item->revision,
> + depth, dirent_fields,
> + fetch_locks,
> + TRUE,
> + list_func, baton, ctx,
> + iterpool),
> + iterpool));
> +
> + /* Notify that we are done with external handling. It is helpful
> + when list is run in xml mode. */
> + SVN_ERR(list_func(baton, NULL, NULL, NULL, NULL,
> + externals_parent_url,
> + item->target_dir, iterpool));
> -/* This implements the svn_client_list_func_t API, printing a single dirent
> +/* This implements the svn_client_list_func2_t API, printing a single dirent
> in XML format. */
> static svn_error_t *
> print_dirent_xml(void *baton,
> @@ -141,16 +174,46 @@
> const svn_dirent_t *dirent,
> const svn_lock_t *lock,
> const char *abs_path,
> - apr_pool_t *pool)
> + const char *external_parent_url,
> + const char *external_target,
> + apr_pool_t *scratch_pool)
> {
Renaming 'pool' to 'scratch_pool' here is correct, but it causes
too many unrelated changes in this patch for my taste.
I'd prefer to apply this change in a separate patch.
Re: [PATCH] Implement '--include-externals' option to 'svn list'
Posted by vijay <vi...@collab.net>.
On Friday 16 November 2012 04:41 PM, Neels J Hofmeyr wrote:
> Hi Vijay,
>
> I gave that patch a spin (after all I was the one who brought
> --include-externals upon us), and found stuff. Using current trunk + your
> patch v3.
I was looking at your code 'commit --include-externals' before working
on this patch. Thanks for your detailed explanation of the issues and
shell script! ;)
>
> BTW, I'm sorry to have to drag this patch submission out; countless times I
> myself tried to just quickly fix a small externals quirk and it ended up
> sticking to my fingers for weeks. Don't give up! ;)
Sure. I will never give up!
>
> There's a small thing and a more complex one.
>
> (1) 'svn list' normally only lists the current dir level unless -R is
> supplied. So --include-externals should actually only print the externals
> dirnames, and step into them only with -R also supplied. Right?
>
> That was the small thing.
Yes. 'svn ls' will run with '--depth=immediates' by default. But I have
few questions regarding this approach.
1. what should we do for "svn ls --include-externals --depth=files" ?
Listing only file externals?
Not sure how complex it will be to implement it on top of the current patch.
2. If we are printing the externals dirnames alone, how we are going to
list the item in verbose mode? How can we get the revision number,
author, date and size for the particular external's dirname?
If we do like as follows, won't it be easy to understand?
Copycatting from my initial proposal of this feature[1],
$ svn list --include-externals
The current working copy directory and all externals(including externals
inside of externals) under cwd will be listed with depth 'immediates'.
$ svn list --depth=infinity --include-externals
The current working copy directory and all externals(including externals
inside of externals) under cwd will be listed with depth 'infinity'.
It works the same way as above for 'svn list --depth=files
--include-externals'.
>
>
> (2)
> I'm comparing
> svn list --include-externals
> with
> svn list ./external-target-dir/
>
> After removing and committing a file inside an external, there is a
> difference between above invocations:
>
> (test script also attached)
> [[[
> # prepare an external dir...
> mkdir a
> echo a > a/file
> echo b > b
> svn add a b
> svn propset svn:externals "^/a x1" .
> svn ci -mm
> svn up
>
> # remove a file and commit inside external dir
> svn rm x1/file
> svn ci -mm x1
>
> svn list --include-externals
> # a/
> # b
> # Listing external 'x1' defined on 'file:///tmp/trunk.qLP/repos':
>
> svn list x1
> # file
>
> ## ^ difference: 'list x1' still shows 'file'
>
> # comparison: 'list' behavior when no external is involved
> # a normal deleted file is still shown, until next update.
> svn rm b
> svn ci -mm
> svn list
> # a/
> # b
> ]]]
> (After the next update, all invocations don't show the deleted file anymore,
> obviously.)
>
> I expected 'list --include-externals' to act exactly like 'svn list x1/',
> but there is a slight difference. I suspect that 'svn list x1/' takes
> advantage of the WC at x1/ already present locally, and that 'svn list
> --include-externals' goes directly to the repos URL. Is that right?
I assume the following when you say, "'svn list x1/' takes
advantage of the WC at x1/ already present locally".
'svn list' will always use the repository URL even if the list target is
a working copy path. The file/directory entries under list target will
always be fetched from repository URL using svn_ra_get_dir2().
But what makes the command 'svn list x1/' to list a deleted file?
Revision and peg-revision of the WC path being listed. If revision and
peg-revision are not specified, they default to
#svn_opt_revision_working for WC targets and #svn_opt_revision_head for
URLs.
> Maybe 'svn list --include-externals' should limit to listing strictly only
> those externals currently checked out (unless the target itself is a URL).
> I.e. if the target is a working copy, traverse the WC for checked out
> externals and maybe even feed the local external WC paths as explicit
> targets to svn_client_list3() or something ... ?
But should we make 'svn list' to be WC dependent while listing externals?
[1] http://svn.haxx.se/dev/archive-2012-10/0381.shtml
Thanks & Regards,
Vijayaguru
> Good speed!
> ~Neels
>
Re: [PATCH] Implement '--include-externals' option to 'svn list'
Posted by Neels J Hofmeyr <ne...@elego.de>.
Hi Vijay,
I gave that patch a spin (after all I was the one who brought
--include-externals upon us), and found stuff. Using current trunk + your
patch v3.
BTW, I'm sorry to have to drag this patch submission out; countless times I
myself tried to just quickly fix a small externals quirk and it ended up
sticking to my fingers for weeks. Don't give up! ;)
There's a small thing and a more complex one.
(1) 'svn list' normally only lists the current dir level unless -R is
supplied. So --include-externals should actually only print the externals
dirnames, and step into them only with -R also supplied. Right?
That was the small thing.
(2)
I'm comparing
svn list --include-externals
with
svn list ./external-target-dir/
After removing and committing a file inside an external, there is a
difference between above invocations:
(test script also attached)
[[[
# prepare an external dir...
mkdir a
echo a > a/file
echo b > b
svn add a b
svn propset svn:externals "^/a x1" .
svn ci -mm
svn up
# remove a file and commit inside external dir
svn rm x1/file
svn ci -mm x1
svn list --include-externals
# a/
# b
# Listing external 'x1' defined on 'file:///tmp/trunk.qLP/repos':
svn list x1
# file
## ^ difference: 'list x1' still shows 'file'
# comparison: 'list' behavior when no external is involved
# a normal deleted file is still shown, until next update.
svn rm b
svn ci -mm
svn list
# a/
# b
]]]
(After the next update, all invocations don't show the deleted file anymore,
obviously.)
I expected 'list --include-externals' to act exactly like 'svn list x1/',
but there is a slight difference. I suspect that 'svn list x1/' takes
advantage of the WC at x1/ already present locally, and that 'svn list
--include-externals' goes directly to the repos URL. Is that right?
Maybe 'svn list --include-externals' should limit to listing strictly only
those externals currently checked out (unless the target itself is a URL).
I.e. if the target is a working copy, traverse the WC for checked out
externals and maybe even feed the local external WC paths as explicit
targets to svn_client_list3() or something ... ?
Good speed!
~Neels
Re: [PATCH] Implement '--include-externals' option to 'svn list'
Posted by vijay <vi...@collab.net>.
On Thursday 08 November 2012 04:10 PM, vijay wrote:
> On Wednesday 07 November 2012 08:19 PM, Stefan Sperling wrote:
>> On Wed, Nov 07, 2012 at 07:54:33PM +0530, vijay wrote:
>>> Thanks stefan for your detailed review.
>>>
>>> I will keep in mind all your review comments. I will correct my
>>> mistakes in future patches.
>>>
>>> Attached the updated patch and log message.
>>
>> This patch looks fine to me overall.
>>
>> I have one more additional suggestion though.
>>
>> Your patch uses boolean parameters to notify start end end of an
>> external item listing. This forces function implementors to reason
>> about all of these possibilities:
>>
>> "What do I do if...":
>>
>> 1) notify_external_start == TRUE
>> notify_external_end == TRUE
>>
>> 2) notify_external_start == TRUE
>> notify_external_start == FALSE
>>
>> 3) notify_external_end == FALSE
>> notify_external_end == TRUE
>>
>> 4) notify_external_end == FALSE
>> notify_external_end == FALSE
>>
>> All this combined with the possible values for external_parent_url and
>> external_target gives quite a few possibilities for implementors to
>> reason
>> about.
>>
>> I wonder if this interface can be simplified somehow, maybe by getting
>> rid
>> of these two boolean parameters. Instead, we could tell implementors that
>> if external_parent_url and external_target are defined, the item being
>> listed is part of the external described by external_parent_url and
>> external_target. Else, the item is not part of any external.
>>
>> It would then be the implementor's responsibility to, for instance,
>> properly open and close XML tags. To help with this, we can tell the
>> implementor that we'll never mix items which are part of separate
>> externals, and will always finish listing an external before listing
>> the next one. I think your code already guarantees this anyway, so we
>> can just make this guarantee explicit in the docstring of the
>> svn_client_list_func2_t interface.
>>
>> Then we could make the callback implementation track state to detect
>> by itself whether or not it is entering or leaving an external.
>> The callback could keep track of the last seen external_parent_url and
>> external_target pair, and open and close XML tags if they change.
>>
>> For an example of this approach, see the code in subversion/svn/notify.c
>> which uses an 'in_external' boolean parameter in the notify_baton for
>> a similar purpose.
>>
>> Does this suggestion make sense?
>>
>
> Yes, stefan. It makes sense.
>
> Initially, I was struggling hard to implement this option without the
> two boolean parameters, notify_external_start and notify_external_end.
> I couldn't find any other way to do it. So I added the two parameters
> reluctantly.
>
> Now, I have an interesting suggestion. Let me dig through svn/notify.c
> and come up with an implementation.
>
I have removed the two boolean parameters notify_external_start and
notify_external_end from svn_client_list_func2 implementation. Now, the
callback keeps track of last seen external information and print details
based on the change. I have updated the docstring of
svn_client_list_func2_t also.
Attached the updated patch and log message.
Thanks & Regards,
Vijayaguru
Re: [PATCH] Implement '--include-externals' option to 'svn list'
Posted by vijay <vi...@collab.net>.
On Wednesday 07 November 2012 08:19 PM, Stefan Sperling wrote:
> On Wed, Nov 07, 2012 at 07:54:33PM +0530, vijay wrote:
>> Thanks stefan for your detailed review.
>>
>> I will keep in mind all your review comments. I will correct my
>> mistakes in future patches.
>>
>> Attached the updated patch and log message.
>
> This patch looks fine to me overall.
>
> I have one more additional suggestion though.
>
> Your patch uses boolean parameters to notify start end end of an
> external item listing. This forces function implementors to reason
> about all of these possibilities:
>
> "What do I do if...":
>
> 1) notify_external_start == TRUE
> notify_external_end == TRUE
>
> 2) notify_external_start == TRUE
> notify_external_start == FALSE
>
> 3) notify_external_end == FALSE
> notify_external_end == TRUE
>
> 4) notify_external_end == FALSE
> notify_external_end == FALSE
>
> All this combined with the possible values for external_parent_url and
> external_target gives quite a few possibilities for implementors to reason
> about.
>
> I wonder if this interface can be simplified somehow, maybe by getting rid
> of these two boolean parameters. Instead, we could tell implementors that
> if external_parent_url and external_target are defined, the item being
> listed is part of the external described by external_parent_url and
> external_target. Else, the item is not part of any external.
>
> It would then be the implementor's responsibility to, for instance,
> properly open and close XML tags. To help with this, we can tell the
> implementor that we'll never mix items which are part of separate
> externals, and will always finish listing an external before listing
> the next one. I think your code already guarantees this anyway, so we
> can just make this guarantee explicit in the docstring of the
> svn_client_list_func2_t interface.
>
> Then we could make the callback implementation track state to detect
> by itself whether or not it is entering or leaving an external.
> The callback could keep track of the last seen external_parent_url and
> external_target pair, and open and close XML tags if they change.
>
> For an example of this approach, see the code in subversion/svn/notify.c
> which uses an 'in_external' boolean parameter in the notify_baton for
> a similar purpose.
>
> Does this suggestion make sense?
>
Yes, stefan. It makes sense.
Initially, I was struggling hard to implement this option without the
two boolean parameters, notify_external_start and notify_external_end.
I couldn't find any other way to do it. So I added the two parameters
reluctantly.
Now, I have an interesting suggestion. Let me dig through svn/notify.c
and come up with an implementation.
Thanks & Regards,
Vijayaguru
Re: [PATCH] Implement '--include-externals' option to 'svn list'
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Nov 07, 2012 at 07:54:33PM +0530, vijay wrote:
> Thanks stefan for your detailed review.
>
> I will keep in mind all your review comments. I will correct my
> mistakes in future patches.
>
> Attached the updated patch and log message.
This patch looks fine to me overall.
I have one more additional suggestion though.
Your patch uses boolean parameters to notify start end end of an
external item listing. This forces function implementors to reason
about all of these possibilities:
"What do I do if...":
1) notify_external_start == TRUE
notify_external_end == TRUE
2) notify_external_start == TRUE
notify_external_start == FALSE
3) notify_external_end == FALSE
notify_external_end == TRUE
4) notify_external_end == FALSE
notify_external_end == FALSE
All this combined with the possible values for external_parent_url and
external_target gives quite a few possibilities for implementors to reason
about.
I wonder if this interface can be simplified somehow, maybe by getting rid
of these two boolean parameters. Instead, we could tell implementors that
if external_parent_url and external_target are defined, the item being
listed is part of the external described by external_parent_url and
external_target. Else, the item is not part of any external.
It would then be the implementor's responsibility to, for instance,
properly open and close XML tags. To help with this, we can tell the
implementor that we'll never mix items which are part of separate
externals, and will always finish listing an external before listing
the next one. I think your code already guarantees this anyway, so we
can just make this guarantee explicit in the docstring of the
svn_client_list_func2_t interface.
Then we could make the callback implementation track state to detect
by itself whether or not it is entering or leaving an external.
The callback could keep track of the last seen external_parent_url and
external_target pair, and open and close XML tags if they change.
For an example of this approach, see the code in subversion/svn/notify.c
which uses an 'in_external' boolean parameter in the notify_baton for
a similar purpose.
Does this suggestion make sense?
Re: [PATCH] Implement '--include-externals' option to 'svn list'
Posted by vijay <vi...@collab.net>.
Thanks stefan for your detailed review.
I will keep in mind all your review comments. I will correct my mistakes
in future patches.
Attached the updated patch and log message.
Thanks & Regards,
Vijayaguru
On Tuesday 06 November 2012 07:31 PM, Stefan Sperling wrote:
> On Mon, Nov 05, 2012 at 09:54:56PM +0530, vijay 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.
>
> My review comments are below.
>
>> Index: subversion/include/svn_client.h
>> ===================================================================
>> --- subversion/include/svn_client.h (revision 1405691)
>> +++ subversion/include/svn_client.h (working copy)
>
>> @@ -5290,11 +5290,41 @@
>> * the entry's lock, if it is locked and if lock information is being
>> * reported by the caller; otherwise @a lock is NULL. @a abs_path is the
>> * repository path of the top node of the list operation; it is relative to
>> - * the repository root and begins with "/". @a pool may be used for
>> - * temporary allocations.
>> + * the repository root and begins with "/".
>> *
>> - * @since New in 1.4.
>> + * If svn_client_list3() was called with @a include_externals set to TRUE,
>> + * @a notify_external_start, @a notify_external_end, @a external_parent_url
>> + * and @a external_target will be set. @a notify_external_start and
>> + * @a notify_external_end is used to control the list output of externals.
>> + * @a external_parent_url is url of the directory which has the externals
>> + * definitions. @a external_target is the target subdirectory of externals
>> + * definitions.
>
> The purpose of the notify_external_start/end booleans isn't clear at first
> sight. The docstring doesn't explain why these flags are necessary.
> Later in the patch, in a code comment you explain that you did this to
> more easily support XML listing mode. Maybe mention this here, too?
>
>> +
>> + * @a pool may be used for temporary allocations.
>
> Please call this 'pool' parameter scratch_pool.
>
>> Index: subversion/libsvn_client/client.h
>> ===================================================================
>> --- subversion/libsvn_client/client.h (revision 1405691)
>> +++ subversion/libsvn_client/client.h (working copy)
>> @@ -1009,7 +1009,6 @@
>> svn_client_ctx_t *ctx,
>> apr_pool_t *pool);
>>
>> -
>
> This is an unnecessary whitespace change.
>
>> /* Perform status operations on each external in EXTERNAL_MAP, a const char *
>> local_abspath of all externals mapping to the const char* defining_abspath.
>> All other options are the same as those passed to svn_client_status(). */
>> @@ -1024,6 +1023,21 @@
>> void *status_baton,
>> apr_pool_t *pool);
>>
>> +/* List external items defined on each external in EXTERNALS, a const char *
>> + externals_parent_url(url of the directory which has the externals
>> + definitions) of all externals mapping to the const char * externals_desc
>> + (externals description text). All other options are the same as those
>> + passed to svn_client_list(). */
>> +svn_error_t *
>> +svn_client__list_externals(apr_hash_t *externals,
>> + svn_depth_t depth,
>> + apr_uint32_t dirent_fields,
>> + svn_boolean_t fetch_locks,
>> + svn_client_list_func2_t list_func,
>> + void *baton,
>> + svn_client_ctx_t *ctx,
>> + apr_pool_t *pool);
>
> Please call this 'pool' parameter scratch_pool.
>
>> Index: subversion/libsvn_client/deprecated.c
>> ===================================================================
>> --- subversion/libsvn_client/deprecated.c (revision 1405691)
>> +++ subversion/libsvn_client/deprecated.c (working copy)
>> @@ -1269,7 +1269,75 @@
>> }
>>
>> /*** From list.c ***/
>> +
>> +/* Baton for use with wrap_list_func */
>> +struct list_func_wrapper_baton {
>> + void *baton;
>> + svn_client_list_func_t list_func;
>
> I'd suggest using the following names instead, for clarity:
>
> svn_client_list_func_t list_func1;
> void *list_func1_baton;
>
>> +};
>> +
>> +/* This implements svn_client_list_func2_t */
>> +static svn_error_t *
>> +list_func_wrapper(void *baton,
>> + const char *path,
>> + const svn_dirent_t *dirent,
>> + const svn_lock_t *lock,
>> + const char *abs_path,
>> + svn_boolean_t notify_external_start,
>> + svn_boolean_t notify_external_end,
>> + const char *external_parent_url,
>> + const char *external_target,
>> + apr_pool_t *pool)
>
> Again, this is a scratch pool so it should be called scratch_pool.
>
>> +static void
>> +wrap_list_func(svn_client_list_func2_t *list_func2,
>> + void **list_func2_baton,
>> + svn_client_list_func_t list_func,
>> + void *baton,
>> + apr_pool_t *pool)
>
> scratch_pool
>
>> Index: subversion/libsvn_client/externals.c
>> ===================================================================
>> --- subversion/libsvn_client/externals.c (revision 1405691)
>> +++ subversion/libsvn_client/externals.c (working copy)
>> @@ -1180,3 +1180,95 @@
>> return SVN_NO_ERROR;
>> }
>>
>> +
>> +svn_error_t *
>> +svn_client__list_externals(apr_hash_t *externals,
>> + svn_depth_t depth,
>> + apr_uint32_t dirent_fields,
>> + svn_boolean_t fetch_locks,
>> + svn_client_list_func2_t list_func,
>> + void *baton,
>> + svn_client_ctx_t *ctx,
>> + apr_pool_t *pool)
>
> scratch_pool
>
>> +{
>> + apr_pool_t *iterpool = svn_pool_create(pool);
>> + apr_pool_t *sub_iterpool = svn_pool_create(pool);
>
> Perhaps avoid having two iterpools in the same function by moving the
> inner loop which uses the second iterpool into a separate helper function?
>
>> + apr_hash_index_t *hi;
>> +
>> + for (hi = apr_hash_first(pool, externals);
>> + hi;
>> + hi = apr_hash_next(hi))
>> + {
>> + const char *externals_parent_url = svn__apr_hash_index_key(hi);
>> + svn_string_t *externals_desc = svn__apr_hash_index_val(hi);
>> + const char *externals_parent_repos_root_url;
>> + apr_array_header_t *items;
>> + int i;
>> +
>> + svn_pool_clear(iterpool);
>> +
>> + items = apr_array_make(iterpool, 1, sizeof(svn_wc_external_item2_t*));
>> +
>> + SVN_ERR(svn_client_get_repos_root(&externals_parent_repos_root_url,
>> + NULL /* uuid */,
>> + externals_parent_url, ctx,
>> + iterpool, iterpool));
>> +
>> + SVN_ERR(svn_wc_parse_externals_description3(&items,
>> + externals_parent_url,
>> + externals_desc->data,
>> + FALSE, iterpool));
>> +
>> + if (! items->nelts)
>> + continue;
>> +
>> + for (i = 0; i < items->nelts; i++)
>> + {
>
> So I mean instead of this inner loop, have a helper function, such as:
>
> static svn_error_t *
> list_external_items(apr_array_header_t *items,
> ...,
> apr_pool_t *scratch_pool);
>
> and call it here like this:
>
> SVN_ERR(list_external_items(items, ..., iterpool));
>
> The list_external_items() function would contain this loop and
> would be free to reuse the name 'iterpool' instead of sub_iterpool.
>
>
>> + /* Notify that we're about to handle an external. */
>> + SVN_ERR(list_func(baton, NULL, NULL, NULL, NULL,
>> + TRUE /*notify_external_start */,
>> + FALSE /*notify_external_end */,
>> + externals_parent_url,
>> + item->target_dir, iterpool));
>
> Looks like you're accidentally not using the sub_iterpool here ;)
> A helper function would avoid this kind of problem.
>
>> +
>> + /* List the external */
>> + SVN_ERR(wrap_external_error(
>> + ctx, item->target_dir,
>
> Please format the above two lines as a single line:
>
> SVN_ERR(wrap_external_error(ctx, item->target_dir,
>
>> + svn_client_list3(resolved_url,
>> + &item->peg_revision,
>> + &item->revision,
>> + depth, dirent_fields,
>> + fetch_locks,
>> + TRUE,
>> + list_func, baton, ctx,
>> + sub_iterpool),
>> + sub_iterpool));
>> +
>> + /* Notify that we are done with external handling. It is helpful
>> + when list is run in xml mode. */
>> + SVN_ERR(list_func(baton, NULL, NULL, NULL, NULL,
>> + FALSE /*notify_external_start */,
>> + TRUE /*notify_external_end */,
>> + NULL, NULL, iterpool));
>
> Again, wrong iterpool?
>
>> Index: subversion/libsvn_client/list.c
>> ===================================================================
>> --- subversion/libsvn_client/list.c (revision 1405691)
>> +++ subversion/libsvn_client/list.c (working copy)
>
>> @@ -82,12 +94,27 @@
>> return SVN_NO_ERROR;
>> }
>> SVN_ERR(err);
>> +
>> + if (prop_hash
>> + && (prop_val = apr_hash_get(prop_hash, SVN_PROP_EXTERNALS,
>> + APR_HASH_KEY_STRING)))
>
> Please don't ever put assignments into 'if' statements.
> Many programmers will expect == instead of = within if (...) and will
> always double-check every occurance of =, because mistyping the ==
> operator as = is a common C programmming error. The code you wrote
> isn't wrong, it's just bad style.
>
> I'd suggest to write this code as follows, for clarity:
>
> if (prop_hash)
> prop_val = apr_hash_get(prop_hash, SVN_PROP_EXTERNALS,
> APR_HASH_KEY_STRING);
> else
> prop_val = NULL;
>
> if (prop_val)
>> + {
>> + const char *url;
>>
>> + SVN_ERR(svn_ra_get_session_url(ra_session, &url, scratch_pool));
>> +
>> + apr_hash_set(*externals, svn_path_url_add_component2(url, dir,
>> + result_pool),
>> + APR_HASH_KEY_STRING, svn_string_dup(prop_val,
>> + result_pool));
>> + }
>
> You never call apr_hash_make() within this function so there is no point
> in passing an apr_hash_t ** into it.
>
> The caller creates the externals hash table. So the caller can pass the
> externals hash table as apr_hash_t *, not apr_hash_t **.
>
> You can get rid of the boolean include_externals parameter which is
> implied anyway by a non-NULL externals hash passed by the caller:
> externals == NULL means 'don't list externals'
> externals != NULL means 'add externals to the hash if any'
>
>> @@ -224,14 +256,16 @@
>> return SVN_NO_ERROR;
>> }
>>
>> +
>> svn_error_t *
>> -svn_client_list2(const char *path_or_url,
>> +svn_client_list3(const char *path_or_url,
>> const svn_opt_revision_t *peg_revision,
>> const svn_opt_revision_t *revision,
>> svn_depth_t depth,
>> apr_uint32_t dirent_fields,
>> svn_boolean_t fetch_locks,
>> - svn_client_list_func_t list_func,
>> + svn_boolean_t include_externals,
>> + svn_client_list_func2_t list_func,
>> void *baton,
>> svn_client_ctx_t *ctx,
>> apr_pool_t *pool)
>> @@ -242,6 +276,7 @@
>> const char *fs_path;
>> svn_error_t *err;
>> apr_hash_t *locks;
>> + apr_hash_t *externals = apr_hash_make(pool);
>
> So with my above idea, you'd initialise externals like this:
>
> apr_hash_t *externals;
>
> if (include_externals)
> externals = apr_hash_make(pool);
> else
> externals = NULL;
>
>> @@ -284,14 +319,29 @@
>
>> + /* We handle externals after listing entries under path_or_url, so that
>> + handling external items (and any errors therefrom) doesn't delay
>> + the primary operation. */
>> + if (include_externals && externals && apr_hash_count(externals))
>
> If include_externals is TRUE, then externals is never NULL, so you can
> write the above line as:
>
> if (include_externals && apr_hash_count(externals))
>
>> Index: subversion/svn/list-cmd.c
>> ===================================================================
>> --- subversion/svn/list-cmd.c (revision 1405691)
>> +++ subversion/svn/list-cmd.c (working copy)
>
>> @@ -52,6 +52,10 @@
>> const svn_dirent_t *dirent,
>> const svn_lock_t *lock,
>> const char *abs_path,
>> + svn_boolean_t notify_external_start,
>> + svn_boolean_t notify_external_end,
>> + const char *external_parent_url,
>> + const char *external_target,
>> apr_pool_t *pool)
>> {
>> struct print_baton *pb = baton;
>
> You should probably make the svn_client_list_func2_t API promise that
> external_parent_url and external_target must either both be non-NULL
> or both be NULL. So that the following assert won't trigger:
>
> SVN_ERR_ASSERT((external_parent_url && external_target) ||
> (external_parent_url == NULL && external_target == NULL));
>
>> @@ -59,6 +63,16 @@
>> static const char *time_format_long = NULL;
>> static const char *time_format_short = NULL;
>>
>> + if (notify_external_start)
>> + {
>> + return svn_cmdline_printf(pool, "Externals on '%s - %s':\n",
>> + external_parent_url,
>> + external_target);
>> + }
>> +
>> + if (notify_external_end)
>> + return SVN_NO_ERROR;
>> +
>
> It seems this is listing one external target, not many.
> So the message you are printing is misleading because it says "Externals"
> rather than "External", even if only one external target is defined.
>
> You also forgot to mark the "Externals on..." message for translation
> with the _() macro.
>
> When returning directly with a function call that returns svn_error_t *,
> never write 'return func()'. You should either write
> 'return svn_error_trace(func());'
> or use 'SVN_ERR(func());' followed by 'return SVN_NO_ERROR;' on the next line.
>
> So taking all that into account, I would suggest something like:
>
> if (notify_external_start)
> {
> SVN_ERR(svn_cmdline_printf(pool,
> _("Listing external '%s' defined on '%s':\n"),
> external_target,
> external_parent_url));
> return SVN_NO_ERROR;
> }
>
>
> Looks good to me otherwise, thanks!
>
Re: [PATCH] Implement '--include-externals' option to 'svn list'
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Nov 05, 2012 at 09:54:56PM +0530, vijay 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.
My review comments are below.
> Index: subversion/include/svn_client.h
> ===================================================================
> --- subversion/include/svn_client.h (revision 1405691)
> +++ subversion/include/svn_client.h (working copy)
> @@ -5290,11 +5290,41 @@
> * the entry's lock, if it is locked and if lock information is being
> * reported by the caller; otherwise @a lock is NULL. @a abs_path is the
> * repository path of the top node of the list operation; it is relative to
> - * the repository root and begins with "/". @a pool may be used for
> - * temporary allocations.
> + * the repository root and begins with "/".
> *
> - * @since New in 1.4.
> + * If svn_client_list3() was called with @a include_externals set to TRUE,
> + * @a notify_external_start, @a notify_external_end, @a external_parent_url
> + * and @a external_target will be set. @a notify_external_start and
> + * @a notify_external_end is used to control the list output of externals.
> + * @a external_parent_url is url of the directory which has the externals
> + * definitions. @a external_target is the target subdirectory of externals
> + * definitions.
The purpose of the notify_external_start/end booleans isn't clear at first
sight. The docstring doesn't explain why these flags are necessary.
Later in the patch, in a code comment you explain that you did this to
more easily support XML listing mode. Maybe mention this here, too?
> +
> + * @a pool may be used for temporary allocations.
Please call this 'pool' parameter scratch_pool.
> Index: subversion/libsvn_client/client.h
> ===================================================================
> --- subversion/libsvn_client/client.h (revision 1405691)
> +++ subversion/libsvn_client/client.h (working copy)
> @@ -1009,7 +1009,6 @@
> svn_client_ctx_t *ctx,
> apr_pool_t *pool);
>
> -
This is an unnecessary whitespace change.
> /* Perform status operations on each external in EXTERNAL_MAP, a const char *
> local_abspath of all externals mapping to the const char* defining_abspath.
> All other options are the same as those passed to svn_client_status(). */
> @@ -1024,6 +1023,21 @@
> void *status_baton,
> apr_pool_t *pool);
>
> +/* List external items defined on each external in EXTERNALS, a const char *
> + externals_parent_url(url of the directory which has the externals
> + definitions) of all externals mapping to the const char * externals_desc
> + (externals description text). All other options are the same as those
> + passed to svn_client_list(). */
> +svn_error_t *
> +svn_client__list_externals(apr_hash_t *externals,
> + svn_depth_t depth,
> + apr_uint32_t dirent_fields,
> + svn_boolean_t fetch_locks,
> + svn_client_list_func2_t list_func,
> + void *baton,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool);
Please call this 'pool' parameter scratch_pool.
> Index: subversion/libsvn_client/deprecated.c
> ===================================================================
> --- subversion/libsvn_client/deprecated.c (revision 1405691)
> +++ subversion/libsvn_client/deprecated.c (working copy)
> @@ -1269,7 +1269,75 @@
> }
>
> /*** From list.c ***/
> +
> +/* Baton for use with wrap_list_func */
> +struct list_func_wrapper_baton {
> + void *baton;
> + svn_client_list_func_t list_func;
I'd suggest using the following names instead, for clarity:
svn_client_list_func_t list_func1;
void *list_func1_baton;
> +};
> +
> +/* This implements svn_client_list_func2_t */
> +static svn_error_t *
> +list_func_wrapper(void *baton,
> + const char *path,
> + const svn_dirent_t *dirent,
> + const svn_lock_t *lock,
> + const char *abs_path,
> + svn_boolean_t notify_external_start,
> + svn_boolean_t notify_external_end,
> + const char *external_parent_url,
> + const char *external_target,
> + apr_pool_t *pool)
Again, this is a scratch pool so it should be called scratch_pool.
> +static void
> +wrap_list_func(svn_client_list_func2_t *list_func2,
> + void **list_func2_baton,
> + svn_client_list_func_t list_func,
> + void *baton,
> + apr_pool_t *pool)
scratch_pool
> Index: subversion/libsvn_client/externals.c
> ===================================================================
> --- subversion/libsvn_client/externals.c (revision 1405691)
> +++ subversion/libsvn_client/externals.c (working copy)
> @@ -1180,3 +1180,95 @@
> return SVN_NO_ERROR;
> }
>
> +
> +svn_error_t *
> +svn_client__list_externals(apr_hash_t *externals,
> + svn_depth_t depth,
> + apr_uint32_t dirent_fields,
> + svn_boolean_t fetch_locks,
> + svn_client_list_func2_t list_func,
> + void *baton,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool)
scratch_pool
> +{
> + apr_pool_t *iterpool = svn_pool_create(pool);
> + apr_pool_t *sub_iterpool = svn_pool_create(pool);
Perhaps avoid having two iterpools in the same function by moving the
inner loop which uses the second iterpool into a separate helper function?
> + apr_hash_index_t *hi;
> +
> + for (hi = apr_hash_first(pool, externals);
> + hi;
> + hi = apr_hash_next(hi))
> + {
> + const char *externals_parent_url = svn__apr_hash_index_key(hi);
> + svn_string_t *externals_desc = svn__apr_hash_index_val(hi);
> + const char *externals_parent_repos_root_url;
> + apr_array_header_t *items;
> + int i;
> +
> + svn_pool_clear(iterpool);
> +
> + items = apr_array_make(iterpool, 1, sizeof(svn_wc_external_item2_t*));
> +
> + SVN_ERR(svn_client_get_repos_root(&externals_parent_repos_root_url,
> + NULL /* uuid */,
> + externals_parent_url, ctx,
> + iterpool, iterpool));
> +
> + SVN_ERR(svn_wc_parse_externals_description3(&items,
> + externals_parent_url,
> + externals_desc->data,
> + FALSE, iterpool));
> +
> + if (! items->nelts)
> + continue;
> +
> + for (i = 0; i < items->nelts; i++)
> + {
So I mean instead of this inner loop, have a helper function, such as:
static svn_error_t *
list_external_items(apr_array_header_t *items,
...,
apr_pool_t *scratch_pool);
and call it here like this:
SVN_ERR(list_external_items(items, ..., iterpool));
The list_external_items() function would contain this loop and
would be free to reuse the name 'iterpool' instead of sub_iterpool.
> + /* Notify that we're about to handle an external. */
> + SVN_ERR(list_func(baton, NULL, NULL, NULL, NULL,
> + TRUE /*notify_external_start */,
> + FALSE /*notify_external_end */,
> + externals_parent_url,
> + item->target_dir, iterpool));
Looks like you're accidentally not using the sub_iterpool here ;)
A helper function would avoid this kind of problem.
> +
> + /* List the external */
> + SVN_ERR(wrap_external_error(
> + ctx, item->target_dir,
Please format the above two lines as a single line:
SVN_ERR(wrap_external_error(ctx, item->target_dir,
> + svn_client_list3(resolved_url,
> + &item->peg_revision,
> + &item->revision,
> + depth, dirent_fields,
> + fetch_locks,
> + TRUE,
> + list_func, baton, ctx,
> + sub_iterpool),
> + sub_iterpool));
> +
> + /* Notify that we are done with external handling. It is helpful
> + when list is run in xml mode. */
> + SVN_ERR(list_func(baton, NULL, NULL, NULL, NULL,
> + FALSE /*notify_external_start */,
> + TRUE /*notify_external_end */,
> + NULL, NULL, iterpool));
Again, wrong iterpool?
> Index: subversion/libsvn_client/list.c
> ===================================================================
> --- subversion/libsvn_client/list.c (revision 1405691)
> +++ subversion/libsvn_client/list.c (working copy)
> @@ -82,12 +94,27 @@
> return SVN_NO_ERROR;
> }
> SVN_ERR(err);
> +
> + if (prop_hash
> + && (prop_val = apr_hash_get(prop_hash, SVN_PROP_EXTERNALS,
> + APR_HASH_KEY_STRING)))
Please don't ever put assignments into 'if' statements.
Many programmers will expect == instead of = within if (...) and will
always double-check every occurance of =, because mistyping the ==
operator as = is a common C programmming error. The code you wrote
isn't wrong, it's just bad style.
I'd suggest to write this code as follows, for clarity:
if (prop_hash)
prop_val = apr_hash_get(prop_hash, SVN_PROP_EXTERNALS,
APR_HASH_KEY_STRING);
else
prop_val = NULL;
if (prop_val)
> + {
> + const char *url;
>
> + SVN_ERR(svn_ra_get_session_url(ra_session, &url, scratch_pool));
> +
> + apr_hash_set(*externals, svn_path_url_add_component2(url, dir,
> + result_pool),
> + APR_HASH_KEY_STRING, svn_string_dup(prop_val,
> + result_pool));
> + }
You never call apr_hash_make() within this function so there is no point
in passing an apr_hash_t ** into it.
The caller creates the externals hash table. So the caller can pass the
externals hash table as apr_hash_t *, not apr_hash_t **.
You can get rid of the boolean include_externals parameter which is
implied anyway by a non-NULL externals hash passed by the caller:
externals == NULL means 'don't list externals'
externals != NULL means 'add externals to the hash if any'
> @@ -224,14 +256,16 @@
> return SVN_NO_ERROR;
> }
>
> +
> svn_error_t *
> -svn_client_list2(const char *path_or_url,
> +svn_client_list3(const char *path_or_url,
> const svn_opt_revision_t *peg_revision,
> const svn_opt_revision_t *revision,
> svn_depth_t depth,
> apr_uint32_t dirent_fields,
> svn_boolean_t fetch_locks,
> - svn_client_list_func_t list_func,
> + svn_boolean_t include_externals,
> + svn_client_list_func2_t list_func,
> void *baton,
> svn_client_ctx_t *ctx,
> apr_pool_t *pool)
> @@ -242,6 +276,7 @@
> const char *fs_path;
> svn_error_t *err;
> apr_hash_t *locks;
> + apr_hash_t *externals = apr_hash_make(pool);
So with my above idea, you'd initialise externals like this:
apr_hash_t *externals;
if (include_externals)
externals = apr_hash_make(pool);
else
externals = NULL;
> @@ -284,14 +319,29 @@
> + /* We handle externals after listing entries under path_or_url, so that
> + handling external items (and any errors therefrom) doesn't delay
> + the primary operation. */
> + if (include_externals && externals && apr_hash_count(externals))
If include_externals is TRUE, then externals is never NULL, so you can
write the above line as:
if (include_externals && apr_hash_count(externals))
> Index: subversion/svn/list-cmd.c
> ===================================================================
> --- subversion/svn/list-cmd.c (revision 1405691)
> +++ subversion/svn/list-cmd.c (working copy)
> @@ -52,6 +52,10 @@
> const svn_dirent_t *dirent,
> const svn_lock_t *lock,
> const char *abs_path,
> + svn_boolean_t notify_external_start,
> + svn_boolean_t notify_external_end,
> + const char *external_parent_url,
> + const char *external_target,
> apr_pool_t *pool)
> {
> struct print_baton *pb = baton;
You should probably make the svn_client_list_func2_t API promise that
external_parent_url and external_target must either both be non-NULL
or both be NULL. So that the following assert won't trigger:
SVN_ERR_ASSERT((external_parent_url && external_target) ||
(external_parent_url == NULL && external_target == NULL));
> @@ -59,6 +63,16 @@
> static const char *time_format_long = NULL;
> static const char *time_format_short = NULL;
>
> + if (notify_external_start)
> + {
> + return svn_cmdline_printf(pool, "Externals on '%s - %s':\n",
> + external_parent_url,
> + external_target);
> + }
> +
> + if (notify_external_end)
> + return SVN_NO_ERROR;
> +
It seems this is listing one external target, not many.
So the message you are printing is misleading because it says "Externals"
rather than "External", even if only one external target is defined.
You also forgot to mark the "Externals on..." message for translation
with the _() macro.
When returning directly with a function call that returns svn_error_t *,
never write 'return func()'. You should either write
'return svn_error_trace(func());'
or use 'SVN_ERR(func());' followed by 'return SVN_NO_ERROR;' on the next line.
So taking all that into account, I would suggest something like:
if (notify_external_start)
{
SVN_ERR(svn_cmdline_printf(pool,
_("Listing external '%s' defined on '%s':\n"),
external_target,
external_parent_url));
return SVN_NO_ERROR;
}
Looks good to me otherwise, thanks!
Re: [PATCH] Implement '--include-externals' option to 'svn list'
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Thu, Nov 8, 2012 at 12:34 PM, vijay <vi...@collab.net> wrote:
> On Wednesday 07 November 2012 08:57 PM, Stefan Fuhrmann wrote:
>
>> On Wed, Nov 7, 2012 at 3:02 PM, vijay <vijay@collab.net
>> <ma...@collab.net>> wrote:
>>
>> On Tuesday 06 November 2012 08:09 PM, Stefan Fuhrmann wrote:
>>
>> On Mon, Nov 5, 2012 at 5:24 PM, vijay <vijay@collab.net
>> <ma...@collab.net>
>> <mailto:vijay@collab.net <ma...@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>
>> <http://subversion.tigris.org/**__issues/show_bug.cgi?id=4225<http://subversion.tigris.org/__issues/show_bug.cgi?id=4225>
>> >
>>
>>
>> <http://subversion.tigris.org/**
>> __issues/show_bug.cgi?id=4225<http://subversion.tigris.org/__issues/show_bug.cgi?id=4225>
>> <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?
>>
>>
>>
>> The external_target member will not be given by path / dirent.
>> We will get it by parsing the externals description.
>>
>> Suppose that path1 in repo1 has svn:externals set to path2 in repo2.
>>
>> When we list path1 with externals included,
>>
>> 1. First, list path1.
>> 2. Then, process all the externals defined under path1. Parse
>> through the individual external description and populate
>> external_target.
>>
>> For example,
>>
>> external description under path1:
>> external_desc = exdir http://<url-of-path2-in-repo2>
>>
>> external_target = exdir
>> external_parent_url = http://<url-of-path1-in-repo1>
>>
>> url of external = http://<url-of-path2-in-repo2>
>>
>> 3. List the entries by reaching 'url of external'.
>>
>>
>> OK, I now see what you are trying to do here -
>> I had read to much into the code.
>>
>> However, in this form, the added functionality is
>> of limited use, IMHO. I understand that what you
>> list is basically which paths you would get for a
>> "svn co $url".
>>
>> While this is fine, I see two issues with it:
>>
>> * trees don't get overlayed. Example:
>> $>svn ls $URL -R
>> sub1
>> sub1/fileA
>> sub1/fileB
>> sub2
>> $>svn propget "svn:externals" $URL
>> http://somewhere/ sub1/subsub
>> $>svn ls $URL -R --include-externals
>> sub1
>> sub1/fileA
>> sub1/fileB
>> sub2
>> sub1/subsub [external].
>>
>> Desired output
>> sub1
>> sub1/fileA
>> sub1/fileB
>> sub1/subsub [external @$URL].
>> sub2
>>
>
> I was referring externals related code base in checkout/update and export
> while implementing this option.
>
> From subversion/libsvn_client/**update.c
> <snip>
> /* We handle externals after the update is complete, so that
>
> handling external items (and any errors therefrom) doesn't delay
> the primary operation. */
> </snip>
>
> So I preferred the same for 'list' also. But there is a difference in
> externals processing between the commands checkout/update and list. The
> former processes the externals by default. The latter processes externals
> only if we specifically ask it. What should we do here?
I think if the goal is to behave the same way
that export / co do, then your approach is
correct. I can live with that.
Listing externals seems to be unusual enough
to not make it the default in our CL UI. I would
expect many tools to run "svn ls" and expect
each line to contain an actual sub-folder or file.
There is no need to break that assumption by
default.
Compatibility trumps consistency in that case.
>> * result of ls on a sub-folder results in less output
>> $>svn ls $URL/sub1 -R --include-externals
>> fileA
>> fileB
>>
>> Desired output:
>> fileA
>> fileB
>> subsub [external @$URL].
>>
>
>
> Bert raised the same question and C-Mike answered here [1].
>
Thanks for the pointer.
> The current implementation uses svn_ra_get_dir() to fetch all properties
> and filters "svn:externals" under current list target. I don't know how to
> extend it for externals defined higher up in the tree.
>
If we want to follow the "do as co does" route,
reading info from higher up the tree would be
inconsistent.
In fact, if I can't get a nice in-lined handling of
externals, I would prefer the implementation to
fetch exactly the data within the given sub-tree.
Due to the reuse of ra sessions, this is faster
than two svn_client_* calls and requesting
parent info for every call would slow things down
again.
-- Stefan^2.
--
Certified & Supported Apache Subversion Downloads:
*
http://www.wandisco.com/subversion/download
*
Re: [PATCH] Implement '--include-externals' option to 'svn list'
Posted by Neels J Hofmeyr <ne...@elego.de>.
On 2012-11-08 15:42, Stefan Sperling wrote:
> With the benefit of hindsight, it seems allowing externals to be configured
> on any ancestor directory was a big mistake in the design that complicates
> things for no good reason. But that's neither your nor my fault :)
Word! So many things are difficult just because of that. If Kirk was
Subversion, instead of "Khaaaan!!" he would scream "Externaaaaals!!"
...and then there are also the externals defined in entire ancestral
*working copies*. That is the real nightmare. Like
svn propset svn:externals "
^/src1 X1
^/src2 X1/X2" .
[...]
cd X1
svn list --include-externals
Does it show X2? No? :)
~Neels
Re: [PATCH] Implement '--include-externals' option to 'svn list'
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Nov 08, 2012 at 05:04:35PM +0530, vijay wrote:
> On Wednesday 07 November 2012 08:57 PM, Stefan Fuhrmann wrote:
> >* result of ls on a sub-folder results in less output
> > $>svn ls $URL/sub1 -R --include-externals
> > fileA
> > fileB
> >
> > Desired output:
> > fileA
> > fileB
> > subsub [external @$URL].
>
> Bert raised the same question and C-Mike answered here [1].
I think we should get something based on your current patch committed.
It is already much better than not listing externals at all.
Unless others have strong objections, of course. In that case you'll
need to rework your current patch before it can be committed.
With the benefit of hindsight, it seems allowing externals to be configured
on any ancestor directory was a big mistake in the design that complicates
things for no good reason. But that's neither your nor my fault :)
> The current implementation uses svn_ra_get_dir() to fetch all
> properties and filters "svn:externals" under current list target. I
> don't know how to extend it for externals defined higher up in the
> tree.
Before listing, you could walk the repository tree from the target upwards
to the repository root, and then while listing also process any externals
definitions found on ancestors if they affect the list target.
You'll need to deal with problems like authz restrictions while traversing
upwards. So I'd suggest working on this problem in a follow-up patch,
which should also include a new test case of course.
Your current patch is already complex enough for me to review so I wouldn't
want it to grow much larger before it can be committed.
Re: [PATCH] Implement '--include-externals' option to 'svn list'
Posted by vijay <vi...@collab.net>.
On Wednesday 07 November 2012 08:57 PM, Stefan Fuhrmann wrote:
> On Wed, Nov 7, 2012 at 3:02 PM, vijay <vijay@collab.net
> <ma...@collab.net>> wrote:
>
> On Tuesday 06 November 2012 08:09 PM, Stefan Fuhrmann wrote:
>
> On Mon, Nov 5, 2012 at 5:24 PM, vijay <vijay@collab.net
> <ma...@collab.net>
> <mailto:vijay@collab.net <ma...@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>
>
> <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?
>
>
>
> The external_target member will not be given by path / dirent.
> We will get it by parsing the externals description.
>
> Suppose that path1 in repo1 has svn:externals set to path2 in repo2.
>
> When we list path1 with externals included,
>
> 1. First, list path1.
> 2. Then, process all the externals defined under path1. Parse
> through the individual external description and populate
> external_target.
>
> For example,
>
> external description under path1:
> external_desc = exdir http://<url-of-path2-in-repo2>
>
> external_target = exdir
> external_parent_url = http://<url-of-path1-in-repo1>
>
> url of external = http://<url-of-path2-in-repo2>
>
> 3. List the entries by reaching 'url of external'.
>
>
> OK, I now see what you are trying to do here -
> I had read to much into the code.
>
> However, in this form, the added functionality is
> of limited use, IMHO. I understand that what you
> list is basically which paths you would get for a
> "svn co $url".
>
> While this is fine, I see two issues with it:
>
> * trees don't get overlayed. Example:
> $>svn ls $URL -R
> sub1
> sub1/fileA
> sub1/fileB
> sub2
> $>svn propget "svn:externals" $URL
> http://somewhere/ sub1/subsub
> $>svn ls $URL -R --include-externals
> sub1
> sub1/fileA
> sub1/fileB
> sub2
> sub1/subsub [external].
>
> Desired output
> sub1
> sub1/fileA
> sub1/fileB
> sub1/subsub [external @$URL].
> sub2
I was referring externals related code base in checkout/update and
export while implementing this option.
From subversion/libsvn_client/update.c
<snip>
/* We handle externals after the update is complete, so that
handling external items (and any errors therefrom) doesn't delay
the primary operation. */
</snip>
So I preferred the same for 'list' also. But there is a difference in
externals processing between the commands checkout/update and list. The
former processes the externals by default. The latter processes
externals only if we specifically ask it. What should we do here?
>
> * result of ls on a sub-folder results in less output
> $>svn ls $URL/sub1 -R --include-externals
> fileA
> fileB
>
> Desired output:
> fileA
> fileB
> subsub [external @$URL].
Bert raised the same question and C-Mike answered here [1].
The current implementation uses svn_ra_get_dir() to fetch all properties
and filters "svn:externals" under current list target. I don't know how
to extend it for externals defined higher up in the tree.
Thanks & Regards,
Vijayaguru
[1] http://svn.haxx.se/dev/archive-2012-10/0353.shtml
>
> So, it is hard to build e.g. explorer-like GUIs based
> on this information. The GUI would still need to collect,
> remember and "splice" the externals manually. That
> would render your extension almost useless.
>
> -- Stefan^2.
>
> --
> Certified & Supported Apache Subversion Downloads:
> /
>
> http://www.wandisco.com/subversion/download
>
> /
Re: [PATCH] Implement '--include-externals' option to 'svn list'
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Wed, Nov 7, 2012 at 3:02 PM, vijay <vi...@collab.net> wrote:
> On Tuesday 06 November 2012 08:09 PM, Stefan Fuhrmann wrote:
>
>> On Mon, Nov 5, 2012 at 5:24 PM, vijay <vijay@collab.net
>> <ma...@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>
>>
>> <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?
>>
>
>
> The external_target member will not be given by path / dirent.
> We will get it by parsing the externals description.
>
> Suppose that path1 in repo1 has svn:externals set to path2 in repo2.
>
> When we list path1 with externals included,
>
> 1. First, list path1.
> 2. Then, process all the externals defined under path1. Parse through the
> individual external description and populate external_target.
>
> For example,
>
> external description under path1:
> external_desc = exdir http://<url-of-path2-in-repo2>
>
> external_target = exdir
> external_parent_url = http://<url-of-path1-in-repo1>
>
> url of external = http://<url-of-path2-in-repo2>
>
> 3. List the entries by reaching 'url of external'.
OK, I now see what you are trying to do here -
I had read to much into the code.
However, in this form, the added functionality is
of limited use, IMHO. I understand that what you
list is basically which paths you would get for a
"svn co $url".
While this is fine, I see two issues with it:
* trees don't get overlayed. Example:
$>svn ls $URL -R
sub1
sub1/fileA
sub1/fileB
sub2
$>svn propget "svn:externals" $URL
http://somewhere/ sub1/subsub
$>svn ls $URL -R --include-externals
sub1
sub1/fileA
sub1/fileB
sub2
sub1/subsub [external].
Desired output
sub1
sub1/fileA
sub1/fileB
sub1/subsub [external @$URL].
sub2
* result of ls on a sub-folder results in less output
$>svn ls $URL/sub1 -R --include-externals
fileA
fileB
Desired output:
fileA
fileB
subsub [external @$URL].
So, it is hard to build e.g. explorer-like GUIs based
on this information. The GUI would still need to collect,
remember and "splice" the externals manually. That
would render your extension almost useless.
-- Stefan^2.
--
Certified & Supported Apache Subversion Downloads:
*
http://www.wandisco.com/subversion/download
*
Re: [PATCH] Implement '--include-externals' option to 'svn list'
Posted by vijay <vi...@collab.net>.
On Tuesday 06 November 2012 08:09 PM, Stefan Fuhrmann wrote:
> On Mon, Nov 5, 2012 at 5:24 PM, vijay <vijay@collab.net
> <ma...@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?
The external_target member will not be given by path / dirent.
We will get it by parsing the externals description.
Suppose that path1 in repo1 has svn:externals set to path2 in repo2.
When we list path1 with externals included,
1. First, list path1.
2. Then, process all the externals defined under path1. Parse through
the individual external description and populate external_target.
For example,
external description under path1:
external_desc = exdir http://<url-of-path2-in-repo2>
external_target = exdir
external_parent_url = http://<url-of-path1-in-repo1>
url of external = http://<url-of-path2-in-repo2>
3. List the entries by reaching 'url of external'.
> Why would you need to group externals?
It will be very easy to understand the output in XML listing mode.
>
>
> + 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?
Externals defined outside the subtree which is being listed don't get
processed. If I do 'svn list --include-externals' for a particular path
or url, I would expect all the externals defined only *under* the path
or url to be listed.
Thanks & Regards,
Vijayaguru
>
> -- Stefan^2.
>
> --
> Certified & Supported Apache Subversion Downloads:
> /
>
> http://www.wandisco.com/subversion/download
>
> /
Re: [PATCH] Implement '--include-externals' option to 'svn list'
Posted by Stefan Fuhrmann <st...@wandisco.com>.
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
*