Re: [PATCH] issue #4527: notify start of exporting external
Schmidt, Michael michael.schm...@mevis.fraunhofer.de writes:
This patch only fixes the missing notification of starting to export
externals. It does not address the second part of issue #4527, which
is the premature reset of the in_external flag in the case of nested
externals, which also affects checkouts. To me it seems that the best
option for fixing that issue would be to turn the bool flag into a
counter which keeps track of the stack deptch of externals, but I am
not totally sure about how this affects the ABI and whether there
might be better ways of fixing this. Any comments/ideas?
The in_external flag is local to svn/notify.c so there is no ABI
problem. Making it into a counter sounds like the right way to fix the
problem.
Index: subversion/libsvn_client/externals.c
===
--- subversion/libsvn_client/externals.c (revision 1638837)
+++ subversion/libsvn_client/externals.c (working copy)
@@ -1188,6 +1188,17 @@ svn_client__export_externals(apr_hash_t *externals
sub_iterpool),
sub_iterpool));
+ /* First notify that we're about to handle an external. */
+ if (ctx-notify_func2)
+ {
+(*ctx-notify_func2)(
+ ctx-notify_baton2,
+ svn_wc_create_notify(item_abspath,
+ svn_wc_notify_update_external,
+ sub_iterpool),
+ sub_iterpool);
+ }
+
SVN_ERR(wrap_external_error(
ctx, item_abspath,
svn_client_export5(NULL, new_url, item_abspath,
The indentation is wrong, it should be more like:
+ /* First notify that we're about to handle an external. */
+ if (ctx-notify_func2)
+{
+ (*ctx-notify_func2)(
+ctx-notify_baton2,
+svn_wc_create_notify(item_abspath,
+ svn_wc_notify_update_external,
+ sub_iterpool),
+sub_iterpool);
+}
I noticed the existing code was using a mix of styles:
(*ctx-notify_func2)(...)
(ctx-notify_func2)(...)
ctx-notify_func2(...)
I've changed them all to the last form.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: [PATCH] issue #4527: notify start of exporting external
Philip Martin wrote:
Schmidt, Michael writes:
This patch only fixes the missing notification of starting to export
externals. It does not address the second part of issue #4527, which
is the premature reset of the in_external flag in the case of nested
externals, which also affects checkouts. To me it seems that the best
option for fixing that issue would be to turn the bool flag into a
counter which keeps track of the stack deptch of externals, but I am
not totally sure about how this affects the ABI and whether there
might be better ways of fixing this. Any comments/ideas?
The in_external flag is local to svn/notify.c so there is no ABI
problem. Making it into a counter sounds like the right way to fix the
problem.
+ /* First notify that we're about to handle an external. */
+ if (ctx-notify_func2)
+ {
+ (*ctx-notify_func2)(
+ ctx-notify_baton2,
+ svn_wc_create_notify(item_abspath,
+ svn_wc_notify_update_external,
+ sub_iterpool),
+ sub_iterpool);
+ }
+
SVN_ERR(wrap_external_error(
ctx, item_abspath,
svn_client_export5(NULL, new_url, item_abspath,
The indentation is wrong, it should be more like:
+ /* First notify that we're about to handle an external. */
+ if (ctx-notify_func2)
+ {
+ (*ctx-notify_func2)(
+ ctx-notify_baton2,
+ svn_wc_create_notify(item_abspath,
+ svn_wc_notify_update_external,
+ sub_iterpool),
+ sub_iterpool);
+ }
I noticed the existing code was using a mix of styles:
(*ctx-notify_func2)(...)
(ctx-notify_func2)(...)
ctx-notify_func2(...)
I've changed them all to the last form.
Nice clean-up, Philip.
Other than the indentation, Michael's patch looks like the right fix in the
right place, wouldn't you agree?
Michael, have you tested it by running the full test suite? If there is a test
that covers this then I would expect that its expected output will need to be
adjusted, unless it ignores this notification; and if there isn't any test that
attempts an export with externals, then could you add one?
- Julian
Re: [PATCH] issue #4527: notify start of exporting external
Julian Foad julianf...@btopenworld.com writes: Other than the indentation, Michael's patch looks like the right fix in the right place, wouldn't you agree? Yes, I think it is correct. Extending the patch to fix the in_external nesting problem would be ideal but we could commit this version. Michael, have you tested it by running the full test suite? If there is a test that covers this then I would expect that its expected output will need to be adjusted, unless it ignores this notification; and if there isn't any test that attempts an export with externals, then could you add one? The normal testsuite mechanisms ignore all the output lines affected by the patch. I suppose one could write a special test that checks the lines ignored by the usual test mechanism but any such test would have to work with all the output orderings that export could produce. That might be non-trivial. -- Philip Martin | Subversion Committer WANdisco // *Non-Stop Data*
