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
Received on 2014-11-13 14:36:47 CET