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/08/08 16:33:05 UTC
[PATCH] issue 3342 - the right patch!
[ I am having trouble replying to "my" thread. Mutt again? Perhaps it's
time to surrender? ]
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 Näslund
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2381637
Re: [PATCH] issue 3342 - the right patch!
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Julian Foad wrote on Wed, 12 Aug 2009 at 17:35 +0100:
> On Wed, 2009-08-12, Julian Foad wrote:
> > On Wed, 2009-08-12 at 16:07 +0300, Daniel Shahaf wrote:
> > > Julian Foad wrote on Wed, 12 Aug 2009 at 13:47 +0100:
> > > > serious. The fact that it prints "At revision ..." is not so serious: we
> > > > could accept that (in addition to a "Skipped" message) because it is
> > > > analogous to the case of updating a versioned child of a versioned
> > > > directory.
> > > >
> > >
> > > I think in this case the file *really* isn't skipped --- for example, if
> > > the wc is at r4 and 'foo' was created at r5, then 'svn up foo' works.
> >
> > > (I'm not saying this is the way it *should* work. But this is how it
> > > *does* work in 1.6.)
> >
> > If 'foo' was created at r5, then 'svn up foo' would print
> > [[[
> > A foo
> > Updated to revision 5.
> > ]]]
> >
> > not just
> > [[[
> > At revision 5.
> > ]]]
> >
Good point, I didn't notice this difference when I wrote the email.
> > That's OK, and that's not what I'm talking about. I'm talking about the
> > case where 'foo' doesn't exist either locally or in the repository at
> > any revision.
>
Oh, okay. And I agree that in that case (when the file exists neither in
BASE nor in HEAD) it makes sense to make some noise about that (i.e.,
agreed that the notification is a good change).
> (I may have misunderstood what you meant here, too.)
>
> - Julian
>
>
>
Thanks,
Daniel
(one of them)
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2383065
Re: [PATCH] issue 3342 - the right patch!
Posted by Julian Foad <ju...@btopenworld.com>.
On Wed, 2009-08-12, Julian Foad wrote:
> On Wed, 2009-08-12 at 16:07 +0300, Daniel Shahaf wrote:
> > Julian Foad wrote on Wed, 12 Aug 2009 at 13:47 +0100:
> > > Hi Daniel. This all looks great but I think it makes an unintended
> > > behaviour change. If 'foo' is non-existent, then:
> > >
> > > Running svn v1.6:
> > , not in a WC:
> > > [[[
> > > $ svn up foo
> > > Skipped 'foo'
> > > ]]]
> > >
> > > Running your version, not in a WC:
> > > [[[
> > > $ svn up foo
> > > Skipped 'foo'
> > > Summary of conflicts:
> > > Skipped paths: 1
> > > ]]]
> > >
> > > That's good.
> > >
> >
> > ...
> >
> > > Running your version, in a WC:
> > > [[[
> > > $ svn up foo
> > > At revision 38693.
> > > ]]]
> > >
> > > That's the unintended change. The failure to print "Skipped 'foo'" is
> >
> > It's not a change --- svn16 has the same output when inside a WC.
>
> Not on my system, using svn built from the 1.6.x branch on 2009-07-15.
> What system and version are you testing on?
I don't know why I compared it against 1.6.x. I should have compared it
against current trunk. The current trunk has the behaviour you say, the
same as after your change.
Apologies for that misunderstanding.
Your patch is good, so I have committed it in 38696. Thanks for the
work!
> > > serious. The fact that it prints "At revision ..." is not so serious: we
> > > could accept that (in addition to a "Skipped" message) because it is
> > > analogous to the case of updating a versioned child of a versioned
> > > directory.
> > >
> >
> > I think in this case the file *really* isn't skipped --- for example, if
> > the wc is at r4 and 'foo' was created at r5, then 'svn up foo' works.
>
> > (I'm not saying this is the way it *should* work. But this is how it
> > *does* work in 1.6.)
>
> If 'foo' was created at r5, then 'svn up foo' would print
> [[[
> A foo
> Updated to revision 5.
> ]]]
>
> not just
> [[[
> At revision 5.
> ]]]
>
> That's OK, and that's not what I'm talking about. I'm talking about the
> case where 'foo' doesn't exist either locally or in the repository at
> any revision.
(I may have misunderstood what you meant here, too.)
- Julian
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2382943
Re: [PATCH] issue 3342 - the right patch!
Posted by Julian Foad <ju...@btopenworld.com>.
On Wed, 2009-08-12 at 16:07 +0300, Daniel Shahaf wrote:
> Julian Foad wrote on Wed, 12 Aug 2009 at 13:47 +0100:
> > Hi Daniel. This all looks great but I think it makes an unintended
> > behaviour change. If 'foo' is non-existent, then:
> >
> > Running svn v1.6:
> , not in a WC:
> > [[[
> > $ svn up foo
> > Skipped 'foo'
> > ]]]
> >
> > Running your version, not in a WC:
> > [[[
> > $ svn up foo
> > Skipped 'foo'
> > Summary of conflicts:
> > Skipped paths: 1
> > ]]]
> >
> > That's good.
> >
>
> ...
>
> > Running your version, in a WC:
> > [[[
> > $ svn up foo
> > At revision 38693.
> > ]]]
> >
> > That's the unintended change. The failure to print "Skipped 'foo'" is
>
> It's not a change --- svn16 has the same output when inside a WC.
Not on my system, using svn built from the 1.6.x branch on 2009-07-15.
What system and version are you testing on?
> > serious. The fact that it prints "At revision ..." is not so serious: we
> > could accept that (in addition to a "Skipped" message) because it is
> > analogous to the case of updating a versioned child of a versioned
> > directory.
> >
>
> I think in this case the file *really* isn't skipped --- for example, if
> the wc is at r4 and 'foo' was created at r5, then 'svn up foo' works.
> (I'm not saying this is the way it *should* work. But this is how it
> *does* work in 1.6.)
If 'foo' was created at r5, then 'svn up foo' would print
[[[
A foo
Updated to revision 5.
]]]
not just
[[[
At revision 5.
]]]
That's OK, and that's not what I'm talking about. I'm talking about the
case where 'foo' doesn't exist either locally or in the repository at
any revision.
- Julian
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2382897
Re: [PATCH] issue 3342 - the right patch!
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Julian Foad wrote on Wed, 12 Aug 2009 at 13:47 +0100:
> Hi Daniel. This all looks great but I think it makes an unintended
> behaviour change. If 'foo' is non-existent, then:
>
> Running svn v1.6:
, not in a WC:
> [[[
> $ svn up foo
> Skipped 'foo'
> ]]]
>
> Running your version, not in a WC:
> [[[
> $ svn up foo
> Skipped 'foo'
> Summary of conflicts:
> Skipped paths: 1
> ]]]
>
> That's good.
>
...
> Running your version, in a WC:
> [[[
> $ svn up foo
> At revision 38693.
> ]]]
>
> That's the unintended change. The failure to print "Skipped 'foo'" is
It's not a change --- svn16 has the same output when inside a WC.
> serious. The fact that it prints "At revision ..." is not so serious: we
> could accept that (in addition to a "Skipped" message) because it is
> analogous to the case of updating a versioned child of a versioned
> directory.
>
I think in this case the file *really* isn't skipped --- for example, if
the wc is at r4 and 'foo' was created at r5, then 'svn up foo' works.
(I'm not saying this is the way it *should* work. But this is how it
*does* work in 1.6.)
> - Julian
>
> ------------------------------------------------------
> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2382884
>
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2382888
Re: [PATCH] issue 3342 - the right patch!
Posted by Julian Foad <ju...@btopenworld.com>.
On Sat, 2009-08-08, Daniel Näslund wrote:
> [[[
> 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.
Also mention the change you made to notify().
> * 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.
> ]]]
[...]
> ------------------------------------------------------
> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2381637
Hi Daniel. This all looks great but I think it makes an unintended
behaviour change. If 'foo' is non-existent, then:
Running svn v1.6:
[[[
$ svn up foo
Skipped 'foo'
]]]
Running your version, not in a WC:
[[[
$ svn up foo
Skipped 'foo'
Summary of conflicts:
Skipped paths: 1
]]]
That's good.
Running your version, in a WC:
[[[
$ svn up foo
At revision 38693.
]]]
That's the unintended change. The failure to print "Skipped 'foo'" is
serious. The fact that it prints "At revision ..." is not so serious: we
could accept that (in addition to a "Skipped" message) because it is
analogous to the case of updating a versioned child of a versioned
directory.
- Julian
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2382884