On 01.04.2014 13:57, Bert Huijben wrote: > > Hi Brane, > > > > I don’t think the BASE optimization into > svn_wc__db_read_children_info() is required to obtain good > performance. In most working copies (I would hope J) most nodes are > status normal so the normal call will obtain the information you need. >
We stat regardless of node status, to detect local modifications and missing nodes. > And as we don’t report symlink vs file in the status output (and no > last_changed_* for deleted) there would just be an additional > svn_wc__db_base_get_info() on replacements. > > (Which the ‘svn’ client already has… but that is a different story) > > Otherwise +1 on using a different query with ‘AND op_depth=0’. > Yes, this is what I had in mind. It'll be the simplest solution, with the least extra code. I'll just have to propagate the flag to a couple more private functions, but that's trivial compared to what I'm doing now. > (Adding a Boolean as flag to the query breaks the sqlite optimizer for > the query as it only performs optimizing once, before it knows the > actual values passed) > Ack, noted. -- Brane > > > > > The only reason we obtain the special flag is to show if a file is > obstructing a symlink or vice versa, and in your case we don’t want to > report that, so setting special to FALSE when handling things from > status should ‘just work’. (If you implement this in wc_db.c I would > say that we should obtain the proper value) > > > > > > The remaining interesting case would be when we see obstructing > working copies… as in that case we don’t support reading the nodes > (and their children) by their abspaths. > > > > > > > > Personally I wouldn’t be surprised if you could obtain the required > performance for this feature by just skipping all file comparisions > (and the individual filestats where necessary), instead of > implementing the whole ignore everything local work. > > I don’t think reading the directory entries per directory is that > expensive… > > Subversion <= 1.6 read them per node during status processing… That > was really expensive on Windows. But it could be that reading all the > directory details is expensive on !Windows now, but perhaps it might > help to pass TRUE for only_check_type on svn_io_get_dirents3(). > > (On Windows that doesn’t change the performance, but I think it does > on other platforms) > > > > Bert > > > > *From:*Branko Čibej [mailto:br...@wandisco.com] > *Sent:* dinsdag 1 april 2014 13:10 > *To:* dev@subversion.apache.org > *Subject:* Re: svn commit: r1583599 - in > /subversion/branches/remote-only-status/subversion: > include/svn_client.h libsvn_wc/status.c tests/libsvn_client/client-test.c > > > > On 01.04.2014 12:54, Bert Huijben wrote: > > > > > > -----Original Message----- > > From: br...@apache.org <mailto:br...@apache.org> > [mailto:br...@apache.org] > > Sent: dinsdag 1 april 2014 12:41 > > To: comm...@subversion.apache.org > <mailto:comm...@subversion.apache.org> > > Subject: svn commit: r1583599 - in /subversion/branches/remote-only- > > status/subversion: include/svn_client.h libsvn_wc/status.c > > tests/libsvn_client/client-test.c > > > > Author: brane > > Date: Tue Apr 1 10:41:29 2014 > > New Revision: 1583599 > > > > URL: http://svn.apache.org/r1583599 > > Log: > > On the remote-only-status branch: Do not report local replacements. > > > > * subversion/include/svn_client.h (svn_client_status6): Update > docstring. > > * subversion/libsvn_wc/status.c (assemble_status): Ignore local adds. > > Report local replacements as deletions of the working node. > > (get_dir_status): Remove redundant (and incorrect) filter for > additions. > > * subversion/tests/libsvn_client/client-test.c > (test_remote_only_status): > > Extend test case with an example of a local replacement. > > > > Modified: > > > subversion/branches/remote-only-status/subversion/include/svn_client.h > > > subversion/branches/remote-only-status/subversion/libsvn_wc/status.c > > subversion/branches/remote-only- > > status/subversion/tests/libsvn_client/client-test.c > > > > Modified: subversion/branches/remote-only- > > status/subversion/include/svn_client.h > > URL: http://svn.apache.org/viewvc/subversion/branches/remote-only- > > status/subversion/include/svn_client.h?rev=1583599&r1=1583598&r2=15835 > > 99&view=diff > > ========================================================== > > ==================== > > --- subversion/branches/remote-only- > > status/subversion/include/svn_client.h (original) > > +++ subversion/branches/remote-only- > > status/subversion/include/svn_client.h Tue Apr 1 10:41:29 2014 > > @@ -2512,6 +2512,13 @@ typedef svn_error_t *(*svn_client_status > > * - If @a check_working_copy is not set, do not scan the working > > * copy for locally modified and missing files. This parameter > > * will be ignored unless @a check_out_of_date is set. > > + * When set, the status report will be different in the > following > > + * details: > > + * > > + * -- Local modifications, missing nodes and locally added nodes > > + * will not be reported. > > + * -- Locally replaced nodes will be reported as deletions of > > + * the original node instead of as replacements. > > * > > * If @a no_ignore is @c FALSE, don't report any file or directory > (or > > * recurse into any directory) that is found by recursion (as > opposed to > > > > Modified: subversion/branches/remote-only- > > status/subversion/libsvn_wc/status.c > > URL: http://svn.apache.org/viewvc/subversion/branches/remote-only- > > status/subversion/libsvn_wc/status.c?rev=1583599&r1=1583598&r2=158359 > > 9&view=diff > > ========================================================== > > ==================== > > --- > subversion/branches/remote-only-status/subversion/libsvn_wc/status.c > > (original) > > +++ subversion/branches/remote-only- > > status/subversion/libsvn_wc/status.c Tue Apr 1 10:41:29 2014 > > @@ -573,7 +573,42 @@ assemble_status(svn_wc_status3_t **statu > > if (below_working != svn_wc__db_status_not_present > > && below_working != svn_wc__db_status_deleted) > > { > > - node_status = svn_wc_status_replaced; > > + if (check_working_copy) > > + node_status = svn_wc_status_replaced; > > + else > > + { > > + /* This is a remote-only walk; report the > > + base node info instead of the replacement. > */ > > + const char *target; > > + const svn_checksum_t *checksum; > > + struct svn_wc__db_info_t *base_info = > > + apr_palloc(scratch_pool, sizeof(*base_info)); > > + memcpy(base_info, info, sizeof(*base_info)); > > + SVN_ERR(svn_wc__db_read_pristine_info( > > + &base_info->status, > > + &base_info->kind, > > + &base_info->changed_rev, > > + &base_info->changed_date, > > + &base_info->changed_author, > > + &base_info->depth, > > + &checksum, &target, > > + &base_info->had_props, NULL, > > + db, local_abspath, > > + scratch_pool, scratch_pool)); > > + SVN_ERR(svn_wc__db_base_get_info( > > + NULL, NULL, &base_info->revnum, > > + NULL, NULL, NULL, NULL, NULL, > > + NULL, NULL, NULL, NULL, > > + NULL, NULL, NULL, NULL, > > + db, local_abspath, > > + scratch_pool, scratch_pool)); > > > > If you really want the repository information you should read all the > values using svn_wc__db_base_get_info as the changed* values read by > svn_wc__db_read_pristine_info are those of the highest layer... > > Only in case of a BASE-delete (not in case of a working delete, or a > replacement!) do they represent the information you want. > > > > + base_info->has_checksum = (checksum != NULL); > > +#ifdef HAVE_SYMLINK > > + base_info->special = (target != NULL); > > > > Target is not used (yet)... you must read the properties to determine if > a node is a symlink or not. I think you can get the property values from > svn_wc__db_base_get_info() now. > > > > +#endif > > > > Bert > > > > + node_status = svn_wc_status_deleted; > > + info = base_info; > > + } > > } > > else > > node_status = svn_wc_status_added; > > @@ -610,6 +645,16 @@ assemble_status(svn_wc_status3_t **statu > > && prop_status != svn_wc_status_none) > > node_status = prop_status; > > > > + > > + /* Ignore local additions in remote-only mode */ > > + if (!check_working_copy > > + && node_status == svn_wc_status_added > > + && !moved_from_abspath) > > + { > > + *status = NULL; > > + return SVN_NO_ERROR; > > + } > > > > I don't think this really checks what you want to check here... I would > guess you want to check the have_base value (too?), as replaced nodes will > also have an added status. > > (And even with that you might still miss a few edge cases in case parent > directories are replaced with files, or the other way around) > > > > Bert > > > Thanks for the review, again! > > I'm actually thinking that this is really a hack, and I should just > modify svn_wc__db_read_single_info and svn_wc__db_read_children_info > to be aware of the remote-only flag. That's where the > BASE->WORKING->ACTUAL overlay happens, and what I'm doing here is > basically just trying to reproduce part of the result, which seems > like a waste of code. IIUC, if I get the modifications just right, the > additions and replacements won't show up in the results anyway, and > I'll be able to revert this latest commit, and not have a higher-level > filter for added nodes. > > -- Brane > > -- > Branko Čibej |*Director of Subversion* > WANdisco ///Non-Stop Data/ > e.br...@wandisco.com <mailto:br...@wandisco.com> > -- Branko Čibej | Director of Subversion WANdisco // Non-Stop Data e. br...@wandisco.com
