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/06/15 00:58:08 UTC
Re: svn commit: r31724 - branches/issue-2843-dev/subversion/libsvn_wc
firemeteor@tigris.org writes:
> Log:
> On issue-2843-dev branch.
>
> Make 'svn update' totally work for svn_depth_exclude. The next step will be a
> test suite for this.
>
> [in subversion/libsvn_wc]
>
> * ambient_depth_filter_editor.c, update_editor.c
> (make_dir_baton): Permit explicitly pull in excluded target.
> (complete_directory): Similar, clear the exclude flag in this situation.
>
> * adm_crawler.c
> (report_revision_and_depths): Explicitly report exclude.
> (svn_wc_crawl_revisions3): Document update.
Same comment about using full relative paths.
> --- branches/issue-2843-dev/subversion/libsvn_wc/adm_crawler.c (r31723)
> +++ branches/issue-2843-dev/subversion/libsvn_wc/adm_crawler.c (r31724)
> @@ -278,6 +278,19 @@ report_revisions_and_depths(svn_wc_adm_a
> continue;
> }
>
> + /* Report the excluded path, no matter whether report_everything. */
> + if (current_entry->depth == svn_depth_exclude)
> + {
> + SVN_ERR(reporter->set_path(report_baton,
> + this_path,
> + SVN_INVALID_REVNUM,
> + svn_depth_exclude,
> + FALSE,
> + NULL,
> + iterpool));
> + continue;
> + }
> +
I think it would be good for this comment to explain in more detail why
we're reporting the excluded path always.
> @@ -507,6 +520,9 @@ svn_wc_crawl_revisions3(const char *path
> if ((! entry) || ((entry->schedule == svn_wc_schedule_add)
> && (entry->kind == svn_node_dir)))
> {
> + /* Don't even check the exclude flag for target.
> + Remember that we permit explicitly pull in target. */
> +
Do you mean "explicitly pulled-in target"?
I'm not sure I understand how to connect the first sentence in that
comment with the second sentence.
> --- branches/issue-2843-dev/subversion/libsvn_wc/ambient_depth_filter_editor.c (r31723)
> +++ branches/issue-2843-dev/subversion/libsvn_wc/ambient_depth_filter_editor.c (r31724)
> @@ -133,7 +133,10 @@ make_dir_baton(struct dir_baton **d_p,
> if (path)
> d->path = svn_path_join(d->path, path, pool);
>
> - if (pb)
> + /* The svn_depth_unknown means that: 1) pb is the anchor; 2) there
> + is an non-null target, for which we are preparing the baton.
> + This enables explicitly pull in the target. */
> + if (pb && pb->ambient_depth != svn_depth_unknown)
> {
> const svn_wc_entry_t *entry;
> svn_boolean_t exclude;
> @@ -152,10 +155,7 @@ make_dir_baton(struct dir_baton **d_p,
> else
> {
> /* If the parent expect all children by default, only exclude
> - it whenever it is explicitly marked as exclude. The
> - svn_depth_unknown means that: 1) pb is the anchor; 2) there
> - is an non-null target, for which we are preparing the baton.
> - This enables explicitly pull in the target. */
> + it whenever it is explicitly marked as exclude. */
> exclude = entry && entry->depth == svn_depth_exclude;
Parens would help readability a lot.
> --- branches/issue-2843-dev/subversion/libsvn_wc/update_editor.c (r31723)
> +++ branches/issue-2843-dev/subversion/libsvn_wc/update_editor.c (r31724)
> @@ -474,10 +474,11 @@ complete_directory(struct edit_baton *eb
> mark this directory complete. */
> if (is_root_dir && *eb->target)
> {
> - if (eb->depth_is_sticky)
> + /* Before we can finish, we may need to clear the exclude flag for
> + target. Also give a chance to the target that is explicitly pulled
> + in. */
> + if (eb->depth_is_sticky || *eb->target)
> {
> - /* Before we can finish, we may need to clear the exclude flag for
> - target */
> SVN_ERR(svn_wc_adm_retrieve(&adm_access,
> eb->adm_access, path, pool));
> SVN_ERR(svn_wc_entries_read(&entries, adm_access, TRUE, pool));
*nod* I think to completely understand this commit, I'll need to just
review the code as a whole -- that is, just go over the file, not the
diff. I was planning to do that anyway, though.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r31724 - branches/issue-2843-dev/subversion/libsvn_wc
Posted by Karl Fogel <kf...@red-bean.com>.
"Rui, Guo" <ti...@mail.ustc.edu.cn> writes:
>> I'm not sure I understand how to connect the first sentence in that
>> comment with the second sentence.
>
> Well, I try to explain it in another way. If we report the TARGET itself as
> excluded the server will send us nothing about the TARGET, right?
>
> I just want 'svn up A' always works, no matter whether the wc root is in
> svn_depth_empty/files or the target A was explicitly excluded while the wc
> root is in svn_depth_immediates/infinity and we want it back now.
Thank you, yes. I've redone the comment in r31917, taking hints from
your explanation above.
(There's another mail from you waiting in my inbox, and I think it
explains how there may be a bug in the code there. I just wanted to get
the comment clarification in before looking at the potential bug. One
thing at a time.)
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r31724 - branches/issue-2843-dev/subversion/libsvn_wc
Posted by "Rui, Guo" <ti...@mail.ustc.edu.cn>.
On Sat, Jun 14, 2008 at 08:58:08PM -0400, Karl Fogel wrote:
> > --- branches/issue-2843-dev/subversion/libsvn_wc/adm_crawler.c (r31723)
> > +++ branches/issue-2843-dev/subversion/libsvn_wc/adm_crawler.c (r31724)
> > @@ -278,6 +278,19 @@ report_revisions_and_depths(svn_wc_adm_a
> > continue;
> > }
> >
> > + /* Report the excluded path, no matter whether report_everything. */
> > + if (current_entry->depth == svn_depth_exclude)
> > + {
> > + SVN_ERR(reporter->set_path(report_baton,
> > + this_path,
> > + SVN_INVALID_REVNUM,
> > + svn_depth_exclude,
> > + FALSE,
> > + NULL,
> > + iterpool));
> > + continue;
> > + }
> > +
>
> I think it would be good for this comment to explain in more detail why
> we're reporting the excluded path always.
Because the report_everything flag indicates that the server will treate the
wc as empty and thus push full content of the files/subdirs. We just want to
say no to this. Does it clear enough? I'll write it as the new comment.
>
> > @@ -507,6 +520,9 @@ svn_wc_crawl_revisions3(const char *path
> > if ((! entry) || ((entry->schedule == svn_wc_schedule_add)
> > && (entry->kind == svn_node_dir)))
> > {
> > + /* Don't even check the exclude flag for target.
> > + Remember that we permit explicitly pull in target. */
> > +
>
> Do you mean "explicitly pulled-in target"?
>
> I'm not sure I understand how to connect the first sentence in that
> comment with the second sentence.
Well, I try to explain it in another way. If we report the TARGET itself as
excluded the server will send us nothing about the TARGET, right?
I just want 'svn up A' always works, no matter whether the wc root is in
svn_depth_empty/files or the target A was explicitly excluded while the wc
root is in svn_depth_immediates/infinity and we want it back now.
Rui
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Mike Pilato, a question for you
Posted by "Rui, Guo" <ti...@mail.ustc.edu.cn>.
On Wed, Jul 02, 2008 at 04:44:31PM -0400, Karl Fogel wrote:
> "Rui, Guo" <ti...@mail.ustc.edu.cn> writes:
> > I am now convinced that it is just a server magic: the server will send the
> > modification up to the degree constrained by "requested_depth" while the
> > reported "wc_depth" (the wc_dir in my original post was just a typo) is only
> > used as a reference to determine whether the full content or just the delta is
> > needed. The 'depth_is_sticky' is relevant because if the depth is not to be
> > updated, we just don't need the content of those does not exist in the wc.
> > This is okay because we have another filter to filter out the redundant
> > information. But since the server support svn_depth_exclude now, we can makes
> > it work better by explicitly exclude the unneeded path upon report. This is a
> > potential performance issue similar to issue 2918 but with a different cause.
> >
> > The above is more a deduction than the result of carefully analysing the
> > server code. So, again, correct me if I'm wrong.
>
> Actually, what you're saying makes sense to me. If the path is excluded
> on the client side, then the server shouldn't send any data for it.
Good. I'll try to fix this problem later.
Rui
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Mike Pilato, a question for you
Posted by "Rui, Guo" <ti...@mail.ustc.edu.cn>.
On Thu, Jul 03, 2008 at 10:40:02AM +0800, Rui, Guo wrote:
> On Wed, Jul 02, 2008 at 04:44:31PM -0400, Karl Fogel wrote:
> > "Rui, Guo" <ti...@mail.ustc.edu.cn> writes:
> > > I am now convinced that it is just a server magic: the server will send the
> > > modification up to the degree constrained by "requested_depth" while the
> > > reported "wc_depth" (the wc_dir in my original post was just a typo) is only
> > > used as a reference to determine whether the full content or just the delta is
> > > needed. The 'depth_is_sticky' is relevant because if the depth is not to be
> > > updated, we just don't need the content of those does not exist in the wc.
> > > This is okay because we have another filter to filter out the redundant
> > > information. But since the server support svn_depth_exclude now, we can makes
> > > it work better by explicitly exclude the unneeded path upon report. This is a
> > > potential performance issue similar to issue 2918 but with a different cause.
> > >
> > > The above is more a deduction than the result of carefully analysing the
> > > server code. So, again, correct me if I'm wrong.
> >
> > Actually, what you're saying makes sense to me. If the path is excluded
> > on the client side, then the server shouldn't send any data for it.
> Good. I'll try to fix this problem later.
After a second thought, I think I'll not fix this in my gsoc project. Because
this work should be done in server side, rather than the crawler logic.
For example, we build a wc by the following commands:
svn co --depth empty http://server/greek_tree
svn up --depth empty A
So, we want to prevent the server from sending the possible modification
within A upon 'svn up'. If we choose to explicitly exclude the subtree as I
mentioned in the previous post, how can we know what is to be excluded?
Remember that we know nothing about the subtree except the name of the root.
We can only pass some more information to the server (e.g. the depth_is_stick
flag) and relies on the server to do the work.
If we designed the depth scheme in a simpler way at the beginning, we may have
other alternatives. For example, three kinds of depth may be enough: excluded,
leaf(files) and infinity. And I think it will be beneficial to record the
excluded items in entries file: adding items that conflict with excluded ones
can be detected earlier. In this alternative scheme, server does not need to
know anything about depth. The only capability that will be beneficial is to
exclude a path when it is requested to.
Rui
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Mike Pilato, a question for you
Posted by Karl Fogel <kf...@red-bean.com>.
"Rui, Guo" <ti...@mail.ustc.edu.cn> writes:
> I am now convinced that it is just a server magic: the server will send the
> modification up to the degree constrained by "requested_depth" while the
> reported "wc_depth" (the wc_dir in my original post was just a typo) is only
> used as a reference to determine whether the full content or just the delta is
> needed. The 'depth_is_sticky' is relevant because if the depth is not to be
> updated, we just don't need the content of those does not exist in the wc.
> This is okay because we have another filter to filter out the redundant
> information. But since the server support svn_depth_exclude now, we can makes
> it work better by explicitly exclude the unneeded path upon report. This is a
> potential performance issue similar to issue 2918 but with a different cause.
>
> The above is more a deduction than the result of carefully analysing the
> server code. So, again, correct me if I'm wrong.
Actually, what you're saying makes sense to me. If the path is excluded
on the client side, then the server shouldn't send any data for it.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Mike Pilato, a question for you
Posted by "Rui, Guo" <ti...@mail.ustc.edu.cn>.
On Mon, Jun 30, 2008 at 12:37:25PM -0400, C. Michael Pilato wrote:
>>> After reading the crawler code, I was quite confused. There seems no easy way
>>> to get the depth_is_sticky information. Why does the code work without knowing
>>> the depth_is_sticky information? Server-side magic? It seems that the server
>>> will always push edit to the degree required by 'requested_depth', while the
>>> wc_dir is only used to determine whether full content or just delta is needed.
>>> I did not read the server code very carefully. Correct me if I am wrong.
>>
>> (In a previous mail, I mistakenly referred to this problem as being
>> elsewhere in the code. Sorry for the confusion. I know where we are
>> now.)
>>
>> Uh. Hmmm. That's an interesting question... I think cmpilato is
>> probably better qualified to answer it. Mike? By the way, the full
>> thread is
>>
>> http://subversion.tigris.org/servlets/BrowseList?list=dev&by=thread&from=660362
>>
>> and the message I'm responding to here is:
>>
>> http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=140208
>
> I. Uh. I dunno.
>
> The crawler is responsible for telling the server what the working copy
> contains. The ra_do_update call tells the server what the working copy
> wants. The server gives the client what is needed to become what it
> wants to become. This much I know. I know no more.
>
> Given this, I would assume that the crawler doesn't need to know the
> "depth_is_sticky" bit because that is a facet of the requested operation,
> not the working copy description.
I am now convinced that it is just a server magic: the server will send the
modification up to the degree constrained by "requested_depth" while the
reported "wc_depth" (the wc_dir in my original post was just a typo) is only
used as a reference to determine whether the full content or just the delta is
needed. The 'depth_is_sticky' is relevant because if the depth is not to be
updated, we just don't need the content of those does not exist in the wc.
This is okay because we have another filter to filter out the redundant
information. But since the server support svn_depth_exclude now, we can makes
it work better by explicitly exclude the unneeded path upon report. This is a
potential performance issue similar to issue 2918 but with a different cause.
The above is more a deduction than the result of carefully analysing the
server code. So, again, correct me if I'm wrong.
Rui
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Mike Pilato, a question for you
Posted by "C. Michael Pilato" <cm...@collab.net>.
Karl Fogel wrote:
> "Rui, Guo" <ti...@mail.ustc.edu.cn> writes:
>> On Sun, Jun 15, 2008 at 02:53:42PM +0800, Rui, Guo wrote:
>>> On Sat, Jun 14, 2008 at 08:58:08PM -0400, Karl Fogel wrote:
>>>>> --- branches/issue-2843-dev/subversion/libsvn_wc/adm_crawler.c (r31723)
>>>>> +++ branches/issue-2843-dev/subversion/libsvn_wc/adm_crawler.c (r31724)
>>>>> @@ -278,6 +278,19 @@ report_revisions_and_depths(svn_wc_adm_a
>>>>> continue;
>>>>> }
>>>>>
>>>>> + /* Report the excluded path, no matter whether report_everything. */
>>>>> + if (current_entry->depth == svn_depth_exclude)
>>>>> + {
>>>>> + SVN_ERR(reporter->set_path(report_baton,
>>>>> + this_path,
>>>>> + SVN_INVALID_REVNUM,
>>>>> + svn_depth_exclude,
>>>>> + FALSE,
>>>>> + NULL,
>>>>> + iterpool));
>>>>> + continue;
>>>>> + }
>>>>> +
>>>> I think it would be good for this comment to explain in more detail why
>>>> we're reporting the excluded path always.
>>> Because the report_everything flag indicates that the server will treate the
>>> wc as empty and thus push full content of the files/subdirs. We just want to
>>> say no to this. Does it clear enough? I'll write it as the new comment.
>> I found a bug here during working on a new test case. The test is too
>> restrictive. We should bypass the svn_depth_exclude report when the
>> depth_is_sticky, so as to let go the server-side modifications.
>>
>> After reading the crawler code, I was quite confused. There seems no easy way
>> to get the depth_is_sticky information. Why does the code work without knowing
>> the depth_is_sticky information? Server-side magic? It seems that the server
>> will always push edit to the degree required by 'requested_depth', while the
>> wc_dir is only used to determine whether full content or just delta is needed.
>> I did not read the server code very carefully. Correct me if I am wrong.
>
> (In a previous mail, I mistakenly referred to this problem as being
> elsewhere in the code. Sorry for the confusion. I know where we are
> now.)
>
> Uh. Hmmm. That's an interesting question... I think cmpilato is
> probably better qualified to answer it. Mike? By the way, the full
> thread is
>
> http://subversion.tigris.org/servlets/BrowseList?list=dev&by=thread&from=660362
>
> and the message I'm responding to here is:
>
> http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=140208
I. Uh. I dunno.
The crawler is responsible for telling the server what the working copy
contains. The ra_do_update call tells the server what the working copy
wants. The server gives the client what is needed to become what it wants
to become. This much I know. I know no more.
Given this, I would assume that the crawler doesn't need to know the
"depth_is_sticky" bit because that is a facet of the requested operation,
not the working copy description.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Mike Pilato, a question for you (was: svn commit: r31724 - branches/issue-2843-dev/subversion/libsvn_wc)
Posted by Karl Fogel <kf...@red-bean.com>.
"Rui, Guo" <ti...@mail.ustc.edu.cn> writes:
> On Sun, Jun 15, 2008 at 02:53:42PM +0800, Rui, Guo wrote:
>> On Sat, Jun 14, 2008 at 08:58:08PM -0400, Karl Fogel wrote:
>> > > --- branches/issue-2843-dev/subversion/libsvn_wc/adm_crawler.c (r31723)
>> > > +++ branches/issue-2843-dev/subversion/libsvn_wc/adm_crawler.c (r31724)
>> > > @@ -278,6 +278,19 @@ report_revisions_and_depths(svn_wc_adm_a
>> > > continue;
>> > > }
>> > >
>> > > + /* Report the excluded path, no matter whether report_everything. */
>> > > + if (current_entry->depth == svn_depth_exclude)
>> > > + {
>> > > + SVN_ERR(reporter->set_path(report_baton,
>> > > + this_path,
>> > > + SVN_INVALID_REVNUM,
>> > > + svn_depth_exclude,
>> > > + FALSE,
>> > > + NULL,
>> > > + iterpool));
>> > > + continue;
>> > > + }
>> > > +
>> >
>> > I think it would be good for this comment to explain in more detail why
>> > we're reporting the excluded path always.
>>
>> Because the report_everything flag indicates that the server will treate the
>> wc as empty and thus push full content of the files/subdirs. We just want to
>> say no to this. Does it clear enough? I'll write it as the new comment.
>
> I found a bug here during working on a new test case. The test is too
> restrictive. We should bypass the svn_depth_exclude report when the
> depth_is_sticky, so as to let go the server-side modifications.
>
> After reading the crawler code, I was quite confused. There seems no easy way
> to get the depth_is_sticky information. Why does the code work without knowing
> the depth_is_sticky information? Server-side magic? It seems that the server
> will always push edit to the degree required by 'requested_depth', while the
> wc_dir is only used to determine whether full content or just delta is needed.
> I did not read the server code very carefully. Correct me if I am wrong.
(In a previous mail, I mistakenly referred to this problem as being
elsewhere in the code. Sorry for the confusion. I know where we are
now.)
Uh. Hmmm. That's an interesting question... I think cmpilato is
probably better qualified to answer it. Mike? By the way, the full
thread is
http://subversion.tigris.org/servlets/BrowseList?list=dev&by=thread&from=660362
and the message I'm responding to here is:
http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=140208
> And I have one more question. The crawler code always use the same
> depth value when calling report_revisions_and_depths(), even when the
> original value is svn_depth_immediates. Some extra files will got
> reported in this situation. Am I right?
Well, in the recursive call within report_revisions_and_depths(), we
only recurse if SVN_DEPTH_IS_RECURSIVE(depth), and svn_depth_immediates
would fail that test.
And in the top call, from svn_wc_crawl_revisions3(), we pass depth
unmodified because report_revisions_and_depths() will do the right thing
with depth: it checks depth before recursing into any subdirs, for
example here:
/*** Directories (in recursive mode) ***/
else if (current_entry->kind == svn_node_dir
&& (depth > svn_depth_files
|| depth == svn_depth_unknown))
So I don't see a problem, but maybe I'm missing something. Could you
describe an exact scenario you're worried about?
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r31724 -
branches/issue-2843-dev/subversion/libsvn_wc
Posted by "Rui, Guo" <ti...@mail.ustc.edu.cn>.
On Sun, Jun 15, 2008 at 02:53:42PM +0800, Rui, Guo wrote:
> On Sat, Jun 14, 2008 at 08:58:08PM -0400, Karl Fogel wrote:
> > > --- branches/issue-2843-dev/subversion/libsvn_wc/adm_crawler.c (r31723)
> > > +++ branches/issue-2843-dev/subversion/libsvn_wc/adm_crawler.c (r31724)
> > > @@ -278,6 +278,19 @@ report_revisions_and_depths(svn_wc_adm_a
> > > continue;
> > > }
> > >
> > > + /* Report the excluded path, no matter whether report_everything. */
> > > + if (current_entry->depth == svn_depth_exclude)
> > > + {
> > > + SVN_ERR(reporter->set_path(report_baton,
> > > + this_path,
> > > + SVN_INVALID_REVNUM,
> > > + svn_depth_exclude,
> > > + FALSE,
> > > + NULL,
> > > + iterpool));
> > > + continue;
> > > + }
> > > +
> >
> > I think it would be good for this comment to explain in more detail why
> > we're reporting the excluded path always.
> Because the report_everything flag indicates that the server will treate the
> wc as empty and thus push full content of the files/subdirs. We just want to
> say no to this. Does it clear enough? I'll write it as the new comment.
I found a bug here during working on a new test case. The test is too
restrictive. We should bypass the svn_depth_exclude report when the
depth_is_sticky, so as to let go the server-side modifications.
After reading the crawler code, I was quite confused. There seems no easy way
to get the depth_is_sticky information. Why does the code work without knowing
the depth_is_sticky information? Server-side magic? It seems that the server
will always push edit to the degree required by 'requested_depth', while the
wc_dir is only used to determine whether full content or just delta is needed.
I did not read the server code very carefully. Correct me if I am wrong.
And I have one more question. The crawler code always use the same depth value
when calling report_revisions_and_depths(), even when the original value is
svn_depth_immediates. Some extra files will got reported in this situation. Am
I right?
Rui
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org