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/30 02:28:45 UTC
Mike Pilato, a question for you (was: svn commit: r31724 - branches/issue-2843-dev/subversion/libsvn_wc)
"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: 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