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 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:brane_at_wandisco.com]
> *Sent:* dinsdag 1 april 2014 13:10
> *To:* dev_at_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: brane_at_apache.org <mailto:brane_at_apache.org> [mailto:brane_at_apache.org]
>
> Sent: dinsdag 1 april 2014 12:41
>
> To: commits_at_subversion.apache.org <mailto:commits_at_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.brane_at_wandisco.com <mailto:brane_at_wandisco.com>
>

-- 
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane_at_wandisco.com