You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Johan Corveleyn <jc...@gmail.com> on 2016/10/12 20:30:38 UTC
Re: svn commit: r1764536 - /subversion/branches/1.9.x/STATUS
On Wed, Oct 12, 2016 at 10:13 PM, <iv...@apache.org> wrote:
>
> Author: ivan
> Date: Wed Oct 12 20:13:24 2016
> New Revision: 1764536
>
> URL: http://svn.apache.org/viewvc?rev=1764536&view=rev
> Log:
> * STATUS: Veto r1759116 group.
>
> Modified:
> subversion/branches/1.9.x/STATUS
...
> @@ -135,6 +119,24 @@ Candidate changes:
> Veto-blocked changes:
> =====================
>
> + * r1759116, r1759117, r1759122, r1759123, r1759124
> + Fix FSFS format 7 packing for revisions packs with lots of changes.
> + Justification:
> + Problem occurred in at least two user repositories. Without the fix,
> + format 7 repositories with an exceptionally large number of changes in
> + a pack cannot be packed - which renders using f7 pointless for those
> + users.
> + Branch:
> + ^/subversion/branches/1.9.x-fsfs-pack-fixes
> + Notes:
> + r1759116 adds a workaround for trunc() corrupting apr_file_t internal state.
> + r1759117-23 provide the actual fixes.
> + r1759124 adds a regression test with the necessary internal API changes.
> + Votes:
> + +1: stefan2
> + -1: ivan (apr_file_trunc() bug should be reported and fixed in APR,
> + not by unreliable workaround with unknown consequences)
> +
Not picking any side here, but: "should be reported and fixed in APR"
does not sound like a veto-argument to me. It's a valid statement /
request, but not grounds for vetoing a workaround in our code. Perhaps
the bug was already reported to APR and not acted upon (I don't know
and don't really care WRT STATUS), or perhaps it will be reported
later but in the meantime can already be worked around in our code ...
but that's not a problem (we often implement workarounds in our code
for apr problems, and report them to apr, and later switch to the
official apr fix ...).
So that leaves you claiming that it's an "unreliable workaround with
unknown consequences". Why? Please clarify. We are talking about a
bugfix which fixes an important problem reported by users. Do you have
any suggestions for improvement? Perhaps the fix should be technically
discussed on the mailing list? Or should we just leave this important
problem unfixed?
--
Johan
Re: svn commit: r1764536 - /subversion/branches/1.9.x/STATUS
Posted by Branko Čibej <br...@apache.org>.
On 13.10.2016 14:56, Ivan Zhakov wrote:
> On 13 October 2016 at 14:00, Branko \u010cibej <br...@apache.org> wrote:
>> On 13.10.2016 13:28, Johan Corveleyn wrote:
>>> On Thu, Oct 13, 2016 at 1:22 PM, Ivan Zhakov <iv...@apache.org> wrote:
>>>> On 12 October 2016 at 22:30, Johan Corveleyn <jc...@gmail.com> wrote:
>>>>> On Wed, Oct 12, 2016 at 10:13 PM, <iv...@apache.org> wrote:
>>>>>> Author: ivan
>>>>>> Date: Wed Oct 12 20:13:24 2016
>>>>>> New Revision: 1764536
>>>>>>
>>>>>> URL: http://svn.apache.org/viewvc?rev=1764536&view=rev
>>>>>> Log:
>>>>>> * STATUS: Veto r1759116 group.
>>>>>>
>>>>>> Modified:
>>>>>> subversion/branches/1.9.x/STATUS
>>>>> ...
>>>>>> @@ -135,6 +119,24 @@ Candidate changes:
>>>>>> Veto-blocked changes:
>>>>>> =====================
>>>>>>
>>>>>> + * r1759116, r1759117, r1759122, r1759123, r1759124
>>>>>> + Fix FSFS format 7 packing for revisions packs with lots of changes.
>>>>>> + Justification:
>>>>>> + Problem occurred in at least two user repositories. Without the fix,
>>>>>> + format 7 repositories with an exceptionally large number of changes in
>>>>>> + a pack cannot be packed - which renders using f7 pointless for those
>>>>>> + users.
>>>>>> + Branch:
>>>>>> + ^/subversion/branches/1.9.x-fsfs-pack-fixes
>>>>>> + Notes:
>>>>>> + r1759116 adds a workaround for trunc() corrupting apr_file_t internal state.
>>>>>> + r1759117-23 provide the actual fixes.
>>>>>> + r1759124 adds a regression test with the necessary internal API changes.
>>>>>> + Votes:
>>>>>> + +1: stefan2
>>>>>> + -1: ivan (apr_file_trunc() bug should be reported and fixed in APR,
>>>>>> + not by unreliable workaround with unknown consequences)
>>>>>> +
>>>> [...]
>>>>> So that leaves you claiming that it's an "unreliable workaround with
>>>>> unknown consequences". Why? Please clarify. We are talking about a
>>>>> bugfix which fixes an important problem reported by users. Do you have
>>>>> any suggestions for improvement? Perhaps the fix should be technically
>>>>> discussed on the mailing list? Or should we just leave this important
>>>>> problem unfixed?
>>>> I think it's almost always better to solve the problem in its origin.
>>>> Any workaround is unreliable, so we should always try to fix the
>>>> problem at the root before trying the workarounds. Note that we are
>>>> talking about a relatively obvious bug in other Apache's project, not
>>>> about something irrecoverable.
>>>>
>>>> As far as I know, this problem has never been reported to the APR's
>>>> dev list. And this is the first action that we should have done.
>>>>
>>>>> Do you have any suggestions for improvement?
>>>> I have plans to send a patch to APR in order to solve this problem. So
>>>> we don't need to release this workaround.
>>> Okay, I understand this should be fixed in APR.
>>>
>>> But should we block a workaround in our code for a bug that causes
>>> problems in the wild for existing svn 1.9 installations? Those users
>>> deserve a fix in a 1.9.x patch release, IMHO. If you can arrange this
>>> by fixing it in APR, creating a patch release of APR, and then
>>> creating a 1.9.x svn release with that fix (in a reasonable
>>> timeframe), then fine. But otherwise I think a svn-internal-workaround
>>> is warranted.
>>
>> There's no chance of doing this in a reasonable time-frame because we
>> require APR-1.3 and no fix we can possibly come up with will be released
>> in a 1.3 patch.
>>
>> We have any number of workarounds for APR quirks in the code. I can't
>> think of a single valid reason to veto another one, especially since
>> we'd /still/ have to have the workaround in our code for people who use
>> older APR versions (likely versions prior to 1.5 or even 1.6, depending
>> on what the fix ends up looking like).
>>
>> "Any workaround is unreliable" is just nonsense, IMHO.
>>
> It seems that you are pulling my phrase out of context. Do you suggest
> to rely on workarounds instead of fixing problems at they roots? Note
> that this particular discussion is about our workaround, not about
> something officially suggested by APR devs.
We're talking about your vetoing a backport because it contains a
workaround for a bug in APR. Obviously we want to fix APR, but there's
no reason to stop doing what we've already done in a number of places in
our code. Your argument for the veto is that workarounds are unreliable
... well, they can be, and maybe this one is, but the general statement
is irrelevant to the discussion.
-- Brane
Re: svn commit: r1764536 - /subversion/branches/1.9.x/STATUS
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 13 October 2016 at 14:00, Branko Čibej <br...@apache.org> wrote:
> On 13.10.2016 13:28, Johan Corveleyn wrote:
>> On Thu, Oct 13, 2016 at 1:22 PM, Ivan Zhakov <iv...@apache.org> wrote:
>>> On 12 October 2016 at 22:30, Johan Corveleyn <jc...@gmail.com> wrote:
>>>> On Wed, Oct 12, 2016 at 10:13 PM, <iv...@apache.org> wrote:
>>>>> Author: ivan
>>>>> Date: Wed Oct 12 20:13:24 2016
>>>>> New Revision: 1764536
>>>>>
>>>>> URL: http://svn.apache.org/viewvc?rev=1764536&view=rev
>>>>> Log:
>>>>> * STATUS: Veto r1759116 group.
>>>>>
>>>>> Modified:
>>>>> subversion/branches/1.9.x/STATUS
>>>> ...
>>>>> @@ -135,6 +119,24 @@ Candidate changes:
>>>>> Veto-blocked changes:
>>>>> =====================
>>>>>
>>>>> + * r1759116, r1759117, r1759122, r1759123, r1759124
>>>>> + Fix FSFS format 7 packing for revisions packs with lots of changes.
>>>>> + Justification:
>>>>> + Problem occurred in at least two user repositories. Without the fix,
>>>>> + format 7 repositories with an exceptionally large number of changes in
>>>>> + a pack cannot be packed - which renders using f7 pointless for those
>>>>> + users.
>>>>> + Branch:
>>>>> + ^/subversion/branches/1.9.x-fsfs-pack-fixes
>>>>> + Notes:
>>>>> + r1759116 adds a workaround for trunc() corrupting apr_file_t internal state.
>>>>> + r1759117-23 provide the actual fixes.
>>>>> + r1759124 adds a regression test with the necessary internal API changes.
>>>>> + Votes:
>>>>> + +1: stefan2
>>>>> + -1: ivan (apr_file_trunc() bug should be reported and fixed in APR,
>>>>> + not by unreliable workaround with unknown consequences)
>>>>> +
>>> [...]
>>>> So that leaves you claiming that it's an "unreliable workaround with
>>>> unknown consequences". Why? Please clarify. We are talking about a
>>>> bugfix which fixes an important problem reported by users. Do you have
>>>> any suggestions for improvement? Perhaps the fix should be technically
>>>> discussed on the mailing list? Or should we just leave this important
>>>> problem unfixed?
>>> I think it's almost always better to solve the problem in its origin.
>>> Any workaround is unreliable, so we should always try to fix the
>>> problem at the root before trying the workarounds. Note that we are
>>> talking about a relatively obvious bug in other Apache's project, not
>>> about something irrecoverable.
>>>
>>> As far as I know, this problem has never been reported to the APR's
>>> dev list. And this is the first action that we should have done.
>>>
>>>> Do you have any suggestions for improvement?
>>> I have plans to send a patch to APR in order to solve this problem. So
>>> we don't need to release this workaround.
>> Okay, I understand this should be fixed in APR.
>>
>> But should we block a workaround in our code for a bug that causes
>> problems in the wild for existing svn 1.9 installations? Those users
>> deserve a fix in a 1.9.x patch release, IMHO. If you can arrange this
>> by fixing it in APR, creating a patch release of APR, and then
>> creating a 1.9.x svn release with that fix (in a reasonable
>> timeframe), then fine. But otherwise I think a svn-internal-workaround
>> is warranted.
>
>
> There's no chance of doing this in a reasonable time-frame because we
> require APR-1.3 and no fix we can possibly come up with will be released
> in a 1.3 patch.
>
> We have any number of workarounds for APR quirks in the code. I can't
> think of a single valid reason to veto another one, especially since
> we'd /still/ have to have the workaround in our code for people who use
> older APR versions (likely versions prior to 1.5 or even 1.6, depending
> on what the fix ends up looking like).
>
> "Any workaround is unreliable" is just nonsense, IMHO.
>
It seems that you are pulling my phrase out of context. Do you suggest
to rely on workarounds instead of fixing problems at they roots? Note
that this particular discussion is about our workaround, not about
something officially suggested by APR devs.
--
Ivan Zhakov
RE: svn commit: r1764536 - /subversion/branches/1.9.x/STATUS
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Branko Čibej [mailto:brane@apache.org]
> Sent: donderdag 13 oktober 2016 14:00
> To: dev@subversion.apache.org
> Subject: Re: svn commit: r1764536 - /subversion/branches/1.9.x/STATUS
> There's no chance of doing this in a reasonable time-frame because we
> require APR-1.3 and no fix we can possibly come up with will be released
> in a 1.3 patch.
As long as nobody even reported the bug on APR on any apr list/group/anything, there won't be a fix... ever...
Please, can we at least prioritize reporting a problem in apr and preferable fixing it there over doing workarounds that we have to maintain forever.
After apr is patched I would like to see the workaround code conditionalized on being compiled against older versions of apr.
Note I haven't looked at the patch in detail, but it looks like it changes a quite generic function by introducing new race conditions in certain error cases. Not something I would like to see in our code forever
Bert
Re: svn commit: r1764536 - /subversion/branches/1.9.x/STATUS
Posted by Branko Čibej <br...@apache.org>.
On 13.10.2016 13:28, Johan Corveleyn wrote:
> On Thu, Oct 13, 2016 at 1:22 PM, Ivan Zhakov <iv...@apache.org> wrote:
>> On 12 October 2016 at 22:30, Johan Corveleyn <jc...@gmail.com> wrote:
>>> On Wed, Oct 12, 2016 at 10:13 PM, <iv...@apache.org> wrote:
>>>> Author: ivan
>>>> Date: Wed Oct 12 20:13:24 2016
>>>> New Revision: 1764536
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=1764536&view=rev
>>>> Log:
>>>> * STATUS: Veto r1759116 group.
>>>>
>>>> Modified:
>>>> subversion/branches/1.9.x/STATUS
>>> ...
>>>> @@ -135,6 +119,24 @@ Candidate changes:
>>>> Veto-blocked changes:
>>>> =====================
>>>>
>>>> + * r1759116, r1759117, r1759122, r1759123, r1759124
>>>> + Fix FSFS format 7 packing for revisions packs with lots of changes.
>>>> + Justification:
>>>> + Problem occurred in at least two user repositories. Without the fix,
>>>> + format 7 repositories with an exceptionally large number of changes in
>>>> + a pack cannot be packed - which renders using f7 pointless for those
>>>> + users.
>>>> + Branch:
>>>> + ^/subversion/branches/1.9.x-fsfs-pack-fixes
>>>> + Notes:
>>>> + r1759116 adds a workaround for trunc() corrupting apr_file_t internal state.
>>>> + r1759117-23 provide the actual fixes.
>>>> + r1759124 adds a regression test with the necessary internal API changes.
>>>> + Votes:
>>>> + +1: stefan2
>>>> + -1: ivan (apr_file_trunc() bug should be reported and fixed in APR,
>>>> + not by unreliable workaround with unknown consequences)
>>>> +
>> [...]
>>> So that leaves you claiming that it's an "unreliable workaround with
>>> unknown consequences". Why? Please clarify. We are talking about a
>>> bugfix which fixes an important problem reported by users. Do you have
>>> any suggestions for improvement? Perhaps the fix should be technically
>>> discussed on the mailing list? Or should we just leave this important
>>> problem unfixed?
>> I think it's almost always better to solve the problem in its origin.
>> Any workaround is unreliable, so we should always try to fix the
>> problem at the root before trying the workarounds. Note that we are
>> talking about a relatively obvious bug in other Apache's project, not
>> about something irrecoverable.
>>
>> As far as I know, this problem has never been reported to the APR's
>> dev list. And this is the first action that we should have done.
>>
>>> Do you have any suggestions for improvement?
>> I have plans to send a patch to APR in order to solve this problem. So
>> we don't need to release this workaround.
> Okay, I understand this should be fixed in APR.
>
> But should we block a workaround in our code for a bug that causes
> problems in the wild for existing svn 1.9 installations? Those users
> deserve a fix in a 1.9.x patch release, IMHO. If you can arrange this
> by fixing it in APR, creating a patch release of APR, and then
> creating a 1.9.x svn release with that fix (in a reasonable
> timeframe), then fine. But otherwise I think a svn-internal-workaround
> is warranted.
There's no chance of doing this in a reasonable time-frame because we
require APR-1.3 and no fix we can possibly come up with will be released
in a 1.3 patch.
We have any number of workarounds for APR quirks in the code. I can't
think of a single valid reason to veto another one, especially since
we'd /still/ have to have the workaround in our code for people who use
older APR versions (likely versions prior to 1.5 or even 1.6, depending
on what the fix ends up looking like).
"Any workaround is unreliable" is just nonsense, IMHO.
-- Brane
Re: svn commit: r1764536 - /subversion/branches/1.9.x/STATUS
Posted by Johan Corveleyn <jc...@gmail.com>.
On Thu, Oct 13, 2016 at 1:22 PM, Ivan Zhakov <iv...@apache.org> wrote:
> On 12 October 2016 at 22:30, Johan Corveleyn <jc...@gmail.com> wrote:
>> On Wed, Oct 12, 2016 at 10:13 PM, <iv...@apache.org> wrote:
>>>
>>> Author: ivan
>>> Date: Wed Oct 12 20:13:24 2016
>>> New Revision: 1764536
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1764536&view=rev
>>> Log:
>>> * STATUS: Veto r1759116 group.
>>>
>>> Modified:
>>> subversion/branches/1.9.x/STATUS
>> ...
>>> @@ -135,6 +119,24 @@ Candidate changes:
>>> Veto-blocked changes:
>>> =====================
>>>
>>> + * r1759116, r1759117, r1759122, r1759123, r1759124
>>> + Fix FSFS format 7 packing for revisions packs with lots of changes.
>>> + Justification:
>>> + Problem occurred in at least two user repositories. Without the fix,
>>> + format 7 repositories with an exceptionally large number of changes in
>>> + a pack cannot be packed - which renders using f7 pointless for those
>>> + users.
>>> + Branch:
>>> + ^/subversion/branches/1.9.x-fsfs-pack-fixes
>>> + Notes:
>>> + r1759116 adds a workaround for trunc() corrupting apr_file_t internal state.
>>> + r1759117-23 provide the actual fixes.
>>> + r1759124 adds a regression test with the necessary internal API changes.
>>> + Votes:
>>> + +1: stefan2
>>> + -1: ivan (apr_file_trunc() bug should be reported and fixed in APR,
>>> + not by unreliable workaround with unknown consequences)
>>> +
>>
> [...]
>> So that leaves you claiming that it's an "unreliable workaround with
>> unknown consequences". Why? Please clarify. We are talking about a
>> bugfix which fixes an important problem reported by users. Do you have
>> any suggestions for improvement? Perhaps the fix should be technically
>> discussed on the mailing list? Or should we just leave this important
>> problem unfixed?
>
> I think it's almost always better to solve the problem in its origin.
> Any workaround is unreliable, so we should always try to fix the
> problem at the root before trying the workarounds. Note that we are
> talking about a relatively obvious bug in other Apache's project, not
> about something irrecoverable.
>
> As far as I know, this problem has never been reported to the APR's
> dev list. And this is the first action that we should have done.
>
>> Do you have any suggestions for improvement?
> I have plans to send a patch to APR in order to solve this problem. So
> we don't need to release this workaround.
Okay, I understand this should be fixed in APR.
But should we block a workaround in our code for a bug that causes
problems in the wild for existing svn 1.9 installations? Those users
deserve a fix in a 1.9.x patch release, IMHO. If you can arrange this
by fixing it in APR, creating a patch release of APR, and then
creating a 1.9.x svn release with that fix (in a reasonable
timeframe), then fine. But otherwise I think a svn-internal-workaround
is warranted.
--
Johan
Re: svn commit: r1764536 - /subversion/branches/1.9.x/STATUS
Posted by Ivan Zhakov <iv...@apache.org>.
On 12 October 2016 at 22:30, Johan Corveleyn <jc...@gmail.com> wrote:
> On Wed, Oct 12, 2016 at 10:13 PM, <iv...@apache.org> wrote:
>>
>> Author: ivan
>> Date: Wed Oct 12 20:13:24 2016
>> New Revision: 1764536
>>
>> URL: http://svn.apache.org/viewvc?rev=1764536&view=rev
>> Log:
>> * STATUS: Veto r1759116 group.
>>
>> Modified:
>> subversion/branches/1.9.x/STATUS
> ...
>> @@ -135,6 +119,24 @@ Candidate changes:
>> Veto-blocked changes:
>> =====================
>>
>> + * r1759116, r1759117, r1759122, r1759123, r1759124
>> + Fix FSFS format 7 packing for revisions packs with lots of changes.
>> + Justification:
>> + Problem occurred in at least two user repositories. Without the fix,
>> + format 7 repositories with an exceptionally large number of changes in
>> + a pack cannot be packed - which renders using f7 pointless for those
>> + users.
>> + Branch:
>> + ^/subversion/branches/1.9.x-fsfs-pack-fixes
>> + Notes:
>> + r1759116 adds a workaround for trunc() corrupting apr_file_t internal state.
>> + r1759117-23 provide the actual fixes.
>> + r1759124 adds a regression test with the necessary internal API changes.
>> + Votes:
>> + +1: stefan2
>> + -1: ivan (apr_file_trunc() bug should be reported and fixed in APR,
>> + not by unreliable workaround with unknown consequences)
>> +
>
[...]
> So that leaves you claiming that it's an "unreliable workaround with
> unknown consequences". Why? Please clarify. We are talking about a
> bugfix which fixes an important problem reported by users. Do you have
> any suggestions for improvement? Perhaps the fix should be technically
> discussed on the mailing list? Or should we just leave this important
> problem unfixed?
I think it's almost always better to solve the problem in its origin.
Any workaround is unreliable, so we should always try to fix the
problem at the root before trying the workarounds. Note that we are
talking about a relatively obvious bug in other Apache's project, not
about something irrecoverable.
As far as I know, this problem has never been reported to the APR's
dev list. And this is the first action that we should have done.
> Do you have any suggestions for improvement?
I have plans to send a patch to APR in order to solve this problem. So
we don't need to release this workaround.
--
Ivan Zhakov