>> @@ -836,6 +836,10 @@
>> SVN_FS__SQLITE_ERR(sqlite3_bind_int64(stmt, 2, min_commit_rev+1), db);
>> SVN_FS__SQLITE_ERR(sqlite3_bind_int64(stmt, 3, max_commit_rev), db);
>> sqlite_result = sqlite3_step(stmt);
>> +
>> + if (!(sqlite_result == SQLITE_ROW || sqlite_result == SQLITE_DONE))
>> + return svn_fs__sqlite_stmt_error(stmt);
>> +
>>
>
> We perform this same check 3 times. How about a macro, something
> like:
>
> #define SQLITE_CURSOR_STEP_OK(sqlite_result) \
> (sqlite_result == SQLITE_ROW || sqlite_result == SQLITE_DONE)
>
> That'd be a lot more comprehensible, too.
>
Recent synch up from /trunk(r28152) made me to use 'svn_fs__sqlite_step'
and hence no more of this duplication.
>
>> SVN_FS__SQLITE_ERR(sqlite3_reset(stmt), db);
>> if (sqlite_result == SQLITE_ROW)
>> {
>> @@ -844,22 +848,31 @@
>> }
>> else
>> {
>> - const char *parent_path = svn_path_dirname(merge_target, pool);
>> + apr_pool_t *subpool = svn_pool_create(pool);
>> + const char *parent_path = svn_path_dirname(merge_target, subpool);
>> while (strcmp(parent_path, "/"))
>>
> ^
> I'd like to see a "!= 0" in this conditional.
>
Karl has fixed this via r28158.
>> else
>> return svn_error_createf(SVN_ERR_FS_SQLITE_ERROR, NULL,
>> - _("No mergeinfo for %s between revisions %ld:%ld"),
>> - merge_target, min_commit_rev, max_commit_rev);
>> + _("No mergeinfo for '%s' between revisions \
>> + %ld:%ld"),
>>
>
> What is the trailing \ for, line continuation? We typically use ",
> followed by another " at the beginning of the text on the next line
> (as shown later in this diff).
>
> Also, how about the description "in revision range", rather than
> "between revisions"?
>
>
>
Fixed in 28205.
Thanks
With regards
Kamesh Jayachandran
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Dec 3 15:26:12 2007