Hi Alan,
the minimal change to make the current test working is committed in
r37723. Can you test if it fixes all problems for you?
I'll add a new test with some local changes in the file, to simulate
the problems that might necessitate the other changes in your patch.
thanks.
Lieven
On Thu, May 14, 2009 at 12:03 AM, Alan Wood <alan.wood_at_clear.net.nz> wrote:
> 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=2251189
Received on 2009-05-14 08:21:37 CEST