Re: svn commit: r1242397 - in /subversion/trunk/subversion/libsvn_subr: skel.c stream.c

> From: Philip Martin <philip.martin_at_wandisco.com>
> To: Greg Stein <gstein_at_gmail.com>
> Cc: dev_at_subversion.apache.org
> Sent: Thursday, 9 February 2012, 18:13
> Subject: Re: svn commit: r1242397 - in /subversion/trunk/subversion/libsvn_subr: skel.c stream.c
>
>G reg Stein <gstein_at_gmail.com> writes:
>
>> I don't think this is the correct approach. svn_stringbuf_t is
>> *designed* to put a NUL at the end of the public length. Thus, it is
>> supposed to properly manage the +1 inside its functions.
>>
>> The correct fix is to put a ++minimum_size into svn_stringbuf_ensure()
>> rather than make callers worry about space for the private NUL
>> character.
>>
>> Please revert this commit. Code should not have to compensate for
>> stringbuf's internal concept.
>
> We would be changing long standing behaviour if we did this. I'd like it
> to be possible as it would remove the inconsistency between the current
> svn_stringbuf_ensure and svn_stringbuf_create_ensure.  I wonder if there
> are is any code that relies on the existing behaviour?  It seems
> unlikely that there is any real world code that would care.

The doc string for svn_stringbuf_ensure() has required "@a minimum_size should include space for the terminating NULL character" since before Subversion 1.0, so I stand by my commit.

I suggest we follow up by doing both of:

  * Change the implementation to always add an extra byte to that requested, to help buggy callers.  This is valid because the number passed in is already only a lower bound so callers can't assume that we don't increase it.

  * Rev the API to 'svn_stringbuf_ensure2()' and document that the caller doesn't have to allow for the trailing NUL.

Of course the implementation of _ensure() and _ensure2() would then be identical.  The point of adding one in the old _ensure() implementation is to help callers in third-party code, given that we've demonstrated a high chance of the existing API being misused.  The point of defining the _ensure2() API is so we can cleanly change callers to not add 1, without them contradicting the old API.  (It would add confusion for people reading the code if we removed the caller's "+1" and didn't rev the name.)

- Julian
Received on 2012-02-09 19:56:53 CET