On 7 September 2015 at 22:59, Lieven Govaerts <l...@mobsol.be> wrote: > On Mon, Sep 7, 2015 at 9:35 AM, Ivan Zhakov <i...@visualsvn.com> wrote: >> On 7 September 2015 at 00:25, Lieven Govaerts <l...@mobsol.be> wrote: >>> Hey Ivan, >>> >>> On Mon, Aug 31, 2015 at 3:20 PM, Ivan Zhakov <i...@visualsvn.com> wrote: >>>> On 6 April 2015 at 20:49, Ivan Zhakov <i...@visualsvn.com> wrote: >>>>> On 6 April 2015 at 19:02, Lieven Govaerts <l...@mobsol.be> wrote: >>>>>> On Mon, Apr 6, 2015 at 3:32 PM, Bert Huijben <b...@qqmail.nl> wrote: >>>>>>>> -----Original Message----- >>>>>>>> From: serf-...@googlegroups.com [mailto:serf-...@googlegroups.com] On >>>>>>>> Behalf Of s...@googlecode.com >>>>>>>> Sent: maandag 6 april 2015 11:25 >>>>>>>> To: serf-...@googlegroups.com >>>>>>>> Subject: [serf-dev] [serf] r2489 committed - In preparation of serf >>>>>>>> 1.4.0, >>>>>>>> remove the get_remaining function from t... >>>>>>>> >>>>>>>> Revision: 2489 >>>>>>>> Author: lieven.govaerts >>>>>>>> Date: Mon Apr 6 09:24:18 2015 UTC >>>>>>>> Log: In preparation of serf 1.4.0, remove the get_remaining >>>>>>>> function >>>>>>>> from the >>>>>>>> bucket API. >>>>>>>> >>>>>>>> This reverts most of r2008, r2009, r2010 and r2198. From r2008 I kept >>>>>>>> the >>>>>>>> read_bucket_v2 function, which is needed for set_config. >>>>>>> >>>>>>> If we still keep the read_bucket_v2 feature, what is the reason for >>>>>>> just removing get_remaining? >>>>>>> >>>>>> >>>>>> I'm fixing all TODO's as discussed in the summer last year. >>>>>> Get_remaining isn't finished yet and not used. So instead of waiting >>>>>> until someone uses it, I'm removing it now so we can get 1.4 branched. >>>>>> >>>>>> We can still revert this revision after 1.4.x is branched though. In >>>>>> fact, we can release 1.5.x with just this if an application wants to >>>>>> make use of the (finalized) get_remaining feature. >>>>>> >>>>> What problem do we have with get_remaining() feature except >>>>> read_bucket_v2() linkage problems? get_remaining() feature is not used >>>>> in Subversion for only one reason: serf-trunk has version 2.0.0 so >>>>> it's not possible to add version detection code. >>> >>> Huh? We always used a check on the next version of serf to check for >>> trunk code, why is that not possible anymore? >>> It's 2 years after you added the get_remaining API, and it's still not >>> used in Subversion. >>> >> Current version in serf trunk is 2.0.0. I posted patch to use >> get_remaining() API on Subversion list two years ago [1]. The only >> reason it wasn't committed because serf-trunk has version 2.0.0 >> instead of 1.4.0, so I cannot complete my patch with correct API >> check. > > Okay. >
>>>> >>>> I still don't understand your arguments on reverting get_remaining() >>>> feature. Why you consider get_remaining() as isn't finished? >>> >>> I consider it as not finished because many bucket types don't have a >>> get_remaining implementation, not even a default function, just NULL. >>> If you use such a bucket in an aggregate bucket, you'll get a quite nasty >>> crash. >>> >> Some buckets doesn't have get_remaining() implementation by design: >> not every bucket type know data length. And >> serf_bucket_get_remaining() returns SERF_LENGTH_UNKNOWN in this case. > > No it doesn't. > > +#define serf_bucket_get_remaining(b) \ > + ((b)->type->read_bucket == serf_buckets_are_v2 ? \ > + (b)->type->get_remaining(b) : \ > + SERF_LENGTH_UNKNOWN) > > The serf_bucket_get_remaining macro will return SERF_LENGTH_UNKNOWN > for non-v2 buckets. Your macro assumes that all v2 buckets implement > get_remaining! > To be fair to you, that was true at the time you wrote this code. But > now that the get_config API is there too, if we apply this > get_remaining patch again we'll have v2 buckets with NULL as > get_remaining pointer. So, the get_remaining() code was correct, but broken with config store implementation which changed all buckets to be v2? > So I'm against using NULL as value for function pointers in the bucket > definitions, to avoid problems like this. > > A simple fix would be to create a serf_default_get_remaining() which > returns SERF_LENGTH_UNKNOWN, and use that in all the serf bucket > types. > It seems to be good improvement anyway. >> Also I think it's better to report bug or fix it instead of reverting >> feature from trunk especially without prior discussion. > > This was discussed, in the thread initially planning the 1.4.0 release > more than a year ago. > > I just executed on my proposed plan to prepare for 1.4.0. This feature > wasn't vetoed or anything, my goal was to remove it from trunk > temporarily to get 1.4.0 out with features that were finished and > wanted. Obviously the assumption was that we would release 1.5.0 a > couple of months later with the get_remaining() API back in. We've > just been a bit slower than that. ;) > Ack. >>>> The read_bucket_v2() linking problem also apply to your serf_config_t >>>> feature, but we didn't reverted it from trunk. >>>> >>>> Also adding this feature latter will require read_bucket_v3() which >>>> increase the mess. > > Yeah, that's a bit annoying. If you prefer to reinstate this code, > with the default implementation of get_remaining, then I'm fine with > that. I'm going to reinstate get_remaining() code if you don't have objections: I think it useful functionality anyway. We still have to find way to fix read_bucket_v2() linkage problem, but it's unrelated problem. My current analysis is that making serf 2.0 is the only option to have v2 buckets reliable. >>> My guess was then, and still is, that this feature can wait a release. >>> I'm not against the feature, I'm against adding features that will not >>> be used. >>> >> Of course we should not block release because of this feature, but you >> reverted completed functionality from serf trunk with justification >> that it's not used by one of serf user (Subversion). But how >> Subversion should work if patch with use of serf_bucket_get_remaining >> was committed to Subversion and released in 1.9.0 and then Serf >> developers decide that serf_bucket_get_remaining() will not be >> released in serf 1.4.0? Subversion cannot depends on serf >> functionality that is not released, because serf developers may change >> everything before release. > > .. >> Subversion cannot rely on unreleased serf >> features. > > Agreed. > -- Ivan Zhakov
