You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@wandisco.com> on 2010/06/15 17:21:16 UTC
Info needed in 1.6.x STATUS file
Hi devs. Going through the 1.6.x. STATUS file, I am finding some items
that could do with a better description or justification to make it
easier to evaluate them. Can anyone provide information on the
following:
> * r934599, r934603
> Fix a concurrency issue in the FSFS rep-cache code. This indirectly
> concerns issue #3506.
> Justification:
> Opening the DB twice (and overwriting the handle) is undesired.
Why is that undesired? I mean that as a serious practical question -
obviously it's theoretically redundant but that in itself doesn't
necessarily matter. What is the user-visible effect of the problem, and
of the proposed change?
> Notes:
> r934599 updates the svn_atomic__init_once() interface.
> r934603 is the actual fix.
> Notes:
> Depends on the r879966 group, which does not merge cleanly.
> Branch:
> ^/subversion/branches/1.6.x-issue3506-init_once
> (based on ^/subversion/branches/1.6.x-issue3506)
> Votes:
> +1: danielsh
> * r878590, r878607, r878625, r878626, r878627
> In trunk we optimized the common case of 'find-and-replace with same uri'
> of proxied content thanks to issue 3445 "WebDAV proxy code munging user
> data".
What is the purpose and effect of the change proposed for back-port?
> r878590 is just a change that adds FIXME marker to code comment. We take it
> to avoid spurious conflicts with other revisions.
> r878607 Special cases no-op find and replace.
> "r878625, r878626, r878627" are follow-up to r878607 and the other long
> pending Master info leak to the clients.
> Justification:
> 1. This group has the most common 'optimization' fix of *not* groking the
> proxied response to find and replace with same string.
> 2. Fixes the master information leak via the Location header.
> 3. Need this to be ported for the other defect to be ported
> without conflict.
> Votes:
> +1: kameshj, cmpilato
> -0: julianfoad (seem to be multiple changes here for different reasons -
> at least issue '3445 and an optimization and an information leak;
> r878607 log msg says it fixes bugs but it's not clear what bugs;
> don't know how to tell whether justification 1 is significant;
> justifications don't seem to refer to issue #3445. Please can we
> separate these changes and clearly describe each one? And update
> the r878607 log msg.)
Ah, I already put my concerns here.
> * r916286, r917512
> ???
What is the purpose and effect of the change proposed for back-port?
(It was me who added the question marks, some time ago.)
> Notes:
> This backport depends on the r878590 group only for the conflict free port.
> Justification:
> Vague commit error when server has been configured with <Location /svn/>
> and write through proxy.
How bad is the old error message? How much better is the new one? An
example might help. Does the change have any other effect?
> Votes:
> +1: kameshj
> * r917523
> ???
Ditto.
> Notes:
> This backport depends on the r878590 and r916286 groups only for the
> conflict free port.
> Justification:
> If the configured slave url has the *encodable* characters *write through*
> is not happening rather commit happens in slave itself.
Commit happens in the slave? That's certainly and obviously wrong. But
I don't understand what you mean by "the configured slave url has the
*encodable* characters".
> Votes:
> +1: kameshj
> * r935631
> Fix one of the sanity checks in the 'svn merge --reintegrate' logic.
What check? What did it do? What will it do after this change?
OK, I've looked at this one, and it appears that the check to ensure the
source and target are in the same repository was not being checked (it
was comparing target-repos with target-repos), and this change will make
it return an error if they are in different repositories, as was
intended. I'm not sure exactly what would happen if this condition were
not caught.
> Justification:
> Sanity checks should themselves be sane.
> Branch:
> ^/subversion/branches/1.6.x-r935631
> Votes:
> +1: cmpilato, pburba
> * r939375-939376
> Fix issue #3623, a bug in foreign repository merges that causes properties
> on merge-added files to be dropped from commits of those merges.
> Justification:
> This can actually result in what is arguably a corrupt working copy,
> one that thinks it is in sync with the repository but actually is not.
(Here's an example of a good description and justification.)
> * r950445, 950468
> Fix for issue #2591 "'svnadmin hotcopy' does not replicate symlinks".
> Justification:
> This issue is waiting for resolution for more than 3 years.
Length of time is not really a justification.
> Notes:
> r950445 fixes the issue and r950468 is a test for this issue.
> Votes:
> +1: stylesen
> * r952973, r952992
> Fix for issue #3651 "svn copy does not eat peg revision within copy target
> path"
> Justification:
> This issue exists in 1.6.x.
All of these issues exist in 1.6.x, that's why the fixes are proposed
for back-port.
> Notes:
> r952973 fixes the issue and r952992 is a test for this issue. Both r952973
> and r952992 are merged into the backport branch 1.6.x-issue3651.
> Use the private API in the 1.6.x branch since the API was made public in
> 1.7. The backport branch exists in order to resolve this.
> Branch:
> ^/subversion/branches/1.6.x-issue3651
> Votes:
> +1: stylesen, pburba
- Julian
Re: Info needed in 1.6.x STATUS file
Posted by Kamesh Jayachandran <ka...@collab.net>.
On 06/15/2010 10:51 PM, Julian Foad wrote:
> Hi devs. Going through the 1.6.x. STATUS file, I am finding some items
> that could do with a better description or justification to make it
> easier to evaluate them. Can anyone provide information on the
> following:
>
>
>
>> * r934599, r934603
>> Fix a concurrency issue in the FSFS rep-cache code. This indirectly
>> concerns issue #3506.
>> Justification:
>> Opening the DB twice (and overwriting the handle) is undesired.
>>
> Why is that undesired? I mean that as a serious practical question -
> obviously it's theoretically redundant but that in itself doesn't
> necessarily matter. What is the user-visible effect of the problem, and
> of the proposed change?
>
>
>> Notes:
>> r934599 updates the svn_atomic__init_once() interface.
>> r934603 is the actual fix.
>> Notes:
>> Depends on the r879966 group, which does not merge cleanly.
>> Branch:
>> ^/subversion/branches/1.6.x-issue3506-init_once
>> (based on ^/subversion/branches/1.6.x-issue3506)
>> Votes:
>> +1: danielsh
>>
>
>
>> * r878590, r878607, r878625, r878626, r878627
>> In trunk we optimized the common case of 'find-and-replace with same uri'
>> of proxied content thanks to issue 3445 "WebDAV proxy code munging user
>> data".
>>
> What is the purpose and effect of the change proposed for back-port?
>
Today with 1.6.11 write through proxy would corrupt the commit if the
url of master and slave(actually path portions of the request) are
different.
We avoid this problem by configuring the the path portions to be the same.
Even if we avoid this we end up parsing the commit body stream to find
and replace the same string with same string(i.e s/\/svn/\/svn/g).
This particular block of fixes is about *avoiding* finding and replace
of same thing with same thing and hence provide better response time.
>
>> r878590 is just a change that adds FIXME marker to code comment. We take it
>> to avoid spurious conflicts with other revisions.
>> r878607 Special cases no-op find and replace.
>> "r878625, r878626, r878627" are follow-up to r878607 and the other long
>> pending Master info leak to the clients.
>> Justification:
>> 1. This group has the most common 'optimization' fix of *not* groking the
>> proxied response to find and replace with same string.
>> 2. Fixes the master information leak via the Location header.
>> 3. Need this to be ported for the other defect to be ported
>> without conflict.
>> Votes:
>> +1: kameshj, cmpilato
>> -0: julianfoad (seem to be multiple changes here for different reasons -
>> at least issue '3445 and an optimization and an information leak;
>> r878607 log msg says it fixes bugs but it's not clear what bugs;
>> don't know how to tell whether justification 1 is significant;
>> justifications don't seem to refer to issue #3445. Please can we
>> separate these changes and clearly describe each one? And update
>> the r878607 log msg.)
>>
> Ah, I already put my concerns here.
>
>
>
>> * r916286, r917512
>> ???
>>
> What is the purpose and effect of the change proposed for back-port?
>
> (It was me who added the question marks, some time ago.)
>
Hope I made it clear now in the STATUS file in my recent commit.
>
>> Notes:
>> This backport depends on the r878590 group only for the conflict free port.
>> Justification:
>> Vague commit error when server has been configured with<Location /svn/>
>> and write through proxy.
>>
> How bad is the old error message? How much better is the new one? An
> example might help. Does the change have any other effect?
>
>
Hope I made it clear now in the STATUS file in my recent commit.
>> Votes:
>> +1: kameshj
>>
>
>
>> * r917523
>> ???
>>
> Ditto.
>
>
Hope I made it clear now in the STATUS file in my recent commit.
>> Notes:
>> This backport depends on the r878590 and r916286 groups only for the
>> conflict free port.
>> Justification:
>> If the configured slave url has the *encodable* characters *write through*
>> is not happening rather commit happens in slave itself.
>>
> Commit happens in the slave? That's certainly and obviously wrong. But
> I don't understand what you mean by "the configured slave url has the
> *encodable* characters".
>
Hope I made it clear now in the STATUS file in my recent commit.
With regards
Kamesh Jayachandran
Re: Info needed in 1.6.x STATUS file
Posted by Paul Burba <pt...@gmail.com>.
On Tue, Jun 15, 2010 at 1:21 PM, Julian Foad <ju...@wandisco.com> wrote:
> Hi devs. Going through the 1.6.x. STATUS file, I am finding some items
> that could do with a better description or justification to make it
> easier to evaluate them. Can anyone provide information on the
> following:
>
>
>> * r934599, r934603
>> Fix a concurrency issue in the FSFS rep-cache code. This indirectly
>> concerns issue #3506.
>> Justification:
>> Opening the DB twice (and overwriting the handle) is undesired.
>
> Why is that undesired? I mean that as a serious practical question -
> obviously it's theoretically redundant but that in itself doesn't
> necessarily matter. What is the user-visible effect of the problem, and
> of the proposed change?
>
>> Notes:
>> r934599 updates the svn_atomic__init_once() interface.
>> r934603 is the actual fix.
>> Notes:
>> Depends on the r879966 group, which does not merge cleanly.
>> Branch:
>> ^/subversion/branches/1.6.x-issue3506-init_once
>> (based on ^/subversion/branches/1.6.x-issue3506)
>> Votes:
>> +1: danielsh
>
>
>> * r878590, r878607, r878625, r878626, r878627
>> In trunk we optimized the common case of 'find-and-replace with same uri'
>> of proxied content thanks to issue 3445 "WebDAV proxy code munging user
>> data".
>
> What is the purpose and effect of the change proposed for back-port?
>
>> r878590 is just a change that adds FIXME marker to code comment. We take it
>> to avoid spurious conflicts with other revisions.
>> r878607 Special cases no-op find and replace.
>> "r878625, r878626, r878627" are follow-up to r878607 and the other long
>> pending Master info leak to the clients.
>> Justification:
>> 1. This group has the most common 'optimization' fix of *not* groking the
>> proxied response to find and replace with same string.
>> 2. Fixes the master information leak via the Location header.
>> 3. Need this to be ported for the other defect to be ported
>> without conflict.
>> Votes:
>> +1: kameshj, cmpilato
>> -0: julianfoad (seem to be multiple changes here for different reasons -
>> at least issue '3445 and an optimization and an information leak;
>> r878607 log msg says it fixes bugs but it's not clear what bugs;
>> don't know how to tell whether justification 1 is significant;
>> justifications don't seem to refer to issue #3445. Please can we
>> separate these changes and clearly describe each one? And update
>> the r878607 log msg.)
>
> Ah, I already put my concerns here.
>
>
>> * r916286, r917512
>> ???
>
> What is the purpose and effect of the change proposed for back-port?
>
> (It was me who added the question marks, some time ago.)
>
>> Notes:
>> This backport depends on the r878590 group only for the conflict free port.
>> Justification:
>> Vague commit error when server has been configured with <Location /svn/>
>> and write through proxy.
>
> How bad is the old error message? How much better is the new one? An
> example might help. Does the change have any other effect?
>
>> Votes:
>> +1: kameshj
>
>
>> * r917523
>> ???
>
> Ditto.
>
>> Notes:
>> This backport depends on the r878590 and r916286 groups only for the
>> conflict free port.
>> Justification:
>> If the configured slave url has the *encodable* characters *write through*
>> is not happening rather commit happens in slave itself.
>
> Commit happens in the slave? That's certainly and obviously wrong. But
> I don't understand what you mean by "the configured slave url has the
> *encodable* characters".
>
>> Votes:
>> +1: kameshj
>
>
>> * r935631
>> Fix one of the sanity checks in the 'svn merge --reintegrate' logic.
>
> What check? What did it do? What will it do after this change?
>
> OK, I've looked at this one, and it appears that the check to ensure the
> source and target are in the same repository was not being checked (it
> was comparing target-repos with target-repos), and this change will make
> it return an error if they are in different repositories, as was
> intended. I'm not sure exactly what would happen if this condition were
> not caught.
Hi Julian,
Fortunately even without r935631 any attempt to reintegrate a source
from a foreign repository fails. So this change is really about
providing a (much) better error mesage:
1.6.11:
>svn merge %REPOS_A_ROOT_URL%file:///C%3A/SVN/src-branch-1.6.x/Release/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-54.other/A_COPY A --reintegrate
svn: URL 'file:///C%3A/SVN/src-branch-1.6.x/Release/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-54.other/A_COPY'
is not a child of repository root URL
'file:///C%3A/SVN/src-branch-1.6.x/Release/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-54'
1.6.12 with Mike's patch:
>svn merge file:///C%3A/SVN/src-branch-1.6.x/Release/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-54.other/A_COPY A --reintegrate
svn: 'file:///C%3A/SVN/src-branch-1.6.x/Release/subversion/tests/cmdline/svn-test-work/repositories/merge_tests-54.other/A_COPY'
must be from the same repository as 'A'
I made a note of the above in STATUS.
Paul
>> Justification:
>> Sanity checks should themselves be sane.
>> Branch:
>> ^/subversion/branches/1.6.x-r935631
>> Votes:
>> +1: cmpilato, pburba
>
>
>> * r939375-939376
>> Fix issue #3623, a bug in foreign repository merges that causes properties
>> on merge-added files to be dropped from commits of those merges.
>> Justification:
>> This can actually result in what is arguably a corrupt working copy,
>> one that thinks it is in sync with the repository but actually is not.
>
> (Here's an example of a good description and justification.)
>
>
>> * r950445, 950468
>> Fix for issue #2591 "'svnadmin hotcopy' does not replicate symlinks".
>> Justification:
>> This issue is waiting for resolution for more than 3 years.
>
> Length of time is not really a justification.
>
>> Notes:
>> r950445 fixes the issue and r950468 is a test for this issue.
>> Votes:
>> +1: stylesen
>
>
>> * r952973, r952992
>> Fix for issue #3651 "svn copy does not eat peg revision within copy target
>> path"
>> Justification:
>> This issue exists in 1.6.x.
>
> All of these issues exist in 1.6.x, that's why the fixes are proposed
> for back-port.
>
>> Notes:
>> r952973 fixes the issue and r952992 is a test for this issue. Both r952973
>> and r952992 are merged into the backport branch 1.6.x-issue3651.
>> Use the private API in the 1.6.x branch since the API was made public in
>> 1.7. The backport branch exists in order to resolve this.
>> Branch:
>> ^/subversion/branches/1.6.x-issue3651
>> Votes:
>> +1: stylesen, pburba
>
>
> - Julian
>
>
>
Re: Info needed in 1.6.x STATUS file
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Julian Foad wrote on Tue, 15 Jun 2010 at 20:21 -0000:
> Hi devs. Going through the 1.6.x. STATUS file, I am finding some items
> that could do with a better description or justification to make it
> easier to evaluate them. Can anyone provide information on the
> following:
>
>
> > * r934599, r934603
> > Fix a concurrency issue in the FSFS rep-cache code. This indirectly
> > concerns issue #3506.
> > Justification:
> > Opening the DB twice (and overwriting the handle) is undesired.
>
> Why is that undesired? I mean that as a serious practical question -
> obviously it's theoretically redundant but that in itself doesn't
> necessarily matter. What is the user-visible effect of the problem, and
> of the proposed change?
Added a thread and some info to the STATUS entry --- hopefully that
clarifies the change a bit. Thanks for inquiring!
Re: Info needed in 1.6.x STATUS file
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
These are all good questions, and I would suggest that whoever answers
them does so in the STATUS file, rather than just on list.
Cheers,
-Hyrum
On Tue, Jun 15, 2010 at 12:21 PM, Julian Foad <ju...@wandisco.com> wrote:
> Hi devs. Going through the 1.6.x. STATUS file, I am finding some items
> that could do with a better description or justification to make it
> easier to evaluate them. Can anyone provide information on the
> following:
>
>
>> * r934599, r934603
>> Fix a concurrency issue in the FSFS rep-cache code. This indirectly
>> concerns issue #3506.
>> Justification:
>> Opening the DB twice (and overwriting the handle) is undesired.
>
> Why is that undesired? I mean that as a serious practical question -
> obviously it's theoretically redundant but that in itself doesn't
> necessarily matter. What is the user-visible effect of the problem, and
> of the proposed change?
>
>> Notes:
>> r934599 updates the svn_atomic__init_once() interface.
>> r934603 is the actual fix.
>> Notes:
>> Depends on the r879966 group, which does not merge cleanly.
>> Branch:
>> ^/subversion/branches/1.6.x-issue3506-init_once
>> (based on ^/subversion/branches/1.6.x-issue3506)
>> Votes:
>> +1: danielsh
>
>
>> * r878590, r878607, r878625, r878626, r878627
>> In trunk we optimized the common case of 'find-and-replace with same uri'
>> of proxied content thanks to issue 3445 "WebDAV proxy code munging user
>> data".
>
> What is the purpose and effect of the change proposed for back-port?
>
>> r878590 is just a change that adds FIXME marker to code comment. We take it
>> to avoid spurious conflicts with other revisions.
>> r878607 Special cases no-op find and replace.
>> "r878625, r878626, r878627" are follow-up to r878607 and the other long
>> pending Master info leak to the clients.
>> Justification:
>> 1. This group has the most common 'optimization' fix of *not* groking the
>> proxied response to find and replace with same string.
>> 2. Fixes the master information leak via the Location header.
>> 3. Need this to be ported for the other defect to be ported
>> without conflict.
>> Votes:
>> +1: kameshj, cmpilato
>> -0: julianfoad (seem to be multiple changes here for different reasons -
>> at least issue '3445 and an optimization and an information leak;
>> r878607 log msg says it fixes bugs but it's not clear what bugs;
>> don't know how to tell whether justification 1 is significant;
>> justifications don't seem to refer to issue #3445. Please can we
>> separate these changes and clearly describe each one? And update
>> the r878607 log msg.)
>
> Ah, I already put my concerns here.
>
>
>> * r916286, r917512
>> ???
>
> What is the purpose and effect of the change proposed for back-port?
>
> (It was me who added the question marks, some time ago.)
>
>> Notes:
>> This backport depends on the r878590 group only for the conflict free port.
>> Justification:
>> Vague commit error when server has been configured with <Location /svn/>
>> and write through proxy.
>
> How bad is the old error message? How much better is the new one? An
> example might help. Does the change have any other effect?
>
>> Votes:
>> +1: kameshj
>
>
>> * r917523
>> ???
>
> Ditto.
>
>> Notes:
>> This backport depends on the r878590 and r916286 groups only for the
>> conflict free port.
>> Justification:
>> If the configured slave url has the *encodable* characters *write through*
>> is not happening rather commit happens in slave itself.
>
> Commit happens in the slave? That's certainly and obviously wrong. But
> I don't understand what you mean by "the configured slave url has the
> *encodable* characters".
>
>> Votes:
>> +1: kameshj
>
>
>> * r935631
>> Fix one of the sanity checks in the 'svn merge --reintegrate' logic.
>
> What check? What did it do? What will it do after this change?
>
> OK, I've looked at this one, and it appears that the check to ensure the
> source and target are in the same repository was not being checked (it
> was comparing target-repos with target-repos), and this change will make
> it return an error if they are in different repositories, as was
> intended. I'm not sure exactly what would happen if this condition were
> not caught.
>
>> Justification:
>> Sanity checks should themselves be sane.
>> Branch:
>> ^/subversion/branches/1.6.x-r935631
>> Votes:
>> +1: cmpilato, pburba
>
>
>> * r939375-939376
>> Fix issue #3623, a bug in foreign repository merges that causes properties
>> on merge-added files to be dropped from commits of those merges.
>> Justification:
>> This can actually result in what is arguably a corrupt working copy,
>> one that thinks it is in sync with the repository but actually is not.
>
> (Here's an example of a good description and justification.)
>
>
>> * r950445, 950468
>> Fix for issue #2591 "'svnadmin hotcopy' does not replicate symlinks".
>> Justification:
>> This issue is waiting for resolution for more than 3 years.
>
> Length of time is not really a justification.
>
>> Notes:
>> r950445 fixes the issue and r950468 is a test for this issue.
>> Votes:
>> +1: stylesen
>
>
>> * r952973, r952992
>> Fix for issue #3651 "svn copy does not eat peg revision within copy target
>> path"
>> Justification:
>> This issue exists in 1.6.x.
>
> All of these issues exist in 1.6.x, that's why the fixes are proposed
> for back-port.
>
>> Notes:
>> r952973 fixes the issue and r952992 is a test for this issue. Both r952973
>> and r952992 are merged into the backport branch 1.6.x-issue3651.
>> Use the private API in the 1.6.x branch since the API was made public in
>> 1.7. The backport branch exists in order to resolve this.
>> Branch:
>> ^/subversion/branches/1.6.x-issue3651
>> Votes:
>> +1: stylesen, pburba
>
>
> - Julian
>
>
>