Thank you so much Stefan for your patience in reviewing and guiding me
through.
I have addressed your points in the patch attached in this mail. I hope
I addressed all the suggestions given by you :)
Please share your views.
I will come up with a test case for this implementation in a new patch.
Thanks and regards
Prabhu
On 10/29/2012 05:01 PM, Stefan Sperling wrote:
> On Mon, Oct 29, 2012 at 03:26:36PM +0530, Prabhu Gnana Sundar wrote:
>> Thanks much Stefan,
>>
>> Now, I have changed the code in libsvn_fs/fs-loader.c to handle the
>> error. Passed the keep_going to the verify_fs so that it can be
>> handled in both libsvn_fs_fs and libsvn_fs_base. I have attached the
>> updated patch with this mail. Please share your reviews.
> More review below (mostly stylistic nits).
>
>> Index: subversion/libsvn_repos/dump.c
>> ===================================================================
>> --- subversion/libsvn_repos/dump.c (revision 1402414)
>> +++ subversion/libsvn_repos/dump.c (working copy)
>> @@ -1360,9 +1360,10 @@
>> }
>>
>> svn_error_t *
>> -svn_repos_verify_fs2(svn_repos_t *repos,
>> +svn_repos_verify_fs3(svn_repos_t *repos,
>> svn_revnum_t start_rev,
>> svn_revnum_t end_rev,
>> + svn_boolean_t keep_going,
>> svn_repos_notify_func_t notify_func,
>> void *notify_baton,
>> svn_cancel_func_t cancel_func,
>> @@ -1398,7 +1399,7 @@
>>
>> /* Verify global/auxiliary data and backend-specific data first. */
>> SVN_ERR(svn_fs_verify(svn_fs_path(fs, pool), cancel_func, cancel_baton,
>> - start_rev, end_rev, pool));
>> + start_rev, end_rev, keep_going, pool));
>>
>> /* Create a notify object that we can reuse within the loop. */
>> if (notify_func)
>> @@ -1413,11 +1414,12 @@
>> void *cancel_edit_baton;
>> svn_fs_root_t *to_root;
>> apr_hash_t *props;
>> + svn_error_t *err;
>>
>> svn_pool_clear(iterpool);
>>
>> /* Get cancellable dump editor, but with our close_directory handler. */
>> - SVN_ERR(get_dump_editor(&dump_editor,&dump_edit_baton,
>> + err = get_dump_editor(&dump_editor,&dump_edit_baton,
>> fs, rev, "",
>> svn_stream_empty(iterpool),
>> NULL, NULL,
> Please also change indentation of function parameters on additional
> lines to align at the opening bracket like this:
>
> err = get_dump_editor(&dump_editor,&dump_edit_baton,
> fs, rev, "",
> svn_stream_empty(iterpool),
> NULL, NULL,
>
> With your patch it would look like this, which is wrong indentation
> for our code base:
>
> err = get_dump_editor(&dump_editor,&dump_edit_baton,
> fs, rev, "",
> svn_stream_empty(iterpool),
> NULL, NULL,
>
>> @@ -1425,7 +1427,19 @@
>> notify_func, notify_baton,
>> start_rev,
>> FALSE, TRUE, /* use_deltas, verify */
>> - iterpool));
>> + iterpool);
>> +
>> + if (err&& keep_going)
>> + {
>> + svn_repos_notify_t *notify2 = svn_repos_notify_create(svn_repos_notify_failure, iterpool);
> Why call the variable 'notify2', not just 'notify'?
> Also, the above line doesn't wrap at column 78.
>
> I'd suggest using 'notify' as variable name and indenting like this:
>
> svn_repos_notify_t *notify;
>
> notify = svn_repos_notify_create(svn_repos_notify_failure,
> iterpool);
>
>> + notify2->err = err;
>> + notify_func(notify_baton, notify2, iterpool);
>> + svn_error_clear(err);
>> + continue;
>> + }
>> + else
>> + SVN_ERR(err);
>> +
>> SVN_ERR(svn_delta_get_cancellation_editor(cancel_func, cancel_baton,
>> dump_editor, dump_edit_baton,
>> &cancel_editor,
>> @@ -1433,9 +1447,21 @@
>> iterpool));
>>
>> SVN_ERR(svn_fs_revision_root(&to_root, fs, rev, iterpool));
>> - SVN_ERR(svn_repos_replay2(to_root, "", SVN_INVALID_REVNUM, FALSE,
>> + err = svn_repos_replay2(to_root, "", SVN_INVALID_REVNUM, FALSE,
>> cancel_editor, cancel_edit_baton,
> Again, please adjust indentation of function parameters.
>
>> - NULL, NULL, iterpool));
>> + NULL, NULL, iterpool);
>> +
>> + if (err&& keep_going)
>> + {
>> + svn_repos_notify_t *notify2 = svn_repos_notify_create(svn_repos_notify_failure, iterpool);
> Same as above suggestion for 'notify' + indentation.
>
>> + notify2->err = err;
>> + notify_func(notify_baton, notify2, iterpool);
>> + svn_error_clear(err);
>> + continue;
>> + }
>> + else
>> + SVN_ERR(err);
>> +
>> /* While our editor close_edit implementation is a no-op, we still
>> do this for completeness. */
>> SVN_ERR(cancel_editor->close_edit(cancel_edit_baton, iterpool));
>> Index: subversion/libsvn_repos/deprecated.c
>> ===================================================================
>> --- subversion/libsvn_repos/deprecated.c (revision 1402414)
>> +++ subversion/libsvn_repos/deprecated.c (working copy)
>> @@ -728,6 +728,27 @@
>> }
>>
>> svn_error_t *
>> +svn_repos_verify_fs2(svn_repos_t *repos,
>> + svn_revnum_t start_rev,
>> + svn_revnum_t end_rev,
>> + svn_repos_notify_func_t notify_func,
>> + void *notify_baton,
>> + svn_cancel_func_t cancel_func,
>> + void *cancel_baton,
>> + apr_pool_t *pool)
>> +{
>> + return svn_error_trace(svn_repos_verify_fs3(repos,
>> + start_rev,
>> + end_rev,
>> + FALSE,
>> + notify_func,
>> + notify_baton,
>> + cancel_func,
>> + cancel_baton,
>> + pool));
>> +}
>> +
>> +svn_error_t *
>> svn_repos_verify_fs(svn_repos_t *repos,
>> svn_stream_t *feedback_stream,
>> svn_revnum_t start_rev,
>> Index: subversion/libsvn_fs_fs/fs_fs.c
>> ===================================================================
>> --- subversion/libsvn_fs_fs/fs_fs.c (revision 1402414)
>> +++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
>> @@ -10117,6 +10117,7 @@
>> void *cancel_baton,
>> svn_revnum_t start,
>> svn_revnum_t end,
>> + svn_boolean_t keep_going,
>> apr_pool_t *pool)
>> {
>> fs_fs_data_t *ffd = fs->fsap_data;
>> @@ -10151,6 +10152,7 @@
>> commit-time checks in validate_root_noderev(). */
>> {
>> svn_revnum_t i;
>> + svn_error_t *err;
> We usually add a blank line between variable declarations and code.
> So you should add a blank here.
>
>> for (i = start; i<= end; i++)
>> {
>> svn_fs_root_t *root;
>> @@ -10162,7 +10164,15 @@
>> When this code is called in the library, we want to ensure we
>> use the on-disk data --- rather than some data that was read
>> in the possibly-distance past and cached since. */
>> - SVN_ERR(svn_fs_fs__revision_root(&root, fs, i, iterpool));
>> + err = svn_fs_fs__revision_root(&root, fs, i, iterpool);
>> + if (err&& keep_going)
>> + {
>> + svn_error_clear(err);
> No notification about this error?
>
> Instead of pushing the repos-layer notifier down into the FS-layer,
> I'd suggest catching this error thrown from svn_fs_fs__verify() in the
> repos-layer caller and notifying there if --keep-going was used.
>
> And then this function won't even need a keep_going parameter and all
> the FS-layer bits can be left unchanged, making your patch a bit smaller.
>
>> + return SVN_NO_ERROR;
>> + }
>> + else
>> + SVN_ERR(err);
>> +
>> SVN_ERR(svn_fs_fs__verify_root(root, iterpool));
>> }
>> }
>> Index: subversion/libsvn_fs_fs/fs_fs.h
>> ===================================================================
>> --- subversion/libsvn_fs_fs/fs_fs.h (revision 1402414)
>> +++ subversion/libsvn_fs_fs/fs_fs.h (working copy)
>> @@ -38,12 +38,14 @@
>> svn_error_t *svn_fs_fs__upgrade(svn_fs_t *fs,
>> apr_pool_t *pool);
>>
>> -/* Verify the fsfs filesystem FS. Use POOL for temporary allocations. */
>> +/* Verify the fsfs filesystem FS. Use POOL for temporary allocations.
>> + * If KEEP_GOING is TRUE, do not stop upon failure. */
>> svn_error_t *svn_fs_fs__verify(svn_fs_t *fs,
>> svn_cancel_func_t cancel_func,
>> void *cancel_baton,
>> svn_revnum_t start,
>> svn_revnum_t end,
>> + svn_boolean_t keep_going,
>> apr_pool_t *pool);
>>
>> /* Copy the fsfs filesystem SRC_FS at SRC_PATH into a new copy DST_FS at
>> Index: subversion/libsvn_fs_fs/fs.c
>> ===================================================================
>> --- subversion/libsvn_fs_fs/fs.c (revision 1402414)
>> +++ subversion/libsvn_fs_fs/fs.c (working copy)
>> @@ -287,6 +287,7 @@
>> void *cancel_baton,
>> svn_revnum_t start,
>> svn_revnum_t end,
>> + svn_boolean_t keep_going,
>> apr_pool_t *pool,
>> apr_pool_t *common_pool)
>> {
>> @@ -295,7 +296,8 @@
>> SVN_ERR(svn_fs_fs__open(fs, path, pool));
>> SVN_ERR(svn_fs_fs__initialize_caches(fs, pool));
>> SVN_ERR(fs_serialized_init(fs, common_pool, pool));
>> - return svn_fs_fs__verify(fs, cancel_func, cancel_baton, start, end, pool);
>> + return svn_fs_fs__verify(fs, cancel_func, cancel_baton, start, end,
>> + keep_going, pool);
>> }
>>
>> static svn_error_t *
>> Index: subversion/libsvn_fs/fs-loader.c
>> ===================================================================
>> --- subversion/libsvn_fs/fs-loader.c (revision 1402414)
>> +++ subversion/libsvn_fs/fs-loader.c (working copy)
>> @@ -487,6 +487,7 @@
>> void *cancel_baton,
>> svn_revnum_t start,
>> svn_revnum_t end,
>> + svn_boolean_t keep_going,
>> apr_pool_t *pool)
>> {
>> fs_library_vtable_t *vtable;
>> @@ -497,7 +498,7 @@
>>
>> SVN_MUTEX__WITH_LOCK(common_pool_lock,
>> vtable->verify_fs(fs, path, cancel_func, cancel_baton,
>> - start, end, pool, common_pool));
>> + start, end, keep_going, pool, common_pool));
>> return SVN_NO_ERROR;
>> }
>>
>> Index: subversion/libsvn_fs/fs-loader.h
>> ===================================================================
>> --- subversion/libsvn_fs/fs-loader.h (revision 1402414)
>> +++ subversion/libsvn_fs/fs-loader.h (working copy)
>> @@ -92,6 +92,7 @@
>> svn_cancel_func_t cancel_func, void *cancel_baton,
>> svn_revnum_t start,
>> svn_revnum_t end,
>> + svn_boolean_t keep_going,
>> apr_pool_t *pool,
>> apr_pool_t *common_pool);
>> svn_error_t *(*delete_fs)(const char *path, apr_pool_t *pool);
>> Index: subversion/svnadmin/main.c
>> ===================================================================
>> --- subversion/svnadmin/main.c (revision 1402414)
>> +++ subversion/svnadmin/main.c (working copy)
>> @@ -176,6 +176,7 @@
>> {
>> svnadmin__version = SVN_OPT_FIRST_LONGOPT_ID,
>> svnadmin__incremental,
>> + svnadmin__keep_going,
>> svnadmin__deltas,
>> svnadmin__ignore_uuid,
>> svnadmin__force_uuid,
>> @@ -288,6 +289,9 @@
>> N_("use format compatible with Subversion versions\n"
>> " earlier than 1.8")},
>>
>> + {"keep-going", svnadmin__keep_going, 0,
>> + N_("continue verifying even if there is a corruption")},
>> +
>> {"memory-cache-size", 'M', 1,
>> N_("size of the extra in-memory cache in MB used to\n"
>> " minimize redundant operations. Default: 16.\n"
>> @@ -475,7 +479,7 @@
>> {"verify", subcommand_verify, {0}, N_
>> ("usage: svnadmin verify REPOS_PATH\n\n"
>> "Verifies the data stored in the repository.\n"),
>> - {'r', 'q', 'M'} },
>> + {'r', 'q', svnadmin__keep_going, 'M'} },
>>
>> { NULL, NULL, {0}, NULL, {0} }
>> };
>> @@ -505,6 +509,7 @@
>> svn_boolean_t clean_logs; /* --clean-logs */
>> svn_boolean_t bypass_hooks; /* --bypass-hooks */
>> svn_boolean_t wait; /* --wait */
>> + svn_boolean_t keep_going; /* --keep-going */
>> svn_boolean_t bypass_prop_validation; /* --bypass-prop-validation */
>> enum svn_repos_load_uuid uuid_action; /* --ignore-uuid,
>> --force-uuid */
>> @@ -738,6 +743,10 @@
>> notify->warning_str));
>> return;
>>
>> + case svn_repos_notify_failure:
>> + svn_handle_error2(notify->err, stderr, FALSE /* non-fatal */, "svnadmin: ");
> Another overlong line. I'd suggest:
>
> svn_handle_error2(notify->err, stderr, FALSE /* non-fatal */,
> "svnadmin: ");
>
>> + return;
>> +
>> case svn_repos_notify_dump_rev_end:
>> svn_error_clear(svn_stream_printf(feedback_stream, scratch_pool,
>> _("* Dumped revision %ld.\n"),
>> @@ -1527,7 +1536,8 @@
>> if (! opt_state->quiet)
>> progress_stream = recode_stream_create(stderr, pool);
>>
>> - return svn_repos_verify_fs2(repos, lower, upper,
>> + return svn_repos_verify_fs3(repos, lower, upper,
>> + opt_state->keep_going,
>> !opt_state->quiet
>> ? repos_notify_handler : NULL,
>> progress_stream, check_cancel, NULL, pool);
>> @@ -1992,6 +2002,9 @@
>> case svnadmin__pre_1_8_compatible:
>> opt_state.pre_1_8_compatible = TRUE;
>> break;
>> + case svnadmin__keep_going:
>> + opt_state.keep_going = TRUE;
>> + break;
>> case svnadmin__fs_type:
>> SVN_INT_ERR(svn_utf_cstring_to_utf8(&opt_state.fs_type, opt_arg, pool));
>> break;
>> Index: subversion/libsvn_fs_base/fs.c
>> ===================================================================
>> --- subversion/libsvn_fs_base/fs.c (revision 1402414)
>> +++ subversion/libsvn_fs_base/fs.c (working copy)
>> @@ -896,6 +896,7 @@
>> void *cancel_baton,
>> svn_revnum_t start,
>> svn_revnum_t end,
>> + svn_boolean_t keep_going,
>> apr_pool_t *pool,
>> apr_pool_t *common_pool)
>> {
>> Index: subversion/include/svn_fs.h
>> ===================================================================
>> --- subversion/include/svn_fs.h (revision 1402414)
>> +++ subversion/include/svn_fs.h (working copy)
>> @@ -280,6 +280,7 @@
>> void *cancel_baton,
>> svn_revnum_t start,
>> svn_revnum_t end,
>> + svn_boolean_t keep_going,
>> apr_pool_t *scratch_pool);
>>
>> /**
>> Index: subversion/include/svn_repos.h
>> ===================================================================
>> --- subversion/include/svn_repos.h (revision 1402414)
>> +++ subversion/include/svn_repos.h (working copy)
>> @@ -193,6 +193,9 @@
>> /** A warning message is waiting. */
>> svn_repos_notify_warning = 0,
>>
>> + /** A revision is found with corruption/errors. */
>> + svn_repos_notify_failure,
>> +
> This enum has already been released in 1.7!
> We must add new values at the end to preserve binary compatibility.
> So please put svn_repos_notify_failure after svn_repos_notify_load_skipped_rev.
>
> And please add "@since New in 1.8" to the docstring, like this:
>
> /** A revision is found with corruption/errors. @since New in 1.8. */
>
>> /** A revision has finished being dumped. */
>> svn_repos_notify_dump_rev_end,
>>
>> @@ -318,9 +321,13 @@
>> /** For #svn_repos_notify_load_node_start, the path of the node. */
>> const char *path;
>>
>> + /** The error chain. */
> Please expand this comment to explain the intended use of this field.
>
> I'd suggest:
>
> /** For #svn_repos_notify_failure, this error chain indicates what
> * went wrong during verification. */
>
>> + svn_error_t *err;
>> +
>> /* NOTE: Add new fields at the end to preserve binary compatibility.
>> Also, if you add fields here, you have to update
>> svn_repos_notify_create(). */
>> +
> Why add a blank line here?
>
>> } svn_repos_notify_t;
>>
>> /** Callback for providing notification from the repository.
>> @@ -2517,8 +2524,29 @@
>> * cancel_baton as argument to see if the caller wishes to cancel the
>> * verification.
>> *
>> + * If @a keep_going is @c TRUE, the verify process prints the error message
>> + * to the stderr and continues.
>> + *
>> + * @since New in 1.8.
>> + */
>> +svn_error_t *
>> +svn_repos_verify_fs3(svn_repos_t *repos,
>> + svn_revnum_t start_rev,
>> + svn_revnum_t end_rev,
>> + svn_boolean_t keep_going,
>> + svn_repos_notify_func_t notify_func,
>> + void *notify_baton,
>> + svn_cancel_func_t cancel,
>> + void *cancel_baton,
>> + apr_pool_t *scratch_pool);
>> +
>> +/**
>> + * Similar to svn_repos_verify_fs3(), but without force.
> It's now called 'keep_going', not 'force'.
>
> I'd suggest to say:
>
> * Like svn_repos_verify_fs3(), but with @a keep_going set to @c FALSE.
>
>> + *
>> * @since New in 1.7.
>> + * @deprecated Provided for backward compatibility with the 1.7 API.
>> */
>> +SVN_DEPRECATED
>> svn_error_t *
>> svn_repos_verify_fs2(svn_repos_t *repos,
>> svn_revnum_t start_rev,
>> Implement svnadmin verify --keep-going, which would continue the verification
>> even if there is some corruption, after printing the errors to stderr.
>>
>> * subversion/svnadmin/main.c
>> (svnadmin__cmdline_options_t):
> Missing change description? Or has svnadmin__cmdline_options_t not changed?
>
>> (svnadmin_opt_state) : add keep-going option.
> The indentation style of the log message differs from the style
> we usually use. I'd suggest formatting log entries like this:
>
> * subversion/svnadmin/main.c
> (svnadmin_opt_state): add keep-going option.
> (subcommand_verify): uses the new svn_repos_verify_fs3 function.
> (repos_notify_handler): svn_repos_notify_failure handles failure and
> prints warning to stderr.
>
> I'd also suggest using proper capitalisation.
>
> * subversion/svnadmin/main.c
> (svnadmin_opt_state): Add keep-going option.
>
> What follows are stylistic suggestions. Log messages all differ based on
> personal style and preferences. There are no strict guidelines and what
> follows is just my personal opinion.
>
>> (subcommand_verify) : uses the new svn_repos_verify_fs3 function.
> The above doesn't describe the change itself but the result of the change.
>
> Also, third-person simple present verbs like "X uses Y" makes it sounds as
> if you're stating a fact, something that is always true and independent of
> your change, rather than describing a change you are making.
>
> Compare to this:
>
> (subcommand_verify): Switch to the new svn_repos_verify_fs3 function
> instead of svn_repos_verify_fs2, and pass the keep-going option.
>
> I've added a minor detail by mentioning the keep-going option.
> When describing changes I often try to describe each chunk of the diff
> in a way that allows the reader of the log message to imagine what the
> chunk might look like. This might seem overly detailed and it's not a
> strict requirement of the project. But I found that this helps me with
> writing log messages that I can make sense of myself when I read them
> again half a year later :)
>
>> (repos_notify_handler) : svn_repos_notify_failure handles failure and
>> prints warning to stderr.
> When expressing code changes in English, I prefer to phrase data structures
> (in this case, svn_repos_notify_failure) as grammatical objects and executing
> code acting on the data (in this case, repos_notify_handler) as the
> grammatical subject acting on the object.
>
> Your wording suggests that svn_repos_notify_failure (the data), and not
> repos_notify_handler (the executing code), prints the warning to stderr.
>
> Compare to phrasing it like this:
>
> (repos_notify_handler): Handle svn_repos_notify_failure notification
> by printing warnings to stderr.
>
> That's all for now. I haven't tried to compile and run the patch yet.
>
> You might also want to add a regression test for your change, by the way,
> to make sure the --keep-going option doesn't break when we make further
> changes in the future. But that can be done as a separate patch.
>
> Thanks!
>
>> * subversion/include/svn_repos.h
>> (svn_repos_notify_action_t): add svn_repos_notify_failure to notify failure.
>> (svn_repos_verify_fs3) : newly added to handle "keep-going" option.
>>
>> * subversion/include/svn_fs.h
>> (svn_fs_verify) : handles keep-going. If keep-going is TRUE, svn_fs_verify
>> would return no error.
>>
>> * subversion/libsvn_fs_fs/fs_fs.h
>> (svn_fs_fs__verify) : added the keep_going parameter.
>>
>> * subversion/libsvn_fs_fs/fs_fs.c
>> (svn_fs_fs__verify) : handles keep_going. If keep_going is TRUE, SVN_NO_ERROR
>> is returned upon failure too.
>>
>> * subversion/libsvn_fs/fs-loader.h
>> (fs_library_vtable_t) : added keep_going parameter.
>>
>> * subversion/libsvn_fs/fs-loader.c
>> (svn_fs_verify) : handles keep_going, which is also passed on to vtable->verify_fs.
>>
>> * subversion/libsvn_fs_base/fs.c
>> (base_verify) : handles keep_going parameter.
>>
>> * subversion/libsvn_fs_fs/fs.c
>> (fs_verify) : send keep_going to svn_fs_fs__verify function.
>>
>> * subversion/libsvn_repos/dump.c
>> (svn_repos_verify_fs3): now handles "keep-going".
>> If "keep-going" is TRUE, then the error message is
>> written to the stderr and verify process continues.
>>
>> * subversion/libsvn_repos/deprecated.c
>> (svn_repos_verify_fs2) : deprecated.
>> Now calls svn_repos_verify_fs3 with keep-going as FALSE.
>>
>>
>> Patch by: Prabhu Gnana Sundar<prabhugs{_AT_}collab.net>
Received on 2012-10-29 18:23:36 CET