You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stefan Küng <to...@gmail.com> on 2006/04/24 17:16:58 UTC
[PATCH] svn_client_diff_summarize() reports 'normal' if properties
were changed
Hi,
The new API in 1.4 svn_client_diff_summarize() returns always
svn_client_diff_summarize_kind_normal as the diff->summarize_kind member
if there were property changes (if diff->prop_changed is true). And this
even if there actually were changes in the text part of a file.
The attached patch fixes this issue.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.tigris.org
Re: [PATCH] svn_client_diff_summarize() reports 'normal' if properties were changed
Posted by Martin Hauner <Ma...@gmx.net>.
Hi,
Stefan Küng wrote:
> Martin Hauner wrote:
> > Stefan Küng wrote:
[..]
> > > Index: subversion/libsvn_client/repos_diff_summarize.c
> > > ===================================================================
> > > --- subversion/libsvn_client/repos_diff_summarize.c (revision
> 19446)
> > > +++ subversion/libsvn_client/repos_diff_summarize.c (working copy)
> > > @@ -89,8 +89,12 @@
> > > {
> > > svn_client_diff_summarize_t *sum;
> > > if (ib->summarize)
> > > + {
> > > + if (ib->summarize == svn_client_diff_summarize_kind_normal)
> > > + ib->summarize = sum_kind;
> >
> > This looks strange here, shouldn't it be something like
> >
> > if (sum->summarize_kind != svn_client_diff_summarize_kind_normal)
> > sum->summarize_kind = sum_kind;
> >
> > and after the alloc?
>
> Not really. You now overwrite the summarize_kind if it's *not* normal.
> [..]
ah, of course, not or not not :)
> It's not necessary to do that after the alloc. Because we overwrite the
> kind if it already is set..
> [..]
Yes, i see. It is a lot easier to read after applying the patch :)
> svn_client_diff_summarize_t *sum;
> if (ib->summarize)
> + {
> + if (ib->summarize == svn_client_diff_summarize_kind_normal)
> + ib->summarize = sum_kind;
Still, ib->summarize is an svn_client_diff_summarize_t pointer and
not the kind value in the svn_client_diff_summarize_t struct you
want to check and set.
Using the sum pointer in my code is wrong too because it is not
initialized at that point, so i think the if should be
if (ib->summarize->summarize_kind == svn_client_diff_summarize_kind_normal)
ib->summarize->summarize_kind = sum_kind;
--
Martin
Subcommander, http://subcommander.tigris.org
a cross platform Win32/Unix/MacOSX subversion gui
client & diff/merge tool.
"Feel free" - 10 GB Mailbox, 100 FreeSMS/Monat ...
Jetzt GMX TopMail testen: http://www.gmx.net/de/go/topmail
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn_client_diff_summarize() reports 'normal' if properties
were changed
Posted by Stefan Küng <to...@gmail.com>.
Martin Hauner wrote:
> Stefan Küng wrote:
>>
>> The new API in 1.4 svn_client_diff_summarize() returns always
>> svn_client_diff_summarize_kind_normal as the diff->summarize_kind
>> member if there were property changes (if diff->prop_changed is true).
>> And this even if there actually were changes in the text part of a file.
>> The attached patch fixes this issue.
>
> The ensure_summarize feels a bit strange anyway. What about removing
> the sum_kind parameter from it and simply setting the kind to normal.
>
> Any caller of ensure_summarize would then overwrite the kind if required:
That would of course be an option too.
Opinions?
> ib->summarize->summarize_kind = svn_client_diff_summarize_kind_modified;
>
> change_prop wouldn't overwrite it. To bad it is such a long line.. ;)
My tests showed that change_prop is called before the check for text
changes.
> > Index: subversion/libsvn_client/repos_diff_summarize.c
> > ===================================================================
> > --- subversion/libsvn_client/repos_diff_summarize.c (revision 19446)
> > +++ subversion/libsvn_client/repos_diff_summarize.c (working copy)
> > @@ -89,8 +89,12 @@
> > {
> > svn_client_diff_summarize_t *sum;
> > if (ib->summarize)
> > + {
> > + if (ib->summarize == svn_client_diff_summarize_kind_normal)
> > + ib->summarize = sum_kind;
>
> This looks strange here, shouldn't it be something like
>
> if (sum->summarize_kind != svn_client_diff_summarize_kind_normal)
> sum->summarize_kind = sum_kind;
>
> and after the alloc?
Not really. You now overwrite the summarize_kind if it's *not* normal.
So basically, we're right were we are now: the normal kind stays, even
if there are text changes detected.
It's not necessary to do that after the alloc. Because we overwrite the
kind if it already is set, which indicates that it has to be already
allocated. If it's not allocated yet, then we don't need to check if we
can overwrite it but simply *set* it - which the code already does.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn_client_diff_summarize() reports 'normal' if properties
were changed
Posted by Martin Hauner <ma...@gmx.net>.
Stefan Küng wrote:
> Hi,
>
> The new API in 1.4 svn_client_diff_summarize() returns always
> svn_client_diff_summarize_kind_normal as the diff->summarize_kind member
> if there were property changes (if diff->prop_changed is true). And this
> even if there actually were changes in the text part of a file.
> The attached patch fixes this issue.
The ensure_summarize feels a bit strange anyway. What about removing
the sum_kind parameter from it and simply setting the kind to normal.
Any caller of ensure_summarize would then overwrite the kind if required:
ib->summarize->summarize_kind = svn_client_diff_summarize_kind_modified;
change_prop wouldn't overwrite it. To bad it is such a long line.. ;)
> Index: subversion/libsvn_client/repos_diff_summarize.c
> ===================================================================
> --- subversion/libsvn_client/repos_diff_summarize.c (revision 19446)
> +++ subversion/libsvn_client/repos_diff_summarize.c (working copy)
> @@ -89,8 +89,12 @@
> {
> svn_client_diff_summarize_t *sum;
> if (ib->summarize)
> + {
> + if (ib->summarize == svn_client_diff_summarize_kind_normal)
> + ib->summarize = sum_kind;
This looks strange here, shouldn't it be something like
if (sum->summarize_kind != svn_client_diff_summarize_kind_normal)
sum->summarize_kind = sum_kind;
and after the alloc?
> sum = apr_pcalloc(ib->item_pool, sizeof(*sum));
>
> sum->node_kind = ib->node_kind;
--
Martin
Subcommander, http://subcommander.tigris.org
a cross platform Win32/Unix/MacOSX subversion GUI client & diff/merge tool.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn_client_diff_summarize() reports 'normal' if properties[Show]
were changed
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
Stefan Küng writes:
> Peter N. Lundblad wrote:
> > Stefan Küng writes:
> > >
> > > The new API in 1.4 svn_client_diff_summarize() returns always
> > > svn_client_diff_summarize_kind_normal as the diff->summarize_kind member
> > > if there were property changes (if diff->prop_changed is true). And this
> > > even if there actually were changes in the text part of a file.
> >
> > Stefan,
> >
> > Thanks for the report. I'll get to this before 1.4. I'll just find the
> > time to write a test for this. (I noticed we don't have any tests for
> > diff --summarize, so, what an oportunity:-)
>
> Thanks for doing that. I guess that means I don't have to send another
> patch then? :)
Exactly. I'll take care of it. Of course, if you had time to write a
test case:-)
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn_client_diff_summarize() reports 'normal' if properties[Show][Show][Show][Show][Show][Show]
were changed
Posted by Stefan Küng <to...@gmail.com>.
Peter N. Lundblad wrote:
> Stefan Küng writes:
> > Thanks for doing that. I guess that means I don't have to send another
> > patch then? :)
> >
>
> This should be fixed in r19479 (with a test in r19483).
Sorry for the late answer.
Tested your patch. Works like a charm :)
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn_client_diff_summarize() reports 'normal' if properties[Show][Show][Show][Show][Show][Show]
were changed
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
Stefan Küng writes:
> Thanks for doing that. I guess that means I don't have to send another
> patch then? :)
>
This should be fixed in r19479 (with a test in r19483).
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn_client_diff_summarize() reports 'normal' if properties
were changed
Posted by Stefan Küng <to...@gmail.com>.
Peter N. Lundblad wrote:
> Stefan Küng writes:
> >
> > The new API in 1.4 svn_client_diff_summarize() returns always
> > svn_client_diff_summarize_kind_normal as the diff->summarize_kind member
> > if there were property changes (if diff->prop_changed is true). And this
> > even if there actually were changes in the text part of a file.
>
> Stefan,
>
> Thanks for the report. I'll get to this before 1.4. I'll just find the
> time to write a test for this. (I noticed we don't have any tests for
> diff --summarize, so, what an oportunity:-)
Thanks for doing that. I guess that means I don't have to send another
patch then? :)
> Also, thank you for trying out the APIs this early!
I thought it best to try out the new features as early as possible to
catch as many bugs as possible before the release. I don't like to wait
6 months for a bugfix release like with 1.3.1.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn_client_diff_summarize() reports 'normal' if properties
were changed
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
Stefan Küng writes:
>
> The new API in 1.4 svn_client_diff_summarize() returns always
> svn_client_diff_summarize_kind_normal as the diff->summarize_kind member
> if there were property changes (if diff->prop_changed is true). And this
> even if there actually were changes in the text part of a file.
Stefan,
Thanks for the report. I'll get to this before 1.4. I'll just find the
time to write a test for this. (I noticed we don't have any tests for
diff --summarize, so, what an oportunity:-)
Also, thank you for trying out the APIs this early!
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org