On 13.10.2016 14:56, Ivan Zhakov wrote:
> On 13 October 2016 at 14:00, Branko Čibej <brane_at_apache.org> wrote:
>> On 13.10.2016 13:28, Johan Corveleyn wrote:
>>> On Thu, Oct 13, 2016 at 1:22 PM, Ivan Zhakov <ivan_at_apache.org> wrote:
>>>> On 12 October 2016 at 22:30, Johan Corveleyn <jcorvel_at_gmail.com> wrote:
>>>>> On Wed, Oct 12, 2016 at 10:13 PM, <ivan_at_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
Received on 2016-10-13 15:27:32 CEST