Re: [WIP] Gracefully handle svn_wc_prop_set4() errors in svn patch
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.)
+}
}
Re: [WIP] Gracefully handle svn_wc_prop_set4() errors in svn patch
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
Daniel Näslund dan...@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
On Tue, Sep 28, 2010 at 07:18:39PM +0100, Philip Martin wrote: Daniel Näslund dan...@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
[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
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;
+
case svn_wc_notify_update_update:
case svn_wc_notify_merge_record_info:
{
Index: subversion/include/svn_wc.h
===
--- subversion/include/svn_wc.h (revision 1001829)
+++ subversion/include/svn_wc.h (arbetskopia)
@@ -1089,8 +1089,11 @@ typedef enum svn_wc_notify_action_t
/** The server has instructed the client to follow a URL
* redirection.
* @since New in 1.7. */
- svn_wc_notify_url_redirect
+ svn_wc_notify_url_redirect,
+ /** A hunk from a patch could not be applied. */
+ svn_wc_notify_failed_prop_patch
+
} svn_wc_notify_action_t;
@@ -1198,7 +1201,8 @@ typedef struct svn_wc_notify_t {
/** Points to an error describing the reason for the failure when @c
* action is one of the following: #svn_wc_notify_failed_lock,
- * #svn_wc_notify_failed_unlock, #svn_wc_notify_failed_external.
+ * #svn_wc_notify_failed_unlock, #svn_wc_notify_failed_external,
+ * #svn_wc_notify_failed_prop_patch.
* Is @c NULL otherwise. */
svn_error_t *err;
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
* ###
