On 06.01.2019 14:10, Stefan Kueng wrote:
>
>
> On 05.01.2019 22:35, Daniel Shahaf wrote:
>> Stefan Kueng wrote on Sat, 05 Jan 2019 21:15 +0100:
>>> here's a patch using svn_stringbuf_t for review.
>>
>> Change "Provided for backwards compatibility with the 1.6 API" to
>> "Provided for backwards compatibility with the 1.11 API" in
>> svn_client_blame5()'s docstring.
>>
>
> done. See attached patch.
Sorry about getting into this late, but as Julian said:
> * we already have a (char*, len) wrapper, called svn_string_t, so I would
> expect it would be neatest to use that;
but the patch has:
> @@ -758,6 +758,28 @@
> ·*·will·be·true·if·the·reason·there·is·no·blame·information·is·that·the·line
>
> ·*·was·modified·locally.·In·all·other·cases·@a·local_change·will·be·false.
>
> ·*
> +·*·@since·New·in·1.12.
> +·*/
> +typedef·svn_error_t·*(*svn_client_blame_receiver4_t)(
> +··void·*baton,
> +··svn_revnum_t·start_revnum,
> +··svn_revnum_t·end_revnum,
> +··apr_int64_t·line_no,
> +··svn_revnum_t·revision,
> +··apr_hash_t·*rev_props,
> +··svn_revnum_t·merged_revision,
> +··apr_hash_t·*merged_rev_props,
> +··const·char·*merged_path,
> +··svn_stringbuf_t·*line,
> +··svn_boolean_t·local_change,
> +··apr_pool_t·*pool);
The svn_stringbuf_t struct is not appropriate here; please use
svn_string_t. The former is used as a buffer to construct larger
strings, that's why it contains a reference to a pool and a blocksize.
the blame receiver gets data that are already allocated from a pool and
should be immutable to the receiver; the appropriate declaration here is
const svn_string_t *line;
as you only need the data pointer and the length, and very much do _not_
need the pool and blocksize, nor do you want the data or length to be
modifiable by the callback.
That will make the changes in libsvn_client/blame.c a bit more complex
since you'll have to create a local svn_string_t object (on stack)
before calling the receiver, but it makes the receiver's interface
cleaner. Something like this would work:
@@ -941,18 +941,20 @@ svn_client_blame5(const char *target,
SVN_ERR(ctx->cancel_func(ctx->cancel_baton));
if (!eof || sb->len)
{
+ const svn_string_t line = {sb->data, sb->len};
+
if (walk->rev)
SVN_ERR(receiver(receiver_baton, start_revnum, end_revnum,
line_no, walk->rev->revision,
walk->rev->rev_props, merged_rev,
merged_rev_props, merged_path,
- sb->data, FALSE, iterpool));
+ &line, FALSE, iterpool));
else
SVN_ERR(receiver(receiver_baton, start_revnum, end_revnum,
line_no, SVN_INVALID_REVNUM,
NULL, SVN_INVALID_REVNUM,
NULL, NULL,
- sb->data, TRUE, iterpool));
+ &line, TRUE, iterpool));
}
if (eof) break;
}
Other than that, I think it's a bad idea to leave the trailing null byte
from the UTF-16-LE representation of U+000a at the beginning of the next
"line". If we're making this change in a *public* API, we should not
expect all users to hack around such a misfeature.
Until we have better support for other Unicode representations, we
should detect that null byte and include it as part of the reported
blame line.
-- Brane
