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 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. > 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. ;) > >>> 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. >>> >> 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. regards, Lieven > > [1] http://svn.haxx.se/dev/archive-2013-07/0133.shtml > > -- > Ivan Zhakov
