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

On Thu, Nov 8, 2012 at 12:34 PM, vijay <vijay_at_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_at_collab.net
>> <mailto:vijay_at_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_at_collab.net
>> <mailto:vijay_at_collab.net>
>> <mailto:vijay_at_collab.net <mailto: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>
>> <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
*