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 <vi...@collab.net >> <mailto: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 <vi...@collab.net >> <mailto:vi...@collab.net> >> <mailto:vi...@collab.net <mailto: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> >> >> <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 *
