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 2009/07/31 07:31:32 UTC
Re: [PATCH v2] issue 3342 - summary of conflicts and skips
Gah! No patch attached. Strange... Trying again.
> [[[
> Fix issue #3342: Summary of conflicts printed at end of up/sw/merge
> * subversion/svn/merge-cmd.c
> (svn_cl__merge): Call svn_cl__print_conflict_stats.
>
> * subversion/svn/cl.h
> (svn_cl__print_conflict_stats): Added declaration.
>
> * subversion/svn/update-cmd.c
> (svn_cl__update): Call svn_cl__print_conflict_stats.
>
> * subversion/svn/switch-cmd.c
> (svn_cl__switch): Call svn_cl__print_conflict_stats.
>
> * subversion/svn_notify.c
> (svn_cl__print_conflict_stats): Changed name from print_conflict_stats.
>
> * subversion/svn_notify.c
> (notify): Remove references to print_conflict_stats. Do not clear
> counters for conflicts.
> ]]]
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2377277
Re: [PATCH v4] - the right patch!
Posted by Daniel Näslund <da...@longitudo.com>.
Hi again!
The patch I submitted earlier was an old one. Really sorry. I need some
smart way of organazing my patches, mercurial patch queues perhaps?
[[[
Fix issue #3342: Summary of conflicts printed at end of up/sw/merge
* subversion/svn/merge-cmd.c
(svn_cl__merge): Call svn_cl__print_conflict_stats.
* subversion/svn/cl.h
(svn_cl__print_conflict_stats): Declare.
* subversion/svn/update-cmd.c
(svn_cl__update): Call svn_cl__print_conflict_stats.
* subversion/svn/switch-cmd.c
(svn_cl__switch): Call svn_cl__print_conflict_stats.
* subversion/svn_notify.c
(svn_cl__print_conflict_stats): Changed name from print_conflict_stats.
* subversion/svn_notify.c
(notify): Remove references to print_conflict_stats. Do not clear
counters for conflicts.
* subversion/tests/cmdline/basic_tests.py
(basic_update): Changed tests involving skipping to include summary.
]]]
> > Index: subversion/tests/cmdline/basic_tests.py
> > ==================================================================--- subversion/tests/cmdline/basic_tests.py (revision 38504)
> > +++ subversion/tests/cmdline/basic_tests.py (arbetskopia)
> > @@ -198,7 +198,8 @@
> > # path, are skipped and do not raise an error
> > xx_path = os.path.join(wc_dir, 'xx', 'xx')
> > exit_code, out, err = svntest.actions.run_and_verify_svn(
> > - "update xx/xx", ["Skipped '"+xx_path+"'\n"], [],
> > + "update xx/xx",
> > + ["Skipped '"+xx_path+"'\nSummary of conflicts:\n Skipped paths: 1\n"], [],
>
> This won't work.
>
> You need to put each line into a separate element of the list:
>
> exit_code, out, err = svntest.actions.run_and_verify_svn(
> "update xx/xx",
> ["Skipped '"+xx_path+"'\n", "Summary of conflicts:\n",
> " Skipped paths: 1\n"],
> [], 'update', xx_path)
Yep, it's a list. I'm learning python bit by bit.
> When I run this test with that change, I get:
>
> update xx/xx
> EXPECTED STDOUT:
> ACTUAL STDOUT:
> Summary of conflicts in external item:
> Skipped paths: 134639479
> EXCEPTION: SVNLineUnequal
>
> Looks like something isn't initialised somewhere. Can you investigate?
It was caused by this line in update-cmd.c:
if (! opt_state->quiet)
svn_cl__get_notifier(&ctx->notify_func2, &ctx->notify_baton2,
FALSE, FALSE, FALSE, pool);
I added a check if we are operating quietly.
if (! opt_state->quiet)
SVN_ERR(svn_cl__print_conflict_stats(ctx->notify_baton2, pool));
> > Index: subversion/svn/update-cmd.c
> > ==================================================================--- subversion/svn/update-cmd.c (revision 38504)
> >
> > - return svn_client_update3(NULL, targets,
> > + SVN_ERR(svn_client_update3(NULL, targets,
> > &(opt_state->start_revision),
> > depth, depth_is_sticky,
> > opt_state->ignore_externals,
> > opt_state->force,
> > - ctx, pool);
> > + ctx, pool));
>
> Parameter indentation here is off.
Fixed
>
> > Index: subversion/svn/switch-cmd.c
> > ==================================================================--- subversion/svn/switch-cmd.c (revision 38504)
> > + SVN_ERR(svn_client_switch2(NULL, target, switch_url, &peg_revision,
> > &(opt_state->start_revision), depth,
> > depth_is_sticky, opt_state->ignore_externals,
> > - opt_state->force, ctx, pool);
> > + opt_state->force, ctx, pool));
>
> Same.
Fixed
> > Index: subversion/svn/notify.c
> > ==================================================================--- subversion/svn/notify.c (revision 38504)
> >
> > case svn_wc_notify_merge_completed:
> > break;
>
> I'd remove the svn_wc_notify_merge_completed case entirely
> because it does not differ from the default case.
Done.
Mvh
Daniel
Re: [PATCH v4] - the right patch!
Posted by Daniel Näslund <da...@longitudo.com>.
Hi again!
The patch I submitted earlier was an old one. Really sorry. I need some
smart way of organazing my patches, mercurial patch queues perhaps?
I have run make check today with no failures.
[[[
Fix issue #3342: Summary of conflicts printed at end of up/sw/merge
* subversion/svn/merge-cmd.c
(svn_cl__merge): Call svn_cl__print_conflict_stats.
* subversion/svn/cl.h
(svn_cl__print_conflict_stats): Declare.
* subversion/svn/update-cmd.c
(svn_cl__update): Call svn_cl__print_conflict_stats.
* subversion/svn/switch-cmd.c
(svn_cl__switch): Call svn_cl__print_conflict_stats.
* subversion/svn_notify.c
(svn_cl__print_conflict_stats): Changed name from print_conflict_stats.
* subversion/svn_notify.c
(notify): Remove references to print_conflict_stats. Do not clear
counters for conflicts.
* subversion/tests/cmdline/basic_tests.py
(basic_update): Changed tests involving skipping to include summary.
]]]
> > Index: subversion/tests/cmdline/basic_tests.py
> > ==================================================================--- subversion/tests/cmdline/basic_tests.py (revision 38504)
> > +++ subversion/tests/cmdline/basic_tests.py (arbetskopia)
> > @@ -198,7 +198,8 @@
> > # path, are skipped and do not raise an error
> > xx_path = os.path.join(wc_dir, 'xx', 'xx')
> > exit_code, out, err = svntest.actions.run_and_verify_svn(
> > - "update xx/xx", ["Skipped '"+xx_path+"'\n"], [],
> > + "update xx/xx",
> > + ["Skipped '"+xx_path+"'\nSummary of conflicts:\n Skipped paths: 1\n"], [],
>
> This won't work.
>
> You need to put each line into a separate element of the list:
>
> exit_code, out, err = svntest.actions.run_and_verify_svn(
> "update xx/xx",
> ["Skipped '"+xx_path+"'\n", "Summary of conflicts:\n",
> " Skipped paths: 1\n"],
> [], 'update', xx_path)
Yep, it's a list. I'm learning python bit by bit.
> When I run this test with that change, I get:
>
> update xx/xx
> EXPECTED STDOUT:
> ACTUAL STDOUT:
> Summary of conflicts in external item:
> Skipped paths: 134639479
> EXCEPTION: SVNLineUnequal
>
> Looks like something isn't initialised somewhere. Can you investigate?
It was caused by this line in update-cmd.c:
if (! opt_state->quiet)
svn_cl__get_notifier(&ctx->notify_func2, &ctx->notify_baton2,
FALSE, FALSE, FALSE, pool);
I added a check if we are operating quietly.
if (! opt_state->quiet)
SVN_ERR(svn_cl__print_conflict_stats(ctx->notify_baton2, pool));
> > Index: subversion/svn/update-cmd.c
> > ==================================================================--- subversion/svn/update-cmd.c (revision 38504)
> >
> > - return svn_client_update3(NULL, targets,
> > + SVN_ERR(svn_client_update3(NULL, targets,
> > &(opt_state->start_revision),
> > depth, depth_is_sticky,
> > opt_state->ignore_externals,
> > opt_state->force,
> > - ctx, pool);
> > + ctx, pool));
>
> Parameter indentation here is off.
Fixed
>
> > Index: subversion/svn/switch-cmd.c
> > ==================================================================--- subversion/svn/switch-cmd.c (revision 38504)
> > + SVN_ERR(svn_client_switch2(NULL, target, switch_url, &peg_revision,
> > &(opt_state->start_revision), depth,
> > depth_is_sticky, opt_state->ignore_externals,
> > - opt_state->force, ctx, pool);
> > + opt_state->force, ctx, pool));
>
> Same.
Fixed
> > Index: subversion/svn/notify.c
> > ==================================================================--- subversion/svn/notify.c (revision 38504)
> >
> > case svn_wc_notify_merge_completed:
> > break;
>
> I'd remove the svn_wc_notify_merge_completed case entirely
> because it does not differ from the default case.
Done.
Mvh
Daniel
Re: [PATCH v4] - right patch!
Posted by Daniel Näslund <da...@longitudo.com>.
Hi again!
The patch I submitted earlier was an old one. Really sorry. I need some
smart way of organazing my patches, mercurial patch queues perhaps?
[[[
Fix issue #3342: Summary of conflicts printed at end of up/sw/merge
* subversion/svn/merge-cmd.c
(svn_cl__merge): Call svn_cl__print_conflict_stats.
* subversion/svn/cl.h
(svn_cl__print_conflict_stats): Declare.
* subversion/svn/update-cmd.c
(svn_cl__update): Call svn_cl__print_conflict_stats.
* subversion/svn/switch-cmd.c
(svn_cl__switch): Call svn_cl__print_conflict_stats.
* subversion/svn_notify.c
(svn_cl__print_conflict_stats): Changed name from print_conflict_stats.
* subversion/svn_notify.c
(notify): Remove references to print_conflict_stats. Do not clear
counters for conflicts.
* subversion/tests/cmdline/basic_tests.py
(basic_update): Changed tests involving skipping to include summary.
]]]
> > Index: subversion/tests/cmdline/basic_tests.py
> > ==================================================================--- subversion/tests/cmdline/basic_tests.py (revision 38504)
> > +++ subversion/tests/cmdline/basic_tests.py (arbetskopia)
> > @@ -198,7 +198,8 @@
> > # path, are skipped and do not raise an error
> > xx_path = os.path.join(wc_dir, 'xx', 'xx')
> > exit_code, out, err = svntest.actions.run_and_verify_svn(
> > - "update xx/xx", ["Skipped '"+xx_path+"'\n"], [],
> > + "update xx/xx",
> > + ["Skipped '"+xx_path+"'\nSummary of conflicts:\n Skipped paths: 1\n"], [],
>
> This won't work.
>
> You need to put each line into a separate element of the list:
>
> exit_code, out, err = svntest.actions.run_and_verify_svn(
> "update xx/xx",
> ["Skipped '"+xx_path+"'\n", "Summary of conflicts:\n",
> " Skipped paths: 1\n"],
> [], 'update', xx_path)
Yep, it's a list. I'm learning python bit by bit.
> When I run this test with that change, I get:
>
> update xx/xx
> EXPECTED STDOUT:
> ACTUAL STDOUT:
> Summary of conflicts in external item:
> Skipped paths: 134639479
> EXCEPTION: SVNLineUnequal
>
> Looks like something isn't initialised somewhere. Can you investigate?
It was caused by this line in update-cmd.c:
if (! opt_state->quiet)
svn_cl__get_notifier(&ctx->notify_func2, &ctx->notify_baton2,
FALSE, FALSE, FALSE, pool);
I added a check if we are operating quietly.
if (! opt_state->quiet)
SVN_ERR(svn_cl__print_conflict_stats(ctx->notify_baton2, pool));
> > Index: subversion/svn/update-cmd.c
> > ==================================================================--- subversion/svn/update-cmd.c (revision 38504)
> >
> > - return svn_client_update3(NULL, targets,
> > + SVN_ERR(svn_client_update3(NULL, targets,
> > &(opt_state->start_revision),
> > depth, depth_is_sticky,
> > opt_state->ignore_externals,
> > opt_state->force,
> > - ctx, pool);
> > + ctx, pool));
>
> Parameter indentation here is off.
Fixed
>
> > Index: subversion/svn/switch-cmd.c
> > ==================================================================--- subversion/svn/switch-cmd.c (revision 38504)
> > + SVN_ERR(svn_client_switch2(NULL, target, switch_url, &peg_revision,
> > &(opt_state->start_revision), depth,
> > depth_is_sticky, opt_state->ignore_externals,
> > - opt_state->force, ctx, pool);
> > + opt_state->force, ctx, pool));
>
> Same.
Fixed
> > Index: subversion/svn/notify.c
> > ==================================================================--- subversion/svn/notify.c (revision 38504)
> >
> > case svn_wc_notify_merge_completed:
> > break;
>
> I'd remove the svn_wc_notify_merge_completed case entirely
> because it does not differ from the default case.
Done.
Mvh
Daniel
Re: [PATCH v4] - issue 3342 summary of conflicts and skips
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Aug 07, 2009 at 06:44:24PM +0200, Daniel Näslund wrote:
>
> Ping!
> This patch hasn't received any attention in a couple of days.
>
> Sorry about the diff headers. Tigris concatenates some line in the diffs
> for me. (I blame tigris for everything :-))
>
> Mvh
> Daniel
>
> > Thanks! My python is a bit rusty (hrm now I know some).
> >
> > Make check passed but with two failures in basic_tests.py. When skipping
> > no summary was printed before. That was caused by theese lines in
> > subversion/libsvn_client/update.c:
> >
> > 344 err = svn_client__update_internal(&result_rev, path, revision, depth,
> > 345 depth_is_sticky, ignore_externals,
> > 346 allow_unver_obstructions,
> > 347 &sleep, TRUE, ctx, subpool);
> > 348 if (err && err->apr_err != SVN_ERR_WC_NOT_DIRECTORY)
> > 349 {
> > 350 return svn_error_return(err);
> > 351 }
> > 352 else if (err)
> > 353 {
> > 354 /* SVN_ERR_WC_NOT_DIRECTORY: it's not versioned */
> > 355 svn_error_clear(err);
> > 356 err = SVN_NO_ERROR;
> > 357 result_rev = SVN_INVALID_REVNUM;
> > 358 if (ctx->notify_func2)
> > 359 (*ctx->notify_func2)(ctx->notify_baton2,
> > 360 svn_wc_create_notify(path,
> > 361 svn_wc_notify_skip,
> > 362 subpool), subpool);
> > 363 }
> >
> > A svn_wc_notify_skip was sent after the svn_wc_notify_update_completed.
> > But now when the summary is not printed in response to an update
> > completed notification but after the call to svn_client_update3 a
> > summary will be printed even if the only action is skipping one
> > directory. Since the summary includes a line for skips I think that the
> > test should be changed and has done so. The patch now passes make
> > check.
>
> [[[
> Fix issue #3342: Summary of conflicts printed at end of up/sw/merge
> * subversion/svn/merge-cmd.c
> (svn_cl__merge): Call svn_cl__print_conflict_stats.
>
> * subversion/svn/cl.h
> (svn_cl__print_conflict_stats): Declare.
>
> * subversion/svn/update-cmd.c
> (svn_cl__update): Call svn_cl__print_conflict_stats.
>
> * subversion/svn/switch-cmd.c
> (svn_cl__switch): Call svn_cl__print_conflict_stats.
>
> * subversion/svn_notify.c
> (svn_cl__print_conflict_stats): Changed name from print_conflict_stats.
>
> * subversion/svn_notify.c
> (notify): Remove references to print_conflict_stats. Do not clear
> counters for conflicts.
>
> * subversion/tests/cmdline/basic_tests.py
> (basic_update): Changed tests involving skipping to include summary.
> ]]]
>
> Mvh
> Daniel Näslund
> Index: subversion/tests/cmdline/basic_tests.py
> ==================================================================--- subversion/tests/cmdline/basic_tests.py (revision 38504)
> +++ subversion/tests/cmdline/basic_tests.py (arbetskopia)
> @@ -198,7 +198,8 @@
> # path, are skipped and do not raise an error
> xx_path = os.path.join(wc_dir, 'xx', 'xx')
> exit_code, out, err = svntest.actions.run_and_verify_svn(
> - "update xx/xx", ["Skipped '"+xx_path+"'\n"], [],
> + "update xx/xx",
> + ["Skipped '"+xx_path+"'\nSummary of conflicts:\n Skipped paths: 1\n"], [],
This won't work.
You need to put each line into a separate element of the list:
exit_code, out, err = svntest.actions.run_and_verify_svn(
"update xx/xx",
["Skipped '"+xx_path+"'\n", "Summary of conflicts:\n",
" Skipped paths: 1\n"],
[], 'update', xx_path)
When I run this test with that change, I get:
update xx/xx
EXPECTED STDOUT:
ACTUAL STDOUT:
Summary of conflicts in external item:
Skipped paths: 134639479
EXCEPTION: SVNLineUnequal
Looks like something isn't initialised somewhere. Can you investigate?
> 'update', xx_path)
> exit_code, out, err = svntest.actions.run_and_verify_svn(
> "update xx/xx", [], [],
> Index: subversion/svn/merge-cmd.c
> ==================================================================--- subversion/svn/merge-cmd.c (revision 38504)
> +++ subversion/svn/merge-cmd.c (arbetskopia)
> @@ -340,6 +340,8 @@
> pool);
> }
>
> + SVN_ERR(svn_cl__print_conflict_stats(ctx->notify_baton2, pool));
> +
> if (err && (! opt_state->reintegrate))
> return svn_cl__may_need_force(err);
>
> Index: subversion/svn/cl.h
> ==================================================================--- subversion/svn/cl.h (revision 38504)
> +++ subversion/svn/cl.h (arbetskopia)
> @@ -537,6 +537,12 @@
> svn_boolean_t suppress_final_line,
> apr_pool_t *pool);
>
> +/* Print conflict stats accumulated in NOTIFY_BATON.
> + * Return any error encountered during printing.
> + * Do all allocations in POOL.*/
> +svn_error_t *
> +svn_cl__print_conflict_stats(void *notify_baton, apr_pool_t *pool);
> +
>
> /*** Log message callback stuffs. ***/
>
> Index: subversion/svn/update-cmd.c
> ==================================================================--- subversion/svn/update-cmd.c (revision 38504)
> +++ subversion/svn/update-cmd.c (arbetskopia)
> @@ -88,10 +88,14 @@
> depth_is_sticky = FALSE;
> }
>
> - return svn_client_update3(NULL, targets,
> + SVN_ERR(svn_client_update3(NULL, targets,
> &(opt_state->start_revision),
> depth, depth_is_sticky,
> opt_state->ignore_externals,
> opt_state->force,
> - ctx, pool);
> + ctx, pool));
Parameter indentation here is off.
> +
> + SVN_ERR(svn_cl__print_conflict_stats(ctx->notify_baton2, pool));
> +
> + return SVN_NO_ERROR;
> }
> Index: subversion/svn/switch-cmd.c
> ==================================================================--- subversion/svn/switch-cmd.c (revision 38504)
> +++ subversion/svn/switch-cmd.c (arbetskopia)
> @@ -161,8 +161,12 @@
> }
>
> /* Do the 'switch' update. */
> - return svn_client_switch2(NULL, target, switch_url, &peg_revision,
> + SVN_ERR(svn_client_switch2(NULL, target, switch_url, &peg_revision,
> &(opt_state->start_revision), depth,
> depth_is_sticky, opt_state->ignore_externals,
> - opt_state->force, ctx, pool);
> + opt_state->force, ctx, pool));
Same.
> +
> + SVN_ERR(svn_cl__print_conflict_stats(ctx->notify_baton2, pool));
> +
> + return SVN_NO_ERROR;
> }
> Index: subversion/svn/notify.c
> ==================================================================--- subversion/svn/notify.c (revision 38504)
> +++ subversion/svn/notify.c (arbetskopia)
> @@ -69,12 +69,10 @@
> };
>
>
> -/* Print conflict stats accumulated in notify baton NB.
> - * Return any error encountered during printing.
> - * Do all allocations in POOL.*/
> -static svn_error_t *
> -print_conflict_stats(struct notify_baton *nb, apr_pool_t *pool)
> +svn_error_t *
> +svn_cl__print_conflict_stats(void *notify_baton, apr_pool_t *pool)
> {
> + struct notify_baton *nb = notify_baton;
> const char *header;
> unsigned int text_conflicts;
> unsigned int prop_conflicts;
> @@ -449,27 +447,15 @@
> }
> }
>
> - if ((err = print_conflict_stats(nb, pool)))
> - goto print_error;
> -
> if (nb->in_external)
> {
> nb->in_external = FALSE;
> - nb->ext_text_conflicts = nb->ext_prop_conflicts
> - = nb->ext_tree_conflicts = nb->ext_skipped_paths = 0;
> if ((err = svn_cmdline_printf(pool, "\n")))
> goto print_error;
> }
> - else
> - nb->text_conflicts = nb->prop_conflicts
> - = nb->tree_conflicts = nb->skipped_paths = 0;
> break;
>
> case svn_wc_notify_merge_completed:
> - if ((err = print_conflict_stats(nb, pool)))
> - goto print_error;
> - nb->text_conflicts = nb->prop_conflicts
> - = nb->tree_conflicts = nb->skipped_paths = 0;
> break;
I'd remove the svn_wc_notify_merge_completed case entirely
because it does not differ from the default case.
Stefan
>
> case svn_wc_notify_status_external:
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2381406
Re: [PATCH v4] - issue 3342 summary of conflicts and skips
Posted by Daniel Näslund <da...@longitudo.com>.
Ping!
This patch hasn't received any attention in a couple of days.
Sorry about the diff headers. Tigris concatenates some line in the diffs
for me. (I blame tigris for everything :-))
Mvh
Daniel
> Thanks! My python is a bit rusty (hrm now I know some).
>
> Make check passed but with two failures in basic_tests.py. When skipping
> no summary was printed before. That was caused by theese lines in
> subversion/libsvn_client/update.c:
>
> 344 err = svn_client__update_internal(&result_rev, path, revision, depth,
> 345 depth_is_sticky, ignore_externals,
> 346 allow_unver_obstructions,
> 347 &sleep, TRUE, ctx, subpool);
> 348 if (err && err->apr_err != SVN_ERR_WC_NOT_DIRECTORY)
> 349 {
> 350 return svn_error_return(err);
> 351 }
> 352 else if (err)
> 353 {
> 354 /* SVN_ERR_WC_NOT_DIRECTORY: it's not versioned */
> 355 svn_error_clear(err);
> 356 err = SVN_NO_ERROR;
> 357 result_rev = SVN_INVALID_REVNUM;
> 358 if (ctx->notify_func2)
> 359 (*ctx->notify_func2)(ctx->notify_baton2,
> 360 svn_wc_create_notify(path,
> 361 svn_wc_notify_skip,
> 362 subpool), subpool);
> 363 }
>
> A svn_wc_notify_skip was sent after the svn_wc_notify_update_completed.
> But now when the summary is not printed in response to an update
> completed notification but after the call to svn_client_update3 a
> summary will be printed even if the only action is skipping one
> directory. Since the summary includes a line for skips I think that the
> test should be changed and has done so. The patch now passes make
> check.
[[[
Fix issue #3342: Summary of conflicts printed at end of up/sw/merge
* subversion/svn/merge-cmd.c
(svn_cl__merge): Call svn_cl__print_conflict_stats.
* subversion/svn/cl.h
(svn_cl__print_conflict_stats): Declare.
* subversion/svn/update-cmd.c
(svn_cl__update): Call svn_cl__print_conflict_stats.
* subversion/svn/switch-cmd.c
(svn_cl__switch): Call svn_cl__print_conflict_stats.
* subversion/svn_notify.c
(svn_cl__print_conflict_stats): Changed name from print_conflict_stats.
* subversion/svn_notify.c
(notify): Remove references to print_conflict_stats. Do not clear
counters for conflicts.
* subversion/tests/cmdline/basic_tests.py
(basic_update): Changed tests involving skipping to include summary.
]]]
Mvh
Daniel Näslund
[PATCH v4] - issue 3342 summary of conflicts and skips
Posted by Daniel Näslund <da...@longitudo.com>.
> > ===================================================================
> > --- basic_tests.py (revision 38504)
> > +++ basic_tests.py (arbetskopia)
> > @@ -198,7 +198,8 @@
> > # path, are skipped and do not raise an error
> > xx_path = os.path.join(wc_dir, 'xx', 'xx')
> > exit_code, out, err = svntest.actions.run_and_verify_svn(
> > - "update xx/xx", ["Skipped '"+xx_path+"'\n"], [],
> > + "update xx/xx",
> > + ["Skipped '"+xx_path+"'\nSummary of conflicts:\n Skipped paths: 1\n"], [],
> > 'update', xx_path)
> > exit_code, out, err = svntest.actions.run_and_verify_svn(
> > "update xx/xx", [], [],
> > The funny thing is that I can't spot any diff between the expected and
> > actual output. Line endings? Nothing shows up in vim. UTF-8?
> >
>
> The first is three one-line strings, the second is one three-lines string?
>
> (Look for examples how run_and_verify_svn() is used with multiline values.)
Thanks! My python is a bit rusty (hrm now I know some).
Make check passed but with two failures in basic_tests.py. When skipping
no summary was printed before. That was caused by theese lines in
subversion/libsvn_client/update.c:
344 err = svn_client__update_internal(&result_rev, path, revision, depth,
345 depth_is_sticky, ignore_externals,
346 allow_unver_obstructions,
347 &sleep, TRUE, ctx, subpool);
348 if (err && err->apr_err != SVN_ERR_WC_NOT_DIRECTORY)
349 {
350 return svn_error_return(err);
351 }
352 else if (err)
353 {
354 /* SVN_ERR_WC_NOT_DIRECTORY: it's not versioned */
355 svn_error_clear(err);
356 err = SVN_NO_ERROR;
357 result_rev = SVN_INVALID_REVNUM;
358 if (ctx->notify_func2)
359 (*ctx->notify_func2)(ctx->notify_baton2,
360 svn_wc_create_notify(path,
361 svn_wc_notify_skip,
362 subpool), subpool);
363 }
A svn_wc_notify_skip was sent after the svn_wc_notify_update_completed.
But now when the summary is not printed in response to an update
completed notification but after the call to svn_client_update3 a
summary will be printed even if the only action is skipping one
directory. Since the summary includes a line for skips I think that the
test should be changed and has done so. The patch now passes make
check.
[[[
Fix issue #3342: Summary of conflicts printed at end of up/sw/merge
* subversion/svn/merge-cmd.c
(svn_cl__merge): Call svn_cl__print_conflict_stats.
* subversion/svn/cl.h
(svn_cl__print_conflict_stats): Added declaration.
* subversion/svn/update-cmd.c
(svn_cl__update): Call svn_cl__print_conflict_stats.
* subversion/svn/switch-cmd.c
(svn_cl__switch): Call svn_cl__print_conflict_stats.
* subversion/svn_notify.c
(svn_cl__print_conflict_stats): Changed name from print_conflict_stats.
* subversion/svn_notify.c
(notify): Remove references to print_conflict_stats. Do not clear
counters for conflicts.
* subversion/tests/cmdline/basic_tests.py
(basic_update): Changed tests involving skipping to include summary.
]]]
Mvh
Daniel Näslund
Re: basic_tests.py 4: basic update command fails
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Daniel Näslund wrote on Fri, 31 Jul 2009 at 17:45 +0200:
> Index: basic_tests.py
> ===================================================================
> --- basic_tests.py (revision 38504)
> +++ basic_tests.py (arbetskopia)
> @@ -198,7 +198,8 @@
> # path, are skipped and do not raise an error
> xx_path = os.path.join(wc_dir, 'xx', 'xx')
> exit_code, out, err = svntest.actions.run_and_verify_svn(
> - "update xx/xx", ["Skipped '"+xx_path+"'\n"], [],
> + "update xx/xx",
> + ["Skipped '"+xx_path+"'\nSummary of conflicts:\n Skipped paths: 1\n"], [],
> 'update', xx_path)
> exit_code, out, err = svntest.actions.run_and_verify_svn(
> "update xx/xx", [], [],
>
> but I got this:
>
> EXPECTED STDOUT:
> Skipped 'svn-test-work/working_copies/basic_tests-4/xx/xx'
> Summary of conflicts:
> Skipped paths: 1
> ACTUAL STDOUT:
> Skipped 'svn-test-work/working_copies/basic_tests-4/xx/xx'
> Summary of conflicts:
> Skipped paths: 1
> EXCEPTION: SVNLineUnequal
>
> The funny thing is that I can't spot any diff between the expected and
> actual output. Line endings? Nothing shows up in vim. UTF-8?
>
The first is three one-line strings, the second is one three-lines string?
(Look for examples how run_and_verify_svn() is used with multiline values.)
> Mvh
> Daniel
>
> ------------------------------------------------------
> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2377459
>
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2377641
basic_tests.py 4: basic update command fails
Posted by Daniel Näslund <da...@longitudo.com>.
> Finally, can you confirm that you have run the test suite ("make
> check")? I don't know whether there are any tests that would detect this
> change.
basic_tests.py 4 tries to update an unversioned path which leads to a
'skip'. Then a summary is printed which the test did not expect. I
thought that it would have printed a summary even without my patch? But
the test says different. What am I missing?
The test says this:
EXPECTED STDOUT:
Skipped 'svn-test-work/working_copies/basic_tests-4/xx/xx'
ACTUAL STDOUT:
Skipped 'svn-test-work/working_copies/basic_tests-4/xx/xx'
Summary of conflicts:
Skipped paths: 1
If the test should be changed, then I'm not the man for it. I tried
this:
Index: basic_tests.py
===================================================================
--- basic_tests.py (revision 38504)
+++ basic_tests.py (arbetskopia)
@@ -198,7 +198,8 @@
# path, are skipped and do not raise an error
xx_path = os.path.join(wc_dir, 'xx', 'xx')
exit_code, out, err = svntest.actions.run_and_verify_svn(
- "update xx/xx", ["Skipped '"+xx_path+"'\n"], [],
+ "update xx/xx",
+ ["Skipped '"+xx_path+"'\nSummary of conflicts:\n Skipped paths: 1\n"], [],
'update', xx_path)
exit_code, out, err = svntest.actions.run_and_verify_svn(
"update xx/xx", [], [],
but I got this:
EXPECTED STDOUT:
Skipped 'svn-test-work/working_copies/basic_tests-4/xx/xx'
Summary of conflicts:
Skipped paths: 1
ACTUAL STDOUT:
Skipped 'svn-test-work/working_copies/basic_tests-4/xx/xx'
Summary of conflicts:
Skipped paths: 1
EXCEPTION: SVNLineUnequal
The funny thing is that I can't spot any diff between the expected and
actual output. Line endings? Nothing shows up in vim. UTF-8?
Mvh
Daniel
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2377459
Re: [PATCH v3] - summary of conflicts and skips
Posted by Julian Foad <ju...@btopenworld.com>.
Daniel Näslund wrote:
> I have tried to follow your receipe. I couldn't find a way for switch to
> use several targets though. The second usage form "switch --relocate
> FROM TO [PATH...]" seems to only be about changing the metadata of the
> wc.
Oh yes, you're right.
Daniel Näslund wrote:
> Double gah! Still no patch, just the original message. I'm a mutt user.
> Could the new patch to tigris be the cause? Trying with a diff ending.
(The patch to Tigris was intended to fix problems in this area, I think.
I don't know if it was intended to fix this particular problem, or if it
might have caused it.)
> > Gah! No patch attached. Strange... Trying again.
> [[[
> Fix issue #3342: Summary of conflicts printed at end of up/sw/merge
> * subversion/svn/merge-cmd.c
> (svn_cl__merge): Call svn_cl__print_conflict_stats.
>
> * subversion/svn/cl.h
> (svn_cl__print_conflict_stats): Added declaration.
>
> * subversion/svn/update-cmd.c
> (svn_cl__update): Call svn_cl__print_conflict_stats.
>
> * subversion/svn/switch-cmd.c
> (svn_cl__switch): Call svn_cl__print_conflict_stats.
>
> * subversion/svn_notify.c
> (svn_cl__print_conflict_stats): Changed name from print_conflict_stats.
>
> * subversion/svn_notify.c
> (notify): Remove references to print_conflict_stats. Do not clear
> counters for conflicts.
> ]]]
This looks great except for a few small things:
> Index: subversion/svn/update-cmd.c
> ==================================================================
>
> - return svn_client_update3(NULL, targets,
> + svn_error_t *err = svn_client_update3(NULL, targets,
> &(opt_state->start_revision),
> depth, depth_is_sticky,
> opt_state->ignore_externals,
> opt_state->force,
> ctx, pool);
We use plain C (C'89/90) in which you can't declare a new variable part
way through the code.
However, we don't need an 'err' variable here. If the call to
svn_client_update3() returns an error, we want to return that error
straight away, and the way to do that is to wrap the call in SVN_ERR:
SVN_ERR(svn_client_update3(...));
Also, please re-indent the follow-on lines to line up with the first
argument, "NULL".
> + svn_cl__print_conflict_stats(ctx->notify_baton2, pool);
This function can also return an error, so you must catch the error.
Again, use SVN_ERR(svn_cl__...(...)).
Of course, the same comments apply to other parts of the patch.
> +svn_error_t *
> +svn_cl__print_conflict_stats(void *notify_baton, apr_pool_t *pool)
> {
> + struct notify_baton *nb = (struct notify_baton*) notify_baton;
You don't need the explicit type cast.
> - if (nb->in_external)
> - {
> - nb->in_external = FALSE;
> - nb->ext_text_conflicts = nb->ext_prop_conflicts
> - = nb->ext_tree_conflicts = nb->ext_skipped_paths = 0;
> - if ((err = svn_cmdline_printf(pool, "\n")))
> - goto print_error;
> - }
You removed this code but I think most of it is still needed. I think
you should have removed only the part that resets the conflict counters.
> @@ -699,7 +678,6 @@
> svn_error_clear(err);
> }
>
> -
> void
> svn_cl__get_notifier(svn_wc_notify_func2_t *notify_func_p,
> void **notify_baton_p,
This is trivial, of course, but it's nicer if you don't include
irrelevant changes like this.
Finally, can you confirm that you have run the test suite ("make
check")? I don't know whether there are any tests that would detect this
change.
Stefan and Edmund are still thinking about moving this into a lower
layer, but I hope they will agree that it is fine to go ahead with your
patch in the meantime, as your patch won't make the move any more
difficult if we do decide to move it.
Thanks.
- Julian
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2377304
Re: [PATCH v3] - summary of conflicts and skips
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Jul 31, 2009 at 09:38:03AM +0200, Daniel Näslund wrote:
> Double gah! Still no patch, just the original message. I'm a mutt user.
> Could the new patch to tigris be the cause? Trying with a diff ending.
Looks like the patch is incomplete, the changes to svn_notify aren't
there.
> [[[
> Fix issue #3342: Summary of conflicts printed at end of up/sw/merge
> * subversion/svn/merge-cmd.c
> (svn_cl__merge): Call svn_cl__print_conflict_stats.
Please indent your log messages like this:
[[[
* subversion/somedir/somefile.c
(some_function): State nature and reason of the change here.
Next line goes here (one or two leading spaces).
(some_other_function): State nature and reason of the change here.
]]]
And no tabs anywhere (you had a tab before "(svn_cl__merge)").
Thanks,
Stefan
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2377298
Re: [PATCH v3] - summary of conflicts and skips
Posted by Daniel Näslund <da...@longitudo.com>.
Double gah! Still no patch, just the original message. I'm a mutt user.
Could the new patch to tigris be the cause? Trying with a diff ending.
> Gah! No patch attached. Strange... Trying again.
[[[
Fix issue #3342: Summary of conflicts printed at end of up/sw/merge
* subversion/svn/merge-cmd.c
(svn_cl__merge): Call svn_cl__print_conflict_stats.
* subversion/svn/cl.h
(svn_cl__print_conflict_stats): Added declaration.
* subversion/svn/update-cmd.c
(svn_cl__update): Call svn_cl__print_conflict_stats.
* subversion/svn/switch-cmd.c
(svn_cl__switch): Call svn_cl__print_conflict_stats.
* subversion/svn_notify.c
(svn_cl__print_conflict_stats): Changed name from print_conflict_stats.
* subversion/svn_notify.c
(notify): Remove references to print_conflict_stats. Do not clear
counters for conflicts.
]]]
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2377278
Re: Patch mangling [was: [PATCH v2] issue 3342 - summary of
conflicts and skips]
Posted by Mark Phippard <ma...@gmail.com>.
Thanks for the details. The proper fix has been identified and we
will patch tigris ASAP. I will send out an email shortly with details
on when the site will be bounced to apply the update.
On Fri, Jul 31, 2009 at 9:08 AM, Julian Foad<ju...@btopenworld.com> wrote:
> On Fri, 2009-07-31, Mark Phippard wrote:
>> I've gotten the patch in GMail on all of the messages you have sent.
>
> The first message he sent with that patch is in the Tigris archive here:
> <http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2377275>
>
> On that web page it shows as having just one attachment, the shell
> script named "test_3342.sh". But it was supposed to have two
> attachments: the source code patch named "patch_issue_3342" and the
> shell script.
>
> The copy I received (via the list) of that message contained two
> attachments with the right names, but the first attachment, named
> "patch_issue_3342", contained a copy of the email body instead of a
> patch.
>
> - Julian
>
>
>
--
Thanks
Mark Phippard
http://markphip.blogspot.com/
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2377497
Re: Patch mangling [was: [PATCH v2] issue 3342 - summary of
conflicts and skips]
Posted by Mark Phippard <ma...@gmail.com>.
On Fri, Jul 31, 2009 at 9:08 AM, Julian Foad<ju...@btopenworld.com> wrote:
> On Fri, 2009-07-31, Mark Phippard wrote:
>> I've gotten the patch in GMail on all of the messages you have sent.
>
> The first message he sent with that patch is in the Tigris archive here:
> <http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2377275>
>
> On that web page it shows as having just one attachment, the shell
> script named "test_3342.sh". But it was supposed to have two
> attachments: the source code patch named "patch_issue_3342" and the
> shell script.
>
> The copy I received (via the list) of that message contained two
> attachments with the right names, but the first attachment, named
> "patch_issue_3342", contained a copy of the email body instead of a
> patch.
Thanks. I will report this internally.
--
Thanks
Mark Phippard
http://markphip.blogspot.com/
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2377391
Patch mangling [was: [PATCH v2] issue 3342 - summary of conflicts
and skips]
Posted by Julian Foad <ju...@btopenworld.com>.
On Fri, 2009-07-31, Mark Phippard wrote:
> I've gotten the patch in GMail on all of the messages you have sent.
The first message he sent with that patch is in the Tigris archive here:
<http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2377275>
On that web page it shows as having just one attachment, the shell
script named "test_3342.sh". But it was supposed to have two
attachments: the source code patch named "patch_issue_3342" and the
shell script.
The copy I received (via the list) of that message contained two
attachments with the right names, but the first attachment, named
"patch_issue_3342", contained a copy of the email body instead of a
patch.
- Julian
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2377378
Re: [PATCH v2] issue 3342 - summary of conflicts and skips
Posted by Mark Phippard <ma...@gmail.com>.
I've gotten the patch in GMail on all of the messages you have sent.
On Fri, Jul 31, 2009 at 3:31 AM, Daniel Näslund<da...@longitudo.com> wrote:
> Gah! No patch attached. Strange... Trying again.
>> [[[
>> Fix issue #3342: Summary of conflicts printed at end of up/sw/merge
>> * subversion/svn/merge-cmd.c
>> (svn_cl__merge): Call svn_cl__print_conflict_stats.
>>
>> * subversion/svn/cl.h
>> (svn_cl__print_conflict_stats): Added declaration.
>>
>> * subversion/svn/update-cmd.c
>> (svn_cl__update): Call svn_cl__print_conflict_stats.
>>
>> * subversion/svn/switch-cmd.c
>> (svn_cl__switch): Call svn_cl__print_conflict_stats.
>>
>> * subversion/svn_notify.c
>> (svn_cl__print_conflict_stats): Changed name from print_conflict_stats.
>>
>> * subversion/svn_notify.c
>> (notify): Remove references to print_conflict_stats. Do not clear
>> counters for conflicts.
>> ]]]
>
> ------------------------------------------------------
> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2377277
--
Thanks
Mark Phippard
http://markphip.blogspot.com/
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2377352