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

On Wed, Nov 7, 2012 at 3:02 PM, vijay <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>> 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
*