On 06.01.2019 15:05, Branko Čibej wrote:
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;
good point.
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:
see attached patch.
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.
problem is: if we just skip any null char after an lf, then we would
screw up utf16be encodings (or le? I always get those two confused).
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.
another situation which unfortunately is not that uncommon: files that
have different encodings in different lines or even inside the same
line. I've seen that especially in log files where the logger first
prints timestamp in ascii, but then the log-text is in utf8 or even
utf16. Such situations I think would require too much detection logic
for the blame function and should be left to clients - they might not be
able to even show such lines so why bother trying to do the detection
right for that.
Stefan
[[[
Extend the blame callback with a string length parameter.
* subversion/incluce/svn_client.h
* subversion/libsvn_client/blame.c
(svn_client_blame_receiver4_t): typedef for new callback
(svn_client_blame6): new API using the svn_client_blame_receiver4_t callback
* subversion/libsvn_client/deprecated.c
(svn_client_blame5): moved API there, calling svn_client_blame6 using a
callback shim
(blame_wrapper_receiver3): callback shim for svn_client_blame5
]]]
Index: subversion/include/svn_client.h
===================================================================
--- subversion/include/svn_client.h (revision 1850484)
+++ subversion/include/svn_client.h (working copy)
@@ -736,7 +736,7 @@
* @{
*/
-/** Callback type used by svn_client_blame5() to notify the caller
+/** Callback type used by svn_client_blame6() to notify the caller
* that line @a line_no of the blamed file was last changed in @a revision
* which has the revision properties @a rev_props, and that the contents were
* @a line.
@@ -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,
+ const svn_string_t *line,
+ svn_boolean_t local_change,
+ apr_pool_t *pool);
+
+/**
+ * Similar to #svn_client_blame_receiver4_t, but with the line parameter
+ * as a (const char*) instead of an svn_string_t.
+ *
+ * @deprecated Provided for backward compatibility with the 1.11 API.
+ *
* @since New in 1.7.
*/
typedef svn_error_t *(*svn_client_blame_receiver3_t)(
@@ -2928,6 +2950,28 @@
*
* Use @a pool for any temporary allocation.
*
+ * @since New in 1.12.
+ */
+svn_error_t *
+svn_client_blame6(const char *path_or_url,
+ const svn_opt_revision_t *peg_revision,
+ const svn_opt_revision_t *start,
+ const svn_opt_revision_t *end,
+ const svn_diff_file_options_t *diff_options,
+ svn_boolean_t ignore_mime_type,
+ svn_boolean_t include_merged_revisions,
+ svn_client_blame_receiver4_t receiver,
+ void *receiver_baton,
+ svn_client_ctx_t *ctx,
+ apr_pool_t *pool);
+
+
+/**
+ * Similar to svn_client_blame6(), but with #svn_client_blame_receiver3_t
+ * as the receiver.
+ *
+ * @deprecated Provided for backwards compatibility with the 1.11 API.
+ *
* @since New in 1.7.
*/
svn_error_t *
@@ -2943,9 +2987,8 @@
svn_client_ctx_t *ctx,
apr_pool_t *pool);
-
/**
- * Similar to svn_client_blame5(), but with #svn_client_blame_receiver3_t
+ * Similar to svn_client_blame5(), but with #svn_client_blame_receiver2_t
* as the receiver.
*
* @deprecated Provided for backwards compatibility with the 1.6 API.
Index: subversion/libsvn_client/blame.c
===================================================================
--- subversion/libsvn_client/blame.c (revision 1850484)
+++ subversion/libsvn_client/blame.c (working copy)
@@ -656,7 +656,7 @@
}
svn_error_t *
-svn_client_blame5(const char *target,
+svn_client_blame6(const char *target,
const svn_opt_revision_t *peg_revision,
const svn_opt_revision_t *start,
const svn_opt_revision_t *end,
@@ -663,7 +663,7 @@
const svn_diff_file_options_t *diff_options,
svn_boolean_t ignore_mime_type,
svn_boolean_t include_merged_revisions,
- svn_client_blame_receiver3_t receiver,
+ svn_client_blame_receiver4_t receiver,
void *receiver_baton,
svn_client_ctx_t *ctx,
apr_pool_t *pool)
@@ -676,6 +676,7 @@
svn_stream_t *last_stream;
svn_stream_t *stream;
const char *target_abspath_or_url;
+ svn_string_t line;
if (start->kind == svn_opt_revision_unspecified
|| end->kind == svn_opt_revision_unspecified)
@@ -941,18 +942,20 @@
SVN_ERR(ctx->cancel_func(ctx->cancel_baton));
if (!eof || sb->len)
{
+ line.data = sb->data;
+ line.len = 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;
}
Index: subversion/libsvn_client/deprecated.c
===================================================================
--- subversion/libsvn_client/deprecated.c (revision 1850484)
+++ subversion/libsvn_client/deprecated.c (working copy)
@@ -166,7 +166,59 @@
}
/*** From blame.c ***/
+struct blame_receiver_wrapper_baton3 {
+ void *baton;
+ svn_client_blame_receiver3_t receiver;
+};
+static svn_error_t *
+blame_wrapper_receiver3(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,
+ const svn_string_t *line,
+ svn_boolean_t local_change,
+ apr_pool_t *pool)
+{
+ struct blame_receiver_wrapper_baton3 *brwb = baton;
+
+ if (brwb->receiver)
+ return brwb->receiver(brwb->baton, start_revnum, end_revnum, line_no,
+ revision, rev_props, merged_revision,
+ merged_rev_props, merged_path, line->data,
+ local_change, pool);
+
+ return SVN_NO_ERROR;
+}
+
+svn_error_t *
+svn_client_blame5(const char *target,
+ const svn_opt_revision_t *peg_revision,
+ const svn_opt_revision_t *start,
+ const svn_opt_revision_t *end,
+ const svn_diff_file_options_t *diff_options,
+ svn_boolean_t ignore_mime_type,
+ svn_boolean_t include_merged_revisions,
+ svn_client_blame_receiver3_t receiver,
+ void *receiver_baton,
+ svn_client_ctx_t *ctx,
+ apr_pool_t *pool)
+{
+ struct blame_receiver_wrapper_baton3 baton;
+
+ baton.receiver = receiver;
+ baton.baton = receiver_baton;
+
+ return svn_client_blame6(target, peg_revision, start, end, diff_options,
+ ignore_mime_type, include_merged_revisions,
+ blame_wrapper_receiver3, &baton, ctx, pool);
+}
+
struct blame_receiver_wrapper_baton2 {
void *baton;
svn_client_blame_receiver2_t receiver;
