You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Näslund <da...@longitudo.com> on 2010/09/27 22:09:26 UTC
[WIP] Gracefully handle svn_wc_prop_set4() errors in svn patch
Hi!
Questions
-------------
* Is it sane to add a svn_wc_notify_failed_prop_patch action for this
special case? We're starting to have a lot of actions.
* What is wrong with the way I handle the error? I hit err_abort() when
the program terminates. (I'm expecting the answer to hurt a bit - this
is surely something that I should have understood by now). I thought
that since the error is allocated on the heap I could just store the
pointer to it and free it later, e.g. call svn_error_clear().
[[[
Print a warning instead of error out if a property could not be set on a
path with 'svn patch'.
* subversion/svn/notify.c
(notify): Add svn_wc_notify_failed_prop_patch case.
* subversion/include/svn_wc.h
(svn_wc_notify_action_t): Add svn_wc_notify_failed_prop_patch field.
(svn_wc_notify_t): Update doc string for 'err' field to mention that
it is set for svn_wc_notify_failed_prop_patch.
* subversion/libsvn_client/patch.c
(prop_patch_target_t): Add 'was_rejected' and 'err' field to record
failed property patches.
(send_patch_notification): Send svn_wc_notify_failed_prop_patch
notifications.
(install_patched_prop_targets): Record failed propsets.
]]]
Daniel
Re: [WIP] Gracefully handle svn_wc_prop_set4() errors in svn patch
Posted by Daniel Näslund <da...@longitudo.com>.
On Tue, Sep 28, 2010 at 07:18:39PM +0100, Philip Martin wrote:
> Daniel Näslund <da...@longitudo.com> writes:
>
> > On Tue, Sep 28, 2010 at 03:45:33PM +0200, Daniel Shahaf wrote:
> >> Daniel Näslund wrote on Mon, Sep 27, 2010 at 22:09:26 +0200:
> >>
> >> err_abort() is called when an error object hadn't been svn_error_clear()'d.
> >> (The error creation installed err_abort() as a pool cleanup callback,
> >> and clearing the error unregisters the callback.)
> >
> > Yes, that was how I understood it.
>
> If you run the program gdb, it will catch the abort. If you step up
> the stack to err_abort and print err[0] then you will see the file and
> line where the error was created. (You may well have worked this out
> already).
Turned out that it was caused by prop_target->was_deleted (the flag that
was set when an error was detected) not beeing initialized to FALSE.
Thanks for the suggestion on checking err. Didn't think of that (but I
really should have!).
Thanks,
Daniel
Re: [WIP] Gracefully handle svn_wc_prop_set4() errors in svn patch
Posted by Daniel Näslund <da...@longitudo.com>.
On Tue, Sep 28, 2010 at 07:18:39PM +0100, Philip Martin wrote:
> Daniel Näslund <da...@longitudo.com> writes:
>
> > On Tue, Sep 28, 2010 at 03:45:33PM +0200, Daniel Shahaf wrote:
> >> Daniel Näslund wrote on Mon, Sep 27, 2010 at 22:09:26 +0200:
> >>
> >> err_abort() is called when an error object hadn't been svn_error_clear()'d.
> >> (The error creation installed err_abort() as a pool cleanup callback,
> >> and clearing the error unregisters the callback.)
> >
> > Yes, that was how I understood it.
>
> If you run the program gdb, it will catch the abort. If you step up
> the stack to err_abort and print err[0] then you will see the file and
> line where the error was created. (You may well have worked this out
> already).
Turned out that it was caused by prop_target->was_deleted (the flag that
was set when an error was detected) not beeing initialized to FALSE.
Thanks for the suggestion on checking err. Didn't think of that (but I
really should have!).
Thanks,
Daniel
Re: [WIP] Gracefully handle svn_wc_prop_set4() errors in svn patch
Posted by Philip Martin <ph...@wandisco.com>.
Daniel Näslund <da...@longitudo.com> writes:
> On Tue, Sep 28, 2010 at 03:45:33PM +0200, Daniel Shahaf wrote:
>> Daniel Näslund wrote on Mon, Sep 27, 2010 at 22:09:26 +0200:
>>
>> err_abort() is called when an error object hadn't been svn_error_clear()'d.
>> (The error creation installed err_abort() as a pool cleanup callback,
>> and clearing the error unregisters the callback.)
>
> Yes, that was how I understood it.
If you run the program gdb, it will catch the abort. If you step up
the stack to err_abort and print err[0] then you will see the file and
line where the error was created. (You may well have worked this out
already).
>> > @@ -2260,14 +2283,23 @@ install_patched_prop_targets(patch_target_t *targe
>> > + err = svn_wc_prop_set4(ctx->wc_ctx, target->local_abspath,
>> > + prop_target->name,
>> > + svn_string_create_from_buf(prop_content,
>> > + iterpool),
>> > + TRUE /* skip_checks */,
>> > + NULL, NULL,
>> > + iterpool);
>> > + if (err)
>> > + {
>> > + prop_target->was_rejected = TRUE;
>> > + prop_target->err = err;
>>
>> Does prop_target->err always get cleared?
>>
>> (The answer is probably "No".)
>
> As I said above, my intention was to clear it in
> send_patch_notification().
You will still have a problem if there is more that one error and the
assignment above overwrites a previous err, the overwritten err will
have leaked.
--
Philip
Re: [WIP] Gracefully handle svn_wc_prop_set4() errors in svn patch
Posted by Daniel Näslund <da...@longitudo.com>.
On Tue, Sep 28, 2010 at 03:45:33PM +0200, Daniel Shahaf wrote:
> Daniel Näslund wrote on Mon, Sep 27, 2010 at 22:09:26 +0200:
> > * What is wrong with the way I handle the error? I hit err_abort() when
> > the program terminates. (I'm expecting the answer to hurt a bit - this
> > is surely something that I should have understood by now). I thought
> > that since the error is allocated on the heap I could just store the
> > pointer to it and free it later, e.g. call svn_error_clear().
>
> err_abort() is called when an error object hadn't been svn_error_clear()'d.
> (The error creation installed err_abort() as a pool cleanup callback,
> and clearing the error unregisters the callback.)
Yes, that was how I understood it.
> So, yeah, you can do whatever you want with the error (they get
> allocated in a global pool) as long as you svn_error_clear() them
> eventually.
Ok.
> > Index: subversion/svn/notify.c
> > ===================================================================
> > --- subversion/svn/notify.c (revision 1001829)
> > +++ subversion/svn/notify.c (arbetskopia)
> > @@ -464,6 +464,16 @@ notify(void *baton, const svn_wc_notify_t *n, apr_
> > goto print_error;
> > break;
> >
> > + case svn_wc_notify_failed_prop_patch:
> > + nb->received_some_change = TRUE;
> > + err = svn_cmdline_printf(pool,
> > + _("property '%s' rejected from '%s'.\n"),
> > + n->prop_name, path_local);
> > + svn_handle_warning2(stderr, n->err, "svn: ");
> > + if (err)
> > + goto print_error;
> > + break;
> > +
>
> That's fine, print_error: clears the error.
> > Index: subversion/libsvn_client/patch.c
> > ===================================================================
> > --- subversion/libsvn_client/patch.c (revision 1001829)
> > +++ subversion/libsvn_client/patch.c (arbetskopia)
> > @@ -130,6 +130,12 @@ typedef struct prop_patch_target_t {
> >
> > /* ### Here we'll add flags telling if the prop was added, deleted,
> > * ### had_rejects, had_local_mods prior to patching and so on. */
> > +
> > + /* TRUE if the property could not be set on the path. */
> > + svn_boolean_t was_rejected;
> > +
> > + /* Set if was_rejected is TRUE. */
> > + svn_error_t *err;
> > } prop_patch_target_t;
> >
> > typedef struct patch_target_t {
> > @@ -1573,6 +1579,22 @@ send_patch_notification(const patch_target_t *targ
> >
> > prop_target = svn__apr_hash_index_val(hash_index);
> >
> > + if (prop_target->was_rejected)
> > + {
> > + svn_wc_notify_t *notify;
> > + svn_wc_notify_action_t action = svn_wc_notify_failed_prop_patch;
> > +
> > + notify = svn_wc_create_notify(target->local_abspath
> > + ? target->local_abspath
> > + : target->local_relpath,
> > + action, pool);
> > + notify->prop_name = prop_target->name;
> > + notify->err = prop_target->err;
> > +
> > + (*ctx->notify_func2)(ctx->notify_baton2, notify, pool);
> > + svn_error_clear(prop_target->err);
Here I'm clearing prop_target->err. Since prop_target->was_rejected is
only set if and error exists (e.g. ! prop_target->err) I was expecting
that err would always be cleared.
[...]
> > @@ -2260,14 +2283,23 @@ install_patched_prop_targets(patch_target_t *targe
> > + err = svn_wc_prop_set4(ctx->wc_ctx, target->local_abspath,
> > + prop_target->name,
> > + svn_string_create_from_buf(prop_content,
> > + iterpool),
> > + TRUE /* skip_checks */,
> > + NULL, NULL,
> > + iterpool);
> > + if (err)
> > + {
> > + prop_target->was_rejected = TRUE;
> > + prop_target->err = err;
>
> Does prop_target->err always get cleared?
>
> (The answer is probably "No".)
As I said above, my intention was to clear it in
send_patch_notification().
I'll check again and see if I can spot why the err isn't always cleared.
Thanks for your feedback,
Daniel (dannas)
Re: [WIP] Gracefully handle svn_wc_prop_set4() errors in svn patch
Posted by Daniel Näslund <da...@longitudo.com>.
On Tue, Sep 28, 2010 at 03:45:33PM +0200, Daniel Shahaf wrote:
> Daniel Näslund wrote on Mon, Sep 27, 2010 at 22:09:26 +0200:
> > * What is wrong with the way I handle the error? I hit err_abort() when
> > the program terminates. (I'm expecting the answer to hurt a bit - this
> > is surely something that I should have understood by now). I thought
> > that since the error is allocated on the heap I could just store the
> > pointer to it and free it later, e.g. call svn_error_clear().
>
> err_abort() is called when an error object hadn't been svn_error_clear()'d.
> (The error creation installed err_abort() as a pool cleanup callback,
> and clearing the error unregisters the callback.)
Yes, that was how I understood it.
> So, yeah, you can do whatever you want with the error (they get
> allocated in a global pool) as long as you svn_error_clear() them
> eventually.
Ok.
> > Index: subversion/svn/notify.c
> > ===================================================================
> > --- subversion/svn/notify.c (revision 1001829)
> > +++ subversion/svn/notify.c (arbetskopia)
> > @@ -464,6 +464,16 @@ notify(void *baton, const svn_wc_notify_t *n, apr_
> > goto print_error;
> > break;
> >
> > + case svn_wc_notify_failed_prop_patch:
> > + nb->received_some_change = TRUE;
> > + err = svn_cmdline_printf(pool,
> > + _("property '%s' rejected from '%s'.\n"),
> > + n->prop_name, path_local);
> > + svn_handle_warning2(stderr, n->err, "svn: ");
> > + if (err)
> > + goto print_error;
> > + break;
> > +
>
> That's fine, print_error: clears the error.
> > Index: subversion/libsvn_client/patch.c
> > ===================================================================
> > --- subversion/libsvn_client/patch.c (revision 1001829)
> > +++ subversion/libsvn_client/patch.c (arbetskopia)
> > @@ -130,6 +130,12 @@ typedef struct prop_patch_target_t {
> >
> > /* ### Here we'll add flags telling if the prop was added, deleted,
> > * ### had_rejects, had_local_mods prior to patching and so on. */
> > +
> > + /* TRUE if the property could not be set on the path. */
> > + svn_boolean_t was_rejected;
> > +
> > + /* Set if was_rejected is TRUE. */
> > + svn_error_t *err;
> > } prop_patch_target_t;
> >
> > typedef struct patch_target_t {
> > @@ -1573,6 +1579,22 @@ send_patch_notification(const patch_target_t *targ
> >
> > prop_target = svn__apr_hash_index_val(hash_index);
> >
> > + if (prop_target->was_rejected)
> > + {
> > + svn_wc_notify_t *notify;
> > + svn_wc_notify_action_t action = svn_wc_notify_failed_prop_patch;
> > +
> > + notify = svn_wc_create_notify(target->local_abspath
> > + ? target->local_abspath
> > + : target->local_relpath,
> > + action, pool);
> > + notify->prop_name = prop_target->name;
> > + notify->err = prop_target->err;
> > +
> > + (*ctx->notify_func2)(ctx->notify_baton2, notify, pool);
> > + svn_error_clear(prop_target->err);
Here I'm clearing prop_target->err. Since prop_target->was_rejected is
only set if and error exists (e.g. ! prop_target->err) I was expecting
that err would always be cleared.
[...]
> > @@ -2260,14 +2283,23 @@ install_patched_prop_targets(patch_target_t *targe
> > + err = svn_wc_prop_set4(ctx->wc_ctx, target->local_abspath,
> > + prop_target->name,
> > + svn_string_create_from_buf(prop_content,
> > + iterpool),
> > + TRUE /* skip_checks */,
> > + NULL, NULL,
> > + iterpool);
> > + if (err)
> > + {
> > + prop_target->was_rejected = TRUE;
> > + prop_target->err = err;
>
> Does prop_target->err always get cleared?
>
> (The answer is probably "No".)
As I said above, my intention was to clear it in
send_patch_notification().
I'll check again and see if I can spot why the err isn't always cleared.
Thanks for your feedback,
Daniel (dannas)
Re: [WIP] Gracefully handle svn_wc_prop_set4() errors in svn patch
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Daniel Näslund wrote on Mon, Sep 27, 2010 at 22:09:26 +0200:
> * What is wrong with the way I handle the error? I hit err_abort() when
> the program terminates. (I'm expecting the answer to hurt a bit - this
> is surely something that I should have understood by now). I thought
> that since the error is allocated on the heap I could just store the
> pointer to it and free it later, e.g. call svn_error_clear().
err_abort() is called when an error object hadn't been svn_error_clear()'d.
(The error creation installed err_abort() as a pool cleanup callback,
and clearing the error unregisters the callback.)
So, yeah, you can do whatever you want with the error (they get
allocated in a global pool) as long as you svn_error_clear() them
eventually.
>
> Index: subversion/svn/notify.c
> ===================================================================
> --- subversion/svn/notify.c (revision 1001829)
> +++ subversion/svn/notify.c (arbetskopia)
> @@ -464,6 +464,16 @@ notify(void *baton, const svn_wc_notify_t *n, apr_
> goto print_error;
> break;
>
> + case svn_wc_notify_failed_prop_patch:
> + nb->received_some_change = TRUE;
> + err = svn_cmdline_printf(pool,
> + _("property '%s' rejected from '%s'.\n"),
> + n->prop_name, path_local);
> + svn_handle_warning2(stderr, n->err, "svn: ");
> + if (err)
> + goto print_error;
> + break;
> +
That's fine, print_error: clears the error.
> Index: subversion/libsvn_client/patch.c
> ===================================================================
> --- subversion/libsvn_client/patch.c (revision 1001829)
> +++ subversion/libsvn_client/patch.c (arbetskopia)
> @@ -130,6 +130,12 @@ typedef struct prop_patch_target_t {
>
> /* ### Here we'll add flags telling if the prop was added, deleted,
> * ### had_rejects, had_local_mods prior to patching and so on. */
> +
> + /* TRUE if the property could not be set on the path. */
> + svn_boolean_t was_rejected;
> +
> + /* Set if was_rejected is TRUE. */
> + svn_error_t *err;
> } prop_patch_target_t;
>
> typedef struct patch_target_t {
> @@ -1573,6 +1579,22 @@ send_patch_notification(const patch_target_t *targ
>
> prop_target = svn__apr_hash_index_val(hash_index);
>
> + if (prop_target->was_rejected)
> + {
> + svn_wc_notify_t *notify;
> + svn_wc_notify_action_t action = svn_wc_notify_failed_prop_patch;
> +
> + notify = svn_wc_create_notify(target->local_abspath
> + ? target->local_abspath
> + : target->local_relpath,
> + action, pool);
> + notify->prop_name = prop_target->name;
> + notify->err = prop_target->err;
> +
> + (*ctx->notify_func2)(ctx->notify_baton2, notify, pool);
> + svn_error_clear(prop_target->err);
> + }
> +
> for (i = 0; i < prop_target->content_info->hunks->nelts; i++)
> {
> const hunk_info_t *hi;
> @@ -2189,6 +2211,7 @@ install_patched_prop_targets(patch_target_t *targe
> svn_stringbuf_t *prop_content;
> const char *eol_str;
> svn_boolean_t eof;
> + svn_error_t *err;
>
> svn_pool_clear(iterpool);
>
> @@ -2260,14 +2283,23 @@ install_patched_prop_targets(patch_target_t *targe
> * ### stsp: I'd say reject the property hunk.
> * ### We should verify all modified prop hunk texts using
> * ### svn_wc_canonicalize_svn_prop() before starting the
> - * ### patching process, to reject them as early as possible. */
> - SVN_ERR(svn_wc_prop_set4(ctx->wc_ctx, target->local_abspath,
> - prop_target->name,
> - svn_string_create_from_buf(prop_content,
> - iterpool),
> - TRUE /* skip_checks */,
> - NULL, NULL,
> - iterpool));
> + * ### patching process, to reject them as early as possible.
> + *
> + * ### dannas: Unfortunately we need the prop_content to run
> + * ### svn_wc_canonicalize_svn_prop() and we don't have that
> + * ### until we've applied our text changes. */
> + err = svn_wc_prop_set4(ctx->wc_ctx, target->local_abspath,
> + prop_target->name,
> + svn_string_create_from_buf(prop_content,
> + iterpool),
> + TRUE /* skip_checks */,
> + NULL, NULL,
> + iterpool);
> + if (err)
> + {
> + prop_target->was_rejected = TRUE;
> + prop_target->err = err;
Does prop_target->err always get cleared?
(The answer is probably "No".)
> + }
> }
>
> svn_pool_destroy(iterpool);
Re: [WIP] Gracefully handle svn_wc_prop_set4() errors in svn patch
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Daniel Näslund wrote on Mon, Sep 27, 2010 at 22:09:26 +0200:
> * What is wrong with the way I handle the error? I hit err_abort() when
> the program terminates. (I'm expecting the answer to hurt a bit - this
> is surely something that I should have understood by now). I thought
> that since the error is allocated on the heap I could just store the
> pointer to it and free it later, e.g. call svn_error_clear().
err_abort() is called when an error object hadn't been svn_error_clear()'d.
(The error creation installed err_abort() as a pool cleanup callback,
and clearing the error unregisters the callback.)
So, yeah, you can do whatever you want with the error (they get
allocated in a global pool) as long as you svn_error_clear() them
eventually.
>
> Index: subversion/svn/notify.c
> ===================================================================
> --- subversion/svn/notify.c (revision 1001829)
> +++ subversion/svn/notify.c (arbetskopia)
> @@ -464,6 +464,16 @@ notify(void *baton, const svn_wc_notify_t *n, apr_
> goto print_error;
> break;
>
> + case svn_wc_notify_failed_prop_patch:
> + nb->received_some_change = TRUE;
> + err = svn_cmdline_printf(pool,
> + _("property '%s' rejected from '%s'.\n"),
> + n->prop_name, path_local);
> + svn_handle_warning2(stderr, n->err, "svn: ");
> + if (err)
> + goto print_error;
> + break;
> +
That's fine, print_error: clears the error.
> Index: subversion/libsvn_client/patch.c
> ===================================================================
> --- subversion/libsvn_client/patch.c (revision 1001829)
> +++ subversion/libsvn_client/patch.c (arbetskopia)
> @@ -130,6 +130,12 @@ typedef struct prop_patch_target_t {
>
> /* ### Here we'll add flags telling if the prop was added, deleted,
> * ### had_rejects, had_local_mods prior to patching and so on. */
> +
> + /* TRUE if the property could not be set on the path. */
> + svn_boolean_t was_rejected;
> +
> + /* Set if was_rejected is TRUE. */
> + svn_error_t *err;
> } prop_patch_target_t;
>
> typedef struct patch_target_t {
> @@ -1573,6 +1579,22 @@ send_patch_notification(const patch_target_t *targ
>
> prop_target = svn__apr_hash_index_val(hash_index);
>
> + if (prop_target->was_rejected)
> + {
> + svn_wc_notify_t *notify;
> + svn_wc_notify_action_t action = svn_wc_notify_failed_prop_patch;
> +
> + notify = svn_wc_create_notify(target->local_abspath
> + ? target->local_abspath
> + : target->local_relpath,
> + action, pool);
> + notify->prop_name = prop_target->name;
> + notify->err = prop_target->err;
> +
> + (*ctx->notify_func2)(ctx->notify_baton2, notify, pool);
> + svn_error_clear(prop_target->err);
> + }
> +
> for (i = 0; i < prop_target->content_info->hunks->nelts; i++)
> {
> const hunk_info_t *hi;
> @@ -2189,6 +2211,7 @@ install_patched_prop_targets(patch_target_t *targe
> svn_stringbuf_t *prop_content;
> const char *eol_str;
> svn_boolean_t eof;
> + svn_error_t *err;
>
> svn_pool_clear(iterpool);
>
> @@ -2260,14 +2283,23 @@ install_patched_prop_targets(patch_target_t *targe
> * ### stsp: I'd say reject the property hunk.
> * ### We should verify all modified prop hunk texts using
> * ### svn_wc_canonicalize_svn_prop() before starting the
> - * ### patching process, to reject them as early as possible. */
> - SVN_ERR(svn_wc_prop_set4(ctx->wc_ctx, target->local_abspath,
> - prop_target->name,
> - svn_string_create_from_buf(prop_content,
> - iterpool),
> - TRUE /* skip_checks */,
> - NULL, NULL,
> - iterpool));
> + * ### patching process, to reject them as early as possible.
> + *
> + * ### dannas: Unfortunately we need the prop_content to run
> + * ### svn_wc_canonicalize_svn_prop() and we don't have that
> + * ### until we've applied our text changes. */
> + err = svn_wc_prop_set4(ctx->wc_ctx, target->local_abspath,
> + prop_target->name,
> + svn_string_create_from_buf(prop_content,
> + iterpool),
> + TRUE /* skip_checks */,
> + NULL, NULL,
> + iterpool);
> + if (err)
> + {
> + prop_target->was_rejected = TRUE;
> + prop_target->err = err;
Does prop_target->err always get cleared?
(The answer is probably "No".)
> + }
> }
>
> svn_pool_destroy(iterpool);