Using terminology from <http://s.apache.org/xy-problem>: if X is "I am
not comfortable writings loops in DOS batch", I don't think applying
your patch to trunk is the correct Y.
Prabhu Gnana Sundar wrote on Thu, Oct 25, 2012 at 20:33:17 +0530:
>
> Thanks Daniel, these are really good pointers for me. Shall I continue
> with this and submit a patch
> with the changes.
>
> Regards
> Prabhu
>
>
>
> On 10/21/2012 10:42 PM, Daniel Shahaf wrote:
>> BTW, I'd like to point out a few further problems with the patch; even
>> if it doesn't get applied, you might find them useful for your next
>> patch.
>>
>> Prabhu Gnana Sundar wrote on Sun, Oct 21, 2012 at 00:57:47 +0530:
>>> Index: subversion/libsvn_repos/dump.c
>>> ===================================================================
>>> --- subversion/libsvn_repos/dump.c (revision 1398254)
>>> +++ subversion/libsvn_repos/dump.c (working copy)
>>> @@ -1397,6 +1398,7 @@
>>> end_rev, youngest);
>>>
>>> /* Verify global/auxiliary data and backend-specific data first. */
>>> + if (!keep_going)
>>> SVN_ERR(svn_fs_verify(svn_fs_path(fs, pool), cancel_func, cancel_baton,
>>> start_rev, end_rev, pool));
>> This hunk is wrong, it causes work to _not be done_, not only to not be
>> a fatal error.
>>
>>>
>>> @@ -1425,7 +1428,21 @@
>>> notify_func, notify_baton,
>>> start_rev,
>>> FALSE, TRUE, /* use_deltas, verify */
>>> - iterpool));
>>> + iterpool);
>>> + if (err&& keep_going)
>>> + {
>> Wrong indentation.
>>
>>> + svn_repos_notify_t *notify2 =
>>> svn_repos_notify_create(svn_repos_notify_failure, iterpool);
>>> +
>>> + notify2->warning_str = _(err->message);
>>> + notify2->apr_err = _(err->apr_err);
>> Type mismatch (because _ takes a 'const char *' parameter, and apr_err is
>> an integral type), probably segfault under --enable-nls.
>>
>>> + notify2->child_apr_err = _(err->child->apr_err);
>> Segfault whenever err->child happens to be NULL. (this will almost
>> never happen in maintainer builds)
>>
>> OTOH, maintainer builds probably want to skip tracing links here (see
>> for example svn_error_purge_tracing()).
>>
>> Why not stow the entire error chain in the notification? Why just the
>> topmost error and the first child?
>>
>>> + notify2->child_warning_str = _(err->child->message);
>>> + notify_func(notify_baton, notify2, iterpool);
>>> + continue;
>> Error leak (ERR needs to be cleared) --- this should have caused svn to
>> assert at exit if you ran this block in maintainer mode.
>>
>>> + }
>>> + else
>>> + SVN_ERR(err);
>>> +
>>> SVN_ERR(svn_delta_get_cancellation_editor(cancel_func, cancel_baton,
>>> dump_editor,
>>> dump_edit_baton,
>>> &cancel_editor,
>>> Index: subversion/include/svn_repos.h
>>> ===================================================================
>>> --- subversion/include/svn_repos.h (revision 1398254)
>>> +++ subversion/include/svn_repos.h (working copy)
>>> + * @deprecated Provided for backward compatibility with the 1.7 API.
>>> */
>>> +
>> Missing SVN_DEPRECATED decorator.
>>
>>> svn_error_t *
>>> svn_repos_verify_fs2(svn_repos_t *repos,
>>> svn_revnum_t start_rev,
>> The rest looks good.
>>
>> Cheers,
>>
>> Daniel
>
