On Fri, 2011-01-07, Prabhu Gnana Sundar wrote:
> On Thu, 2011-01-06 at 14:22 +0000, Julian Foad wrote:
> > The tabular format is good but it would be easier to follow if instead
> > of "A" or "B" or "C" etc. you write "Shown as diff against source" or
> > "Shown as all lines added".
>
> Thank you for your valuable suggestion :) I have added a modified
> tabular summary in the file attached with this mail.
Thanks - that's easier to understand.
It looks like where you write "N/A" the diff operation proceeds OK but
produces no output. In my mind, since these are "copied and not
modified" cases, that's effectively showing a diff against the source.
So the patch doesn't make the "copied and not modified" cases
consistent, it only affects the "copied and modified" cases. Have I got
that right? If that would require completely different code changes,
then maybe that would be best as a separate patch, but I think we should
fix those cases too, don't you?
> > Why are some of your tests testing deletes, not adds?
>
> I have shown tests for 'adds' as well as 'deletes' to show the 'diff'
> behaviour.
>
> And with context to the above shown 'diff', clearly, the 'copiedfile1'
> was just a 'svn copy' of the file 'file1' from revision1, meaning that
> it has no diff content. That's why I opted to show the deletion in this
> case and a few similar cases. :)
OK, but it's possible that "delete" and "add" may not be shown the same
way, so let's be consistent and either test an "add" in every case or
test both ways in every case.
> > I seem to recall that the result sometimes depends on whether the target
> > of the diff command is the actual file or a directory that contains the
> > file. In other words, "svn diff ./" may behave differently from "svn
> > diff copiedfile1". Can you test that too please?
> >
>
> Sure Julian, I'll test it. :)
>
> I tested it and I don't see any 'behavioural' change in the diffs.
> Here is what I got with this patch... :)
>
> $ svn diff -r1:3 copiedfile2
Did you try all the different cases? If you put your test commands in a
script that automatically loops through all the cases, and show us that
script, that will help.
I should point out that I don't have enough time to review your code
(and architectural issues) or to guide this patch all the way to
completion, I only have enough time to make a few comments and ask a few
questions that I think are important, so you'll need to get help from
someone else as well. Especially on that library layering issue. The
converse of this is that you don't have to do what I say :-)
- Julian
Received on 2011-01-07 14:22:37 CET