On 13 May 2009 at 19:34, Lieven Govaerts wrote:
> Hi Alan,
>
> I have a few questions on your patch (inline).
>
> > @@ -781,7 +781,7 @@
> > sb->data, FALSE, iterpool));
> > else
> > SVN_ERR(receiver(receiver_baton, line_no, SVN_INVALID_REVNUM,
> > - NULL, SVN_INVALID_REVNUM, NULL, NULL,
> > + NULL, merged_rev, merged_rev_props, merged_path,
>
> Why is this chunk needed?
I can't guarantee that it is needed, but my logic goes like this.
if walk->rev can be NULL // the case we are talking about
then it doesn't follow that walk_merged-> rev would also be NULL at the same line.
So there should be cases where there is merged information but no trunk!
Anyway that was the logic, but it may be flawed.
My thought today when having another look at this code:
I have found that walk->rev can be NULL from line 706, but there is no
corresponding case for the merged chain. This is the case with local working copy
mods which I have never tested.
Looks like this part of the change is not needed.
>
> > sb->data, TRUE, iterpool));
> > }
> > if (eof) break;
> > Index: subversion/svn/blame-cmd.c
> > ===================================================================
> > --- subversion/svn/blame-cmd.c (revision 37376)
> > +++ subversion/svn/blame-cmd.c (working copy)
> > @@ -178,8 +178,9 @@
> > revision which put the line in its current state, so we use the
> > earliest revision. If we ever switch to a backward blame algorithm,
> > we may need to adjust this. */
> > - if (merged_revision < revision)
> > - {
> > + if (SVN_IS_VALID_REVNUM(merged_revision) &&
> > + (!SVN_IS_VALID_REVNUM(revision) || merged_revision < revision))
> > + {
>
> Same question here.
The interface specification ( svn_client.h line 677 ) states that revision can be invalid
so it shouldn't be used in the comparison without first being checked to see if it's
valid. The same is also true for merged_revision.
> The test case you provided - committed in r37719 - doesn't need those
> two changes to pass.
It looks like the invalid revs only occur with using blame on a modified working
copy. I haven't looked into this.
> Lieven
>
> ------------------------------------------------------
> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2240565
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2244289
Received on 2009-05-14 00:04:32 CEST