Re: [PATCH] svn_client_diff_summarize() reports 'normal' if properties were changed

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