You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2009/07/30 16:59:23 UTC
Re: issue 3342 - summary of conflicts and skips [was: ... 3432 ...]
Hi Daniel.
Thanks for your interest.
Issue #3342 <http://subversion.tigris.org/issues/show_bug.cgi?id=3342>
is not talking about the "G foo1" notifications. It is about the message
that says
[[[
Summary of conflicts:
Text conflicts: 2
Property conflicts: 1
Skipped paths: 10
]]]
Sorry - that's my fault for not being clear when I wrote it. I've just
added a note to the issue to clarify this.
To fix this, I would change the notifier function in
subversion/svn/notify.c so that it still collects the statistics in
'nb->text_conflicts' etc., but does NOT call print_conflict_stats() when
it gets an 'update_completed' or 'merge_completed' notification.
Instead, make print_conflict_stats() a public function (named
svn_cl__print_conflict_stats()) so that the top-level update function
(svn_cl__update()) can call it to print the stats when the whole update
is finished.
Then adjust the callers (update, switch and merge) to call
svn_cl__print_conflict_stats() after finishing the update/switch/merge
operation.
Does that make sense?
- Julian
Daniel Näslund wrote:
> If I do an update s summary is printed. If there are conflicts a summary
> for each file is printed after each interactive operation. If many
> files, the summary may scroll off the screen. An example:
>
> test2> s up
> Conflict discovered in 'foo1'.
> Select: (p) postpone, (df) diff-full, (e) edit,
> (mc) mine-conflict, (tc) theirs-conflict,
> (s) show all options: mc
> G foo1
> Conflict discovered in 'foo2'.
> Select: (p) postpone, (df) diff-full, (e) edit,
> (mc) mine-conflict, (tc) theirs-conflict,
> (s) show all options: mc
> G foo2
> Updated to revision 2.
>
> Better would be to have this printed at the bottom:
> Conflict discovered in 'foo1'.
> Select: (p) postpone, (df) diff-full, (e) edit,
> (mc) mine-conflict, (tc) theirs-conflict,
> (s) show all options: mc
> Conflict discovered in 'foo2'.
> Select: (p) postpone, (df) diff-full, (e) edit,
> (mc) mine-conflict, (tc) theirs-conflict,
> (s) show all options: mc
> G foo1
> G foo2
> Updated to revision 2.
>
> The summary is now at the END!
I don't think the per-file notifications should be grouped together at
the end. (They are a form of progress reporting.)
I agree that just a single "Updated to revision REV." line should be
printed at the end
> In subversion/svn/update-cmd.c a notify()-function is registered which
> gets called whenever there is new information that libsvn_client wants
> to pass through. My idea was to create another notify()-callback inside
> libsvn_client that could store the notifications the command produces
> and then at the end call the original notify() function in svn. That way
> all notifications would get printed last. Is this a working solution?
>
> I assume that there are notifications which needs to be sent at exactly
> the right time. How do I know which notifications that can be sent at
> the end?
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2377047
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
Re: [PATCH v2] issue 3342 - summary of conflicts and skips
Posted by Daniel Näslund <da...@longitudo.com>.
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
[PATCH] issue 3342 - summary of conflicts and skips
Posted by Daniel Näslund <da...@longitudo.com>.
> Issue #3342 <http://subversion.tigris.org/issues/show_bug.cgi?id=3342>
> is not talking about the "G foo1" notifications. It is about the message
> that says
>
> [[[
> Summary of conflicts:
> Text conflicts: 2
> Property conflicts: 1
> Skipped paths: 10
> ]]]
>
> To fix this, I would change the notifier function in
> subversion/svn/notify.c so that it still collects the statistics in
> 'nb->text_conflicts' etc., but does NOT call print_conflict_stats() when
> it gets an 'update_completed' or 'merge_completed' notification.
> Instead, make print_conflict_stats() a public function (named
> svn_cl__print_conflict_stats()) so that the top-level update function
> (svn_cl__update()) can call it to print the stats when the whole update
> is finished.
>
> Then adjust the callers (update, switch and merge) to call
> svn_cl__print_conflict_stats() after finishing the update/switch/merge
> operation.
>
> Does that make sense?
>
> - Julian
Yes. 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.
I also attached a simple script I used for testing update/merge with
multple targets. There is a switch option too but it only uses a single
target.
[[[
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.
]]]
Mvh
Daniel Näslund
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2377275
Re: issue 3342 - summary of conflicts and skips [was: ... 3432 ...]
Posted by Edmund Wong <ed...@belfordhk.com>.
Stefan Sperling wrote:
>
> OK. Note that Edmund Wong has been working on this issue, too,
> and I told him to do the stat gathering in libsvn_client.
> Maybe that is why he hasn't made much progress, given that it
> makes the task more difficult -- I gave him bad advice :(
Well, I'm at the point of attempting to use the svn_wc_traversal_info_t
to gather the stats; but, the snag is I haven't quite figured
out where the conflict checks are made in the recursive update
routine (on hand, I can't remember the name). I was looking
at the notify() function and the text-conflict checks are
clearcut and easy to follow. While I know I'm not barking
up the wrong tree, I just don't think I'm looking deep enough.
>
> Cc'ing Edmund to make him aware of his new companion Daniel.
>
I saw him on IRC but I've been busy at work so I couldn't
do much in terms of communicating ideas.
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2377190
Re: issue 3342 - summary of conflicts and skips
Posted by Julian Foad <ju...@btopenworld.com>.
Stefan Sperling wrote:
> On Thu, Jul 30, 2009 at 07:15:04PM +0100, Julian Foad wrote:
> > Stefan Sperling wrote:
> > > OK. Note that Edmund Wong has been working on this issue, too,
> > > and I told him to do the stat gathering in libsvn_client.
> > > Maybe that is why he hasn't made much progress, given that it
> > > makes the task more difficult -- I gave him bad advice :(
> >
> > Oh... I didn't remember that.
> >
> > The information coming up from libsvn_client is "here are notifications
> > of all the things happening". I think it would be wrong to make
> > libsvn_client say "and also here is some summary information about those
> > notifications, classified in a particular way".
>
> The way I saw it was that notification callbacks exist to print
> notifications. They don't exist to keep track of what happened
> during the entire operation.
Hi Stefan and Edmund. Sorry I didn't join in when you were discussing
this the first time.
The more I think about it, the less I think the libsvn_wc or even
libsvn_client should be involved in the summarizing at all.
I think the notifications exist to notify the client what's happening.
The library's job is to provide information; the client's job is to
decide what to do with that information. The client can choose to print
them if it wants to, and/or it can advance a progress bar, and/or it can
record and categorise them and maybe display a summary later, or
whatever it wants to do with the info. The client is perfectly entitled
to use them to create a record of what's happened during the entire
operation, and I would expect GUI clients to do so. In effect, CLI
clients create such a record too, implicitly, by streaming the list to
stdout.
The notifications are intended to be complete and accurate and are the
primary means of reporting what is happening. We don't expect, for
example, that the library should be able to generate more accurate
statistics internally because certain conflicts are not notified but
should be counted, or because some other conflicts are notified but
should not be counted.
> That's what the batons are for,
> and there is a baton of type svn_wc_traversal_info_t which has
> precisely the scope we need.
It's a technically possible solution.
But summarizing the reported info is a very high-level task. The
low-level libraries already provide the necessary information. It
doesn't seem right to me for a library to provide information and then
provide a particular kind of summary of that information. (In other
situations, it can make sense for a library to provide a summary of
something in advance, if it's then optional for the client to request
the full info and expend significantly more time or resources in doing
so. But that's not what we're talking about.)
> I'm quoting a mail I sent to Edmund about it below.
I know some text in that email might be outdated by now, but there's one
comment I'll make that's still relevant.
> I agree that the approach you suggested is probably simpler, though.
>
> Stefan
>
> ====
>
> So, what we'll do is:
>
> 1) We'll move the conflict counters (see struct notify_baton in
> svn/notify.c) from the svn client's notification baton into the
> svn_wc_notify_t struct.
>
> 2) We introduce a new notification action. Right now, we have
> svn_wc_notify_update_completed, but that is called for each
> update target individually (see svn_client__update_internal
> in libsvn_client/update.c). Rather, we want a notification that
> says "the whole operation has completed and here are the combined
> stats I have collected for you for every target."
The problem there is a case of poor layering. The libsvn_client is
passing (multiple) WC-layer "update completed" notifications straight
through to the client even though the client shouldn't know anything
about how it's driving the WC layer multiple times to accomplish a
single task.
That's a separate issure that we should fix. Libsvn_client could trap
those and send its own (single) "update completed" notification instead.
(We could either keep using the WC-layer notifier data type or create a
client-layer notifier type.)
- Julian
> So we could add a new action like svn_wc_notify_conflict_stats.
> This notification should be issued by svn_client_update3(), after
> processing all the targets (see the "for (i = 0; i < paths->nelts;
> ++i)" loop in svn_client_update3()). When the notification is made,
> the stats the library collected during the operation are added
> to the new counters we added to svn_wc_notify_t in step 1), and
> passed to the client. The client can then read the stats.
>
> I'll leave it to you to figure out where to issue the notification
> for merge. But note that it would suffice for now to just focus on
> update. Once that is solved, solving the problem again for merge
> should be fairly easy.
>
> 3) During update, and merge, whenever the client library notifies
> the client about a conflict, we increment the conflict stats.
>
> The stats need to be stored somewhere with sufficient lifetime
> so that they survive the whole operation. Local variables in
> svn_client_update3() (and the equivalent for merge) might be
> a good choice for storage. However, local variables aren't that
> useful for gathering the stats, because the stats will be gathered
> way down in the call chain (in libsvn_wc/update-editor.c, for update).
>
> There is this interesting thing called svn_wc_traversal_info_t,
> which is used during an update to gather and summarise information
> about externals. It could easily be extended to gather conflict
> stats, too. We'll just need to add conflict counters to it.
>
> The traversal info is allocated in svn_client__update_internal(),
> and passed down the call chain, until it eventually ends up
> in the edit_baton of libsvn_wc/update-editor.c. This is the
> perfect spot to collect conflict stats, because we also issue
> notifications from there.
>
> Because svn_client__update_internal() operates on a single target,
> we can use a traversal info to gather stats for one target only.
> But that's fine, and no worse than the current situation.
> In addition, we could pass pointers to the local variables we
> have in svn_client_update3() down to svn_client__update_internal(),
> which could then add the per-target stats to the overall stats
> maintained in the local variables of svn_client_update3().
>
> So this is the high-level description of the process for update.
>
> For merge, things might end up being a bit different, because most
> of the merge logic is in libsvn_client/merge.c, which also handles
> notifications. We won't need a traversal info, for example.
> Instead, we can tweak any non-public functions inside libsvn_client/merge.c
> to our liking so that we can gather conflict stats.
> But again, it's enough if you focus on update, first.
>
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2377314
Re: issue 3342 - summary of conflicts and skips
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Jul 30, 2009 at 07:15:04PM +0100, Julian Foad wrote:
> Stefan Sperling wrote:
> > OK. Note that Edmund Wong has been working on this issue, too,
> > and I told him to do the stat gathering in libsvn_client.
> > Maybe that is why he hasn't made much progress, given that it
> > makes the task more difficult -- I gave him bad advice :(
>
> Oh... I didn't remember that.
>
> The information coming up from libsvn_client is "here are notifications
> of all the things happening". I think it would be wrong to make
> libsvn_client say "and also here is some summary information about those
> notifications, classified in a particular way".
The way I saw it was that notification callbacks exist to print
notifications. They don't exist to keep track of what happened
during the entire operation. That's what the batons are for,
and there is a baton of type svn_wc_traversal_info_t which has
precisely the scope we need.
I'm quoting a mail I sent to Edmund about it below.
I agree that the approach you suggested is probably simpler, though.
Stefan
====
So, what we'll do is:
1) We'll move the conflict counters (see struct notify_baton in
svn/notify.c) from the svn client's notification baton into the
svn_wc_notify_t struct.
2) We introduce a new notification action. Right now, we have
svn_wc_notify_update_completed, but that is called for each
update target individually (see svn_client__update_internal
in libsvn_client/update.c). Rather, we want a notification that
says "the whole operation has completed and here are the combined
stats I have collected for you for every target."
So we could add a new action like svn_wc_notify_conflict_stats.
This notification should be issued by svn_client_update3(), after
processing all the targets (see the "for (i = 0; i < paths->nelts;
++i)" loop in svn_client_update3()). When the notification is made,
the stats the library collected during the operation are added
to the new counters we added to svn_wc_notify_t in step 1), and
passed to the client. The client can then read the stats.
I'll leave it to you to figure out where to issue the notification
for merge. But note that it would suffice for now to just focus on
update. Once that is solved, solving the problem again for merge
should be fairly easy.
3) During update, and merge, whenever the client library notifies
the client about a conflict, we increment the conflict stats.
The stats need to be stored somewhere with sufficient lifetime
so that they survive the whole operation. Local variables in
svn_client_update3() (and the equivalent for merge) might be
a good choice for storage. However, local variables aren't that
useful for gathering the stats, because the stats will be gathered
way down in the call chain (in libsvn_wc/update-editor.c, for update).
There is this interesting thing called svn_wc_traversal_info_t,
which is used during an update to gather and summarise information
about externals. It could easily be extended to gather conflict
stats, too. We'll just need to add conflict counters to it.
The traversal info is allocated in svn_client__update_internal(),
and passed down the call chain, until it eventually ends up
in the edit_baton of libsvn_wc/update-editor.c. This is the
perfect spot to collect conflict stats, because we also issue
notifications from there.
Because svn_client__update_internal() operates on a single target,
we can use a traversal info to gather stats for one target only.
But that's fine, and no worse than the current situation.
In addition, we could pass pointers to the local variables we
have in svn_client_update3() down to svn_client__update_internal(),
which could then add the per-target stats to the overall stats
maintained in the local variables of svn_client_update3().
So this is the high-level description of the process for update.
For merge, things might end up being a bit different, because most
of the merge logic is in libsvn_client/merge.c, which also handles
notifications. We won't need a traversal info, for example.
Instead, we can tweak any non-public functions inside libsvn_client/merge.c
to our liking so that we can gather conflict stats.
But again, it's enough if you focus on update, first.
Re: issue 3342 - summary of conflicts and skips
Posted by Julian Foad <ju...@btopenworld.com>.
Stefan Sperling wrote:
> OK. Note that Edmund Wong has been working on this issue, too,
> and I told him to do the stat gathering in libsvn_client.
> Maybe that is why he hasn't made much progress, given that it
> makes the task more difficult -- I gave him bad advice :(
Oh... I didn't remember that.
The information coming up from libsvn_client is "here are notifications
of all the things happening". I think it would be wrong to make
libsvn_client say "and also here is some summary information about those
notifications, classified in a particular way".
If we want to make the summary stats available from libsvn_client, one
way we could do it would be to make libsvn_client provide a notification
wrapper function that just does stat-counting and then forwards all
notifications on to the "real" notification function:
svn_error_t *
svn_client_get_stats_reporting_notifier(
svn_wc_notify_func_t *wrapper_notify_func,
void **wrapper_notify_baton,
svn_wc_notify_func_t real_notify_func,
void *real_notify_baton,
...);
and provide some way to read the stats out of it on request.
That way would not interfere with the current libsvn_client API and
would be replaceable if the particular client wanted stats classified
differently.
But I just don't think we need to make this available at libsvn_client
level unless we see other clients that would actually use it.
- Julian
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2377088
Re: issue 3342 - summary of conflicts and skips [was: ... 3432 ...]
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Jul 30, 2009 at 06:40:00PM +0100, Julian Foad wrote:
> On Thu, 2009-07-30 at 18:05 +0100, Stefan Sperling wrote:
> > On Thu, Jul 30, 2009 at 05:59:23PM +0100, Julian Foad wrote:
> > > Hi Daniel.
> > >
> > > Thanks for your interest.
> > >
> > > Issue #3342 <http://subversion.tigris.org/issues/show_bug.cgi?id=3342>
> > > is not talking about the "G foo1" notifications. It is about the message
> > > that says
> > >
> > > [[[
> > > Summary of conflicts:
> > > Text conflicts: 2
> > > Property conflicts: 1
> > > Skipped paths: 10
> > > ]]]
> > >
> > > Sorry - that's my fault for not being clear when I wrote it. I've just
> > > added a note to the issue to clarify this.
> > >
> > > To fix this, I would change the notifier function in
> > > subversion/svn/notify.c so that it still collects the statistics in
> > > 'nb->text_conflicts' etc., but does NOT call print_conflict_stats() when
> > > it gets an 'update_completed' or 'merge_completed' notification.
> > > Instead, make print_conflict_stats() a public function (named
> > > svn_cl__print_conflict_stats()) so that the top-level update function
> > > (svn_cl__update()) can call it to print the stats when the whole update
> > > is finished.
> > > Then adjust the callers (update, switch and merge) to call
> > > svn_cl__print_conflict_stats() after finishing the update/switch/merge
> > > operation.
> >
> > Why not gather stats entirely inside of libsvn_client so that
> > other clients can also benefit?
>
> That might be useful... but I'm not entirely convinced it would be best.
> The summary stats seem to be quite closely tied to the way the client
> wants to classify the notifications. For example, it records conflicts
> in externals separately from conflicts in non-externals. A different
> client may well not be interested in that particular distinction, but
> may instead want to classify them in some other way such as skipped
> directories vs. skipped files.
>
> I would guess that most clients are GUIs and a GUI would not need
> libsvn_client to provide the stats because it would, I assume, remember
> the actual notification results in a list, and filter or classify them
> however it wants, and then it would just count the elements in its
> list(s). So it seems to me that this kind of summary is rather specific
> to command-line clients.
>
> To move the counting into libsvn_client would be a much bigger change,
> including API change, and Daniel said he's a newbie, so I think just
> changing the 'svn' client would be a good task.
OK. Note that Edmund Wong has been working on this issue, too,
and I told him to do the stat gathering in libsvn_client.
Maybe that is why he hasn't made much progress, given that it
makes the task more difficult -- I gave him bad advice :(
Cc'ing Edmund to make him aware of his new companion Daniel.
Stefan
Re: issue 3342 - summary of conflicts and skips [was: ... 3432 ...]
Posted by Julian Foad <ju...@btopenworld.com>.
On Thu, 2009-07-30 at 18:05 +0100, Stefan Sperling wrote:
> On Thu, Jul 30, 2009 at 05:59:23PM +0100, Julian Foad wrote:
> > Hi Daniel.
> >
> > Thanks for your interest.
> >
> > Issue #3342 <http://subversion.tigris.org/issues/show_bug.cgi?id=3342>
> > is not talking about the "G foo1" notifications. It is about the message
> > that says
> >
> > [[[
> > Summary of conflicts:
> > Text conflicts: 2
> > Property conflicts: 1
> > Skipped paths: 10
> > ]]]
> >
> > Sorry - that's my fault for not being clear when I wrote it. I've just
> > added a note to the issue to clarify this.
> >
> > To fix this, I would change the notifier function in
> > subversion/svn/notify.c so that it still collects the statistics in
> > 'nb->text_conflicts' etc., but does NOT call print_conflict_stats() when
> > it gets an 'update_completed' or 'merge_completed' notification.
> > Instead, make print_conflict_stats() a public function (named
> > svn_cl__print_conflict_stats()) so that the top-level update function
> > (svn_cl__update()) can call it to print the stats when the whole update
> > is finished.
> > Then adjust the callers (update, switch and merge) to call
> > svn_cl__print_conflict_stats() after finishing the update/switch/merge
> > operation.
>
> Why not gather stats entirely inside of libsvn_client so that
> other clients can also benefit?
That might be useful... but I'm not entirely convinced it would be best.
The summary stats seem to be quite closely tied to the way the client
wants to classify the notifications. For example, it records conflicts
in externals separately from conflicts in non-externals. A different
client may well not be interested in that particular distinction, but
may instead want to classify them in some other way such as skipped
directories vs. skipped files.
I would guess that most clients are GUIs and a GUI would not need
libsvn_client to provide the stats because it would, I assume, remember
the actual notification results in a list, and filter or classify them
however it wants, and then it would just count the elements in its
list(s). So it seems to me that this kind of summary is rather specific
to command-line clients.
To move the counting into libsvn_client would be a much bigger change,
including API change, and Daniel said he's a newbie, so I think just
changing the 'svn' client would be a good task.
- Julian
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2377067
Re: issue 3342 - summary of conflicts and skips [was: ... 3432 ...]
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Jul 30, 2009 at 05:59:23PM +0100, Julian Foad wrote:
> Hi Daniel.
>
> Thanks for your interest.
>
> Issue #3342 <http://subversion.tigris.org/issues/show_bug.cgi?id=3342>
> is not talking about the "G foo1" notifications. It is about the message
> that says
>
> [[[
> Summary of conflicts:
> Text conflicts: 2
> Property conflicts: 1
> Skipped paths: 10
> ]]]
>
> Sorry - that's my fault for not being clear when I wrote it. I've just
> added a note to the issue to clarify this.
>
> To fix this, I would change the notifier function in
> subversion/svn/notify.c so that it still collects the statistics in
> 'nb->text_conflicts' etc., but does NOT call print_conflict_stats() when
> it gets an 'update_completed' or 'merge_completed' notification.
> Instead, make print_conflict_stats() a public function (named
> svn_cl__print_conflict_stats()) so that the top-level update function
> (svn_cl__update()) can call it to print the stats when the whole update
> is finished.
> Then adjust the callers (update, switch and merge) to call
> svn_cl__print_conflict_stats() after finishing the update/switch/merge
> operation.
Why not gather stats entirely inside of libsvn_client so that
other clients can also benefit?
Stefan
Re: issue 3342 - summary of conflicts and skips [was: ... 3432 ...]
Posted by Edmund Wong <ed...@belfordhk.com>.
Julian Foad wrote:
> Hi Daniel.
>
> Thanks for your interest.
>
> Issue #3342 <http://subversion.tigris.org/issues/show_bug.cgi?id=3342>
> is not talking about the "G foo1" notifications. It is about the message
> that says
>
> [[[
> Summary of conflicts:
> Text conflicts: 2
> Property conflicts: 1
> Skipped paths: 10
> ]]]
>
> Sorry - that's my fault for not being clear when I wrote it. I've just
> added a note to the issue to clarify this.
>
> To fix this, I would change the notifier function in
> subversion/svn/notify.c so that it still collects the statistics in
> 'nb->text_conflicts' etc., but does NOT call print_conflict_stats() when
> it gets an 'update_completed' or 'merge_completed' notification.
> Instead, make print_conflict_stats() a public function (named
> svn_cl__print_conflict_stats()) so that the top-level update function
> (svn_cl__update()) can call it to print the stats when the whole update
> is finished.
>
Hmm. I thought the solution would've been more complicated than
that. It was my (probably faulty) understanding that the collection
of stats should be decoupled with the notification baton and done
entirely (for example) during the update stage in which the stats
are collected during the traversal of the tree and then notify
the client that the stats are ready for usage and stored in the
(currently proposed struct) svn_conflict_stats_t (which basically
contains all the needed fields, i.e. int text-conflicts ).
Edmund
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2377192