You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Karl Fogel <kf...@red-bean.com> on 2008/08/17 22:49:52 UTC
Re: svn commit: r32298 - branches/issue-2843-dev/subversion/libsvn_wc
firemeteor@tigris.org writes:
> Log:
> On the issue-2843-dev branch.
>
> * subversion/libsvn_wc/crop.c
> (svn_wc_crop_tree): Explicitly prohibit excluding switched path.
> (crop_children): Move a TODO item to svn_wc_crop_tree().
This log message doesn't mention the change to adm_crawler.c.
> --- branches/issue-2843-dev/subversion/libsvn_wc/adm_crawler.c
> +++ branches/issue-2843-dev/subversion/libsvn_wc/adm_crawler.c
> @@ -288,6 +288,10 @@ report_revisions_and_depths(svn_wc_adm_a
> that the server will treate the wc as empty and thus push
> full content of the files/subdirs. But we want to prevent the
> server from pushing the full content of this_path at us. */
> +
> + /* The server does not support link_path report on excluded
> + path. We explicitly prohibit this situation in
> + svn_wc_crop_tree(). */
> SVN_ERR(reporter->set_path(report_baton,
> this_path,
> SVN_INVALID_REVNUM,
This is the change not mentioned. It's only a comment, so it's no big
deal, but to avoid confusion the log message should still mention it.
> --- branches/issue-2843-dev/subversion/libsvn_wc/crop.c
> +++ branches/issue-2843-dev/subversion/libsvn_wc/crop.c
> @@ -220,43 +219,80 @@ svn_wc_crop_tree(svn_wc_adm_access_t *an
> /* Crop the target itself if we are requested to. */
> if (depth == svn_depth_exclude)
> {
> - svn_boolean_t is_root;
> + svn_boolean_t switched, entry_in_repos;
> + const svn_wc_entry_t *parent_entry = NULL;
> + svn_wc_adm_access_t *p_access;
> +
> + /* Safeguard on bad target. */
> + if (*full_path == 0)
> + return svn_error_createf
> + (SVN_ERR_UNSUPPORTED_FEATURE, NULL,
> + _("Cannot exclude current directory."));
> +
> + if (svn_dirent_is_root(full_path, strlen(full_path)))
> + return svn_error_createf
> + (SVN_ERR_UNSUPPORTED_FEATURE, NULL,
> + _("Cannot exclude root directory."));
Looks good so far.
> + /* This simulates the logic of svn_wc_is_wc_root(). */
> + {
> + const char *bname, *pname;
> + svn_error_t *err = NULL;
> + svn_path_split(full_path, &pname, &bname, pool);
> + SVN_ERR(svn_wc__adm_retrieve_internal(&p_access, anchor, pname,
> + pool));
> + if (! p_access)
> + err = svn_wc_adm_probe_open3(&p_access, NULL, pname, FALSE, 0,
> + NULL, NULL, pool);
> +
> + if (! err)
> + err = svn_wc_entry(&parent_entry, pname, p_access, FALSE, pool);
> +
> + if (err)
> + svn_error_clear(err);
> +
> + switched
> + = parent_entry && strcmp(entry->url,
> + svn_path_url_add_component
> + (parent_entry->url, bname, pool));
> +
> + /* The server simply do not accept excluded link_path and thus
> + switched path can not be excluede. Just completely prohibit this
> + situation. */
> + if (switched)
> + return svn_error_createf
> + (SVN_ERR_UNSUPPORTED_FEATURE, NULL,
> + _("Cannot crop '%s': it is a switched path."),
> + svn_path_local_style(full_path, pool));
> + }
When I first read the comment at the top...
"/* This simulates the logic of svn_wc_is_wc_root(). */"
...I thought that you must be about to test whether the path is a wc
root because a wc root cannot be excluded (right?). But it turns out
you're simulating the logic of svn_wc_is_wc_root() just to find out
whether the path is switched (because if it is switched, then we cannot
exclude it -- a switched directory behaves "like" a wc root in this
way).
So would it be simpler to just use svn_wc__path_switched()? This is its
doc string, from include/private/svn_wc_private.h:
/** Given a @a wcpath with its accompanying @a entry, set @a
* switched to true if @a wcpath is switched, otherwise set @a
* switched to false. If @a entry is an incomplete entry obtained
* from @a wcpath's parent return @c SVN_ERR_ENTRY_MISSING_URL.
* All allocations are done in @a pool.
*
* @since New in 1.5.
*/
svn_error_t *
svn_wc__path_switched(const char *wcpath,
svn_boolean_t *switched,
const svn_wc_entry_t *entry,
apr_pool_t *pool);
> /* If the target entry is just added without history, it does not exist
> in the repos (in which case we won't exclude it). */
> - svn_boolean_t entry_in_repos
> + entry_in_repos
> = ! ((entry->schedule == svn_wc_schedule_add
> || entry->schedule == svn_wc_schedule_replace)
> && ! entry->copied);
I had to read this conditional twice before I understood :-).
> - /* TODO(#2843) Switched path will be treated as root and bypass the
> - exclude flag setup below. Beause the server simply do not accept
> - excluded link_path. But without the flag it is not an excluded path
> - at all, maybe just prohibit this situation? */
> - svn_wc_is_wc_root(&is_root, full_path, anchor, pool);
> - if ((! is_root) && entry_in_repos)
> + /* Mark the target as excluded, if the parent requires it by
> + default. */
> + if (parent_entry && entry_in_repos
> + && (parent_entry->depth > svn_depth_files))
> {
Should it be
(parent_entry->depth >= svn_depth_files)
? (Or do we already know for sure that target is a directory?)
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r32298 - branches/issue-2843-dev/subversion/libsvn_wc
Posted by Karl Fogel <kf...@red-bean.com>.
"Rui, Guo" <ti...@mail.ustc.edu.cn> writes:
>> When I first read the comment at the top...
>>
>> "/* This simulates the logic of svn_wc_is_wc_root(). */"
>>
>> ...I thought that you must be about to test whether the path is a wc
>> root because a wc root cannot be excluded (right?). But it turns out
>> you're simulating the logic of svn_wc_is_wc_root() just to find out
>> whether the path is switched (because if it is switched, then we cannot
>> exclude it -- a switched directory behaves "like" a wc root in this
>> way).
>>
>> So would it be simpler to just use svn_wc__path_switched()? This is its
>> doc string, from include/private/svn_wc_private.h:
>>
>> /** Given a @a wcpath with its accompanying @a entry, set @a
>> * switched to true if @a wcpath is switched, otherwise set @a
>> * switched to false. If @a entry is an incomplete entry obtained
>> * from @a wcpath's parent return @c SVN_ERR_ENTRY_MISSING_URL.
>> * All allocations are done in @a pool.
>> *
>> * @since New in 1.5.
>> */
>> svn_error_t *
>> svn_wc__path_switched(const char *wcpath,
>> svn_boolean_t *switched,
>> const svn_wc_entry_t *entry,
>> apr_pool_t *pool);
>
> Thanks for pointing this out for me. I just did not aware of this function.
> But even if I call svn_wc__path_switched() instead, I have to prepare
> parent_entry for later use. So it does not save many code, right?
Sure. I just wanted to point it out; use your judgement on what will
produce the best code.
>> > + /* Mark the target as excluded, if the parent requires it by
>> > + default. */
>> > + if (parent_entry && entry_in_repos
>> > + && (parent_entry->depth > svn_depth_files))
>> > {
>>
>> Should it be
>>
>> (parent_entry->depth >= svn_depth_files)
>>
>> ? (Or do we already know for sure that target is a directory?)
>
> We just do not setup an exclude flag if the parent only requires files, since
> directories is excluded by default.
Might be good to add that to the comment, then.
Best,
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r32298 -
branches/issue-2843-dev/subversion/libsvn_wc
Posted by "Rui, Guo" <ti...@mail.ustc.edu.cn>.
On Sun, Aug 17, 2008 at 06:49:52PM -0400, Karl Fogel wrote:
> firemeteor@tigris.org writes:
> > Log:
> > On the issue-2843-dev branch.
> >
> > * subversion/libsvn_wc/crop.c
> > (svn_wc_crop_tree): Explicitly prohibit excluding switched path.
> > (crop_children): Move a TODO item to svn_wc_crop_tree().
>
> This log message doesn't mention the change to adm_crawler.c.
>
> > --- branches/issue-2843-dev/subversion/libsvn_wc/adm_crawler.c
> > +++ branches/issue-2843-dev/subversion/libsvn_wc/adm_crawler.c
> > @@ -288,6 +288,10 @@ report_revisions_and_depths(svn_wc_adm_a
> > that the server will treate the wc as empty and thus push
> > full content of the files/subdirs. But we want to prevent the
> > server from pushing the full content of this_path at us. */
> > +
> > + /* The server does not support link_path report on excluded
> > + path. We explicitly prohibit this situation in
> > + svn_wc_crop_tree(). */
> > SVN_ERR(reporter->set_path(report_baton,
> > this_path,
> > SVN_INVALID_REVNUM,
>
> This is the change not mentioned. It's only a comment, so it's no big
> deal, but to avoid confusion the log message should still mention it.
My fault. I must be sleepy when doing the commit. :-(
Fixed.
> > + /* This simulates the logic of svn_wc_is_wc_root(). */
> > + {
> > + const char *bname, *pname;
> > + svn_error_t *err = NULL;
> > + svn_path_split(full_path, &pname, &bname, pool);
> > + SVN_ERR(svn_wc__adm_retrieve_internal(&p_access, anchor, pname,
> > + pool));
> > + if (! p_access)
> > + err = svn_wc_adm_probe_open3(&p_access, NULL, pname, FALSE, 0,
> > + NULL, NULL, pool);
> > +
> > + if (! err)
> > + err = svn_wc_entry(&parent_entry, pname, p_access, FALSE, pool);
> > +
> > + if (err)
> > + svn_error_clear(err);
> > +
> > + switched
> > + = parent_entry && strcmp(entry->url,
> > + svn_path_url_add_component
> > + (parent_entry->url, bname, pool));
> > +
> > + /* The server simply do not accept excluded link_path and thus
> > + switched path can not be excluede. Just completely prohibit this
> > + situation. */
> > + if (switched)
> > + return svn_error_createf
> > + (SVN_ERR_UNSUPPORTED_FEATURE, NULL,
> > + _("Cannot crop '%s': it is a switched path."),
> > + svn_path_local_style(full_path, pool));
> > + }
>
> When I first read the comment at the top...
>
> "/* This simulates the logic of svn_wc_is_wc_root(). */"
>
> ...I thought that you must be about to test whether the path is a wc
> root because a wc root cannot be excluded (right?). But it turns out
> you're simulating the logic of svn_wc_is_wc_root() just to find out
> whether the path is switched (because if it is switched, then we cannot
> exclude it -- a switched directory behaves "like" a wc root in this
> way).
>
> So would it be simpler to just use svn_wc__path_switched()? This is its
> doc string, from include/private/svn_wc_private.h:
>
> /** Given a @a wcpath with its accompanying @a entry, set @a
> * switched to true if @a wcpath is switched, otherwise set @a
> * switched to false. If @a entry is an incomplete entry obtained
> * from @a wcpath's parent return @c SVN_ERR_ENTRY_MISSING_URL.
> * All allocations are done in @a pool.
> *
> * @since New in 1.5.
> */
> svn_error_t *
> svn_wc__path_switched(const char *wcpath,
> svn_boolean_t *switched,
> const svn_wc_entry_t *entry,
> apr_pool_t *pool);
Thanks for pointing this out for me. I just did not aware of this function.
But even if I call svn_wc__path_switched() instead, I have to prepare
parent_entry for later use. So it does not save many code, right?
> > - /* TODO(#2843) Switched path will be treated as root and bypass the
> > - exclude flag setup below. Beause the server simply do not accept
> > - excluded link_path. But without the flag it is not an excluded path
> > - at all, maybe just prohibit this situation? */
> > - svn_wc_is_wc_root(&is_root, full_path, anchor, pool);
> > - if ((! is_root) && entry_in_repos)
> > + /* Mark the target as excluded, if the parent requires it by
> > + default. */
> > + if (parent_entry && entry_in_repos
> > + && (parent_entry->depth > svn_depth_files))
> > {
>
> Should it be
>
> (parent_entry->depth >= svn_depth_files)
>
> ? (Or do we already know for sure that target is a directory?)
We just do not setup an exclude flag if the parent only requires files, since
directories is excluded by default.
Rui
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org