You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Kamesh Jayachandran <ka...@collab.net> on 2010/12/01 15:11:45 UTC
Re: [PATCH] enhance diff to make it behave uniformly
Hi Prabhu,
I understand your patch fixes the following two cases.
1. svn di -cN explicitly_reinstated_file_with_mod_at_rN
Without your patch we get a failure
"explicitly_reinstated_file_with_mod_at_rN does not exist at rN-1"
With your patch we get *modification* alone.
2. svn di -cN Parent_Dir_of_explicitly_reinstated_file_at_rN
Without your patch we get all lines as added.
With your patch we get only what got injected along with copy in rN.
So if somebody wants all lines as added in above cases they can use
--show-copies-as-adds.
Please fix your patch to remove this new param diff-copy-from in all the
layers and assume it to be TRUE unless "--show-copies-as-adds" is given.
Thanks
With regards
Kamesh Jayachandran
On 11/30/2010 03:26 PM, Prabhu Gnana Sundar wrote:
> Hi,
>
> In my previous patch [1] I suggested the addition of the
> '--diff-copy-from' switch to 'svn diff' to solve the cases where 'svn
> diff' would do the diff against the copyfrom source file when a file is
> reinstated or diff target is *not* explicit file.
>
> But since there is already a point that diff behaves differently in
> different scenarios, making 'svn diff' work the same in all scenarios is
> one good option. This patch would solve the problem of making 'svn diff'
> work the same in all conditions by passing 'TRUE' for the
> 'diff_copy_from' option by default. Hence I removed the
> '--diff-copy-from' switch in this patch. This is a more generic way to
> bridge the gap.
>
> I am attaching the patch and the log with this mail. Please review and
> comment on the same.
>
> I still retain this param on svn_client_diff5 and svn_ra_diff4 in case
> somebody wants old behaviour, otherwise I can remove that too.
>
> Regards
> Prabhu
>
> [1]http://mail-archives.apache.org/mod_mbox/subversion-dev/201011.mbox/%
> 3C1291018400.4021.3.camel@prabhugnanasundar%3E
RE: [PATCH] enhance diff to make it behave uniformly
Posted by Julian Foad <ju...@wandisco.com>.
Kamesh Jayachandran wrote:
> >I've been on this list for several years, and that's the first time
> >I hear "he will" and "he would".
>
> I am one feet away from him geographically and reviewed his work
> closely and hence got resonated to answer moment I saw Julian emails.
>
> Sorry if it meant anything else.
For me it's no problem at all. Thank you for explaining the situation.
- Julian
RE: [PATCH] enhance diff to make it behave uniformly
Posted by Kamesh Jayachandran <ka...@collab.net>.
>> Sure he would. Right now he is teaching the 'svn_wc_get_diff_editor6'
>> what he has taught for svn_client__get_diff_editor.
>>
>> He will have tests for that too.
>>
>I've been on this list for several years, and that's the first time
>I hear "he will" and "he would".
I am one feet away from him geographically and reviewed his work closely and hence got resonated to answer moment I saw Julian emails.
Sorry if it meant anything else.
With regards
Kamesh Jayachandran
Re: [PATCH] enhance diff to make it behave uniformly
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Kamesh Jayachandran wrote on Wed, Dec 08, 2010 at 21:22:05 +0530:
> On 12/08/2010 09:21 PM, Julian Foad wrote:
>> On Sat, 2010-12-04, Kamesh Jayachandran wrote:
>>>> I understand your patch fixes the following two cases.
>>>>
>>>> 1. svn di -cN explicitly_reinstated_file_with_mod_at_rN
>>>> Hi Kamesh and Prabhu.
>>>> What you're describing here sounds good - it sounds like a simpler and
>>>> more definite change than what I understood before - but I'm not sure
>>>> precisely what "explicitly_reinstated_file_with_mod_at_rN" means.
>>> I mean the following,
>> [...]
>>
>> Thanks, Kamesh - that clarifies it. So that's the case where a file is
>> deleted and then a pre-deletion revision of it is copied back to the
>> same path where it existed before.
>>
>> In my experiments I find the same problem also exists when a file is
>> copied to a new path from a revision older than the value of HEAD at the
>> time of the copy. For example:
>>
>> $ cd wc
>>
>> $ echo "line1"> test.c
>>
>> $ svn add test.c
>> A test.c
>>
>> $ svn ci -m "" test.c
>> Adding test.c
>> Transmitting file data .
>> Committed revision 1.
>>
>> $ svn mkdir ^/foo -m "An unrelated change"
>>
>> Committed revision 2.
>>
>> $ svn cp test.c new.c
>> A new.c
>>
>> $ echo "line2">> new.c
>>
>> $ svn ci -m "" new.c
>> Adding new.c
>> Transmitting file data .
>> Committed revision 3.
>>
>> $ svn diff -c3 new.c
>> svn: Unable to find repository location for 'new.c' in revision 2
>>
That looks similar to the issue #2873 ('svn diff -cN of file added in rN')
that stsp tried to persuade me on IRC into looking into a couple of days ago.
(I still haven't closed the editor instance open on that part of the code)
>> $ svn diff -c3
>> Index: new.c
>> ===================================================================
>> --- new.c (revision 0)
>> +++ new.c (revision 3)
>> @@ -0,0 +1,2 @@
>> +line1
>> +line2
>>
>>
>>>> Please could you include a test for these cases in your patch? Then it
>>>> will be absolutely clear.
>>> Prabhu already has one. Right now he is working on removing the
>>> profileration of UI param diff-copy-from from all the layer in favour
>>> of generic send_copyfrom_args.
>> That's great. It would be good to include the above test scenario as
>> well.
>>
>> Thanks.
>> - Julian
>
> Sure he would. Right now he is teaching the 'svn_wc_get_diff_editor6'
> what he has taught for svn_client__get_diff_editor.
>
> He will have tests for that too.
>
I've been on this list for several years, and that's the first time
I hear "he will" and "he would".
> With regards
> Kamesh Jayachandran
>>
>>
>>
>>
>>
>>
>
Re: [PATCH] enhance diff to make it behave uniformly
Posted by Kamesh Jayachandran <ka...@collab.net>.
On 12/08/2010 09:21 PM, Julian Foad wrote:
> On Sat, 2010-12-04, Kamesh Jayachandran wrote:
>>> I understand your patch fixes the following two cases.
>>>
>>> 1. svn di -cN explicitly_reinstated_file_with_mod_at_rN
>>> Hi Kamesh and Prabhu.
>>> What you're describing here sounds good - it sounds like a simpler and
>>> more definite change than what I understood before - but I'm not sure
>>> precisely what "explicitly_reinstated_file_with_mod_at_rN" means.
>> I mean the following,
> [...]
>
> Thanks, Kamesh - that clarifies it. So that's the case where a file is
> deleted and then a pre-deletion revision of it is copied back to the
> same path where it existed before.
>
> In my experiments I find the same problem also exists when a file is
> copied to a new path from a revision older than the value of HEAD at the
> time of the copy. For example:
>
> $ cd wc
>
> $ echo "line1"> test.c
>
> $ svn add test.c
> A test.c
>
> $ svn ci -m "" test.c
> Adding test.c
> Transmitting file data .
> Committed revision 1.
>
> $ svn mkdir ^/foo -m "An unrelated change"
>
> Committed revision 2.
>
> $ svn cp test.c new.c
> A new.c
>
> $ echo "line2">> new.c
>
> $ svn ci -m "" new.c
> Adding new.c
> Transmitting file data .
> Committed revision 3.
>
> $ svn diff -c3 new.c
> svn: Unable to find repository location for 'new.c' in revision 2
>
> $ svn diff -c3
> Index: new.c
> ===================================================================
> --- new.c (revision 0)
> +++ new.c (revision 3)
> @@ -0,0 +1,2 @@
> +line1
> +line2
>
>
>>> Please could you include a test for these cases in your patch? Then it
>>> will be absolutely clear.
>> Prabhu already has one. Right now he is working on removing the
>> profileration of UI param diff-copy-from from all the layer in favour
>> of generic send_copyfrom_args.
> That's great. It would be good to include the above test scenario as
> well.
>
> Thanks.
> - Julian
Sure he would. Right now he is teaching the 'svn_wc_get_diff_editor6'
what he has taught for svn_client__get_diff_editor.
He will have tests for that too.
With regards
Kamesh Jayachandran
>
>
>
>
>
>
RE: [PATCH] enhance diff to make it behave uniformly
Posted by Julian Foad <ju...@wandisco.com>.
On Sat, 2010-12-04, Kamesh Jayachandran wrote:
> > I understand your patch fixes the following two cases.
> >
> > 1. svn di -cN explicitly_reinstated_file_with_mod_at_rN
>
> >Hi Kamesh and Prabhu.
>
> >What you're describing here sounds good - it sounds like a simpler and
> >more definite change than what I understood before - but I'm not sure
> >precisely what "explicitly_reinstated_file_with_mod_at_rN" means.
>
> I mean the following,
[...]
Thanks, Kamesh - that clarifies it. So that's the case where a file is
deleted and then a pre-deletion revision of it is copied back to the
same path where it existed before.
In my experiments I find the same problem also exists when a file is
copied to a new path from a revision older than the value of HEAD at the
time of the copy. For example:
$ cd wc
$ echo "line1" > test.c
$ svn add test.c
A test.c
$ svn ci -m "" test.c
Adding test.c
Transmitting file data .
Committed revision 1.
$ svn mkdir ^/foo -m "An unrelated change"
Committed revision 2.
$ svn cp test.c new.c
A new.c
$ echo "line2" >> new.c
$ svn ci -m "" new.c
Adding new.c
Transmitting file data .
Committed revision 3.
$ svn diff -c3 new.c
svn: Unable to find repository location for 'new.c' in revision 2
$ svn diff -c3
Index: new.c
===================================================================
--- new.c (revision 0)
+++ new.c (revision 3)
@@ -0,0 +1,2 @@
+line1
+line2
> >Please could you include a test for these cases in your patch? Then it
> >will be absolutely clear.
>
> Prabhu already has one. Right now he is working on removing the
> profileration of UI param diff-copy-from from all the layer in favour
> of generic send_copyfrom_args.
That's great. It would be good to include the above test scenario as
well.
Thanks.
- Julian
RE: [PATCH] enhance diff to make it behave uniformly
Posted by Kamesh Jayachandran <ka...@collab.net>.
> I understand your patch fixes the following two cases.
>
> 1. svn di -cN explicitly_reinstated_file_with_mod_at_rN
>Hi Kamesh and Prabhu.
>What you're describing here sounds good - it sounds like a simpler and
>more definite change than what I understood before - but I'm not sure
>precisely what "explicitly_reinstated_file_with_mod_at_rN" means.
I mean the following,
$cd /tmp
$svnadmin create aaa
$svn co file:///tmp/aaa/ aaa_wc
Checked out revision 0.
$cd aaa_wc/
$echo "line1"> test.c
$svn add test.c
A test.c
$svn ci -m "q"
Adding test.c
Transmitting file data .
Committed revision 1.
$svn rm test.c
D test.c
$svn ci -m "q"
Deleting test.c
Committed revision 2.
$/u1/SvnEdge/svn-server/bin/svn cp file:///tmp/aaa/test.c@1 .
A test.c
$echo "line2">> test.c
$svn ci -m "q"
Adding test.c
Transmitting file data .
Committed revision 3.
$/u1/SvnEdge/svn-server/bin/svn di -c3 test.c
svn: Unable to find repository location for 'test.c' in revision 2
Above error would not happen with this patch. It would rather give something like svnlook diff --diff-copy-from.
i.e
$svnlook diff --diff-copy-from -r3 /tmp/aaa
Copied: test.c (from rev 1, test.c)
===================================================================
--- test.c 2010-12-03 19:48:15 UTC (rev 1)
+++ test.c 2010-12-03 19:49:40 UTC (rev 3)
@@ -1 +1,2 @@
line1
+line2
>Please could you include a test for these cases in your patch? Then it
>will be absolutely clear.
Prabhu already has one. Right now he is working on removing the profileration of UI param diff-copy-from from all the layer in favour of generic send_copyfrom_args.
With regards
Kamesh Jayachandran
Re: [PATCH] enhance diff to make it behave uniformly
Posted by Julian Foad <ju...@wandisco.com>.
On Wed, 2010-12-01, Kamesh Jayachandran wrote:
> Hi Prabhu,
>
> I understand your patch fixes the following two cases.
>
> 1. svn di -cN explicitly_reinstated_file_with_mod_at_rN
Hi Kamesh and Prabhu.
What you're describing here sounds good - it sounds like a simpler and
more definite change than what I understood before - but I'm not sure
precisely what "explicitly_reinstated_file_with_mod_at_rN" means.
Please could you include a test for these cases in your patch? Then it
will be absolutely clear.
Thanks.
- Julian
> Without your patch we get a failure
> "explicitly_reinstated_file_with_mod_at_rN does not exist at rN-1"
> With your patch we get *modification* alone.
>
> 2. svn di -cN Parent_Dir_of_explicitly_reinstated_file_at_rN
> Without your patch we get all lines as added.
> With your patch we get only what got injected along with copy in rN.
>
>
> So if somebody wants all lines as added in above cases they can use
> --show-copies-as-adds.
>
> Please fix your patch to remove this new param diff-copy-from in all the
> layers and assume it to be TRUE unless "--show-copies-as-adds" is given.
>
> Thanks
> With regards
> Kamesh Jayachandran
>
> On 11/30/2010 03:26 PM, Prabhu Gnana Sundar wrote:
> > Hi,
> >
> > In my previous patch [1] I suggested the addition of the
> > '--diff-copy-from' switch to 'svn diff' to solve the cases where 'svn
> > diff' would do the diff against the copyfrom source file when a file is
> > reinstated or diff target is *not* explicit file.
> >
> > But since there is already a point that diff behaves differently in
> > different scenarios, making 'svn diff' work the same in all scenarios is
> > one good option. This patch would solve the problem of making 'svn diff'
> > work the same in all conditions by passing 'TRUE' for the
> > 'diff_copy_from' option by default. Hence I removed the
> > '--diff-copy-from' switch in this patch. This is a more generic way to
> > bridge the gap.
> >
> > I am attaching the patch and the log with this mail. Please review and
> > comment on the same.
> >
> > I still retain this param on svn_client_diff5 and svn_ra_diff4 in case
> > somebody wants old behaviour, otherwise I can remove that too.
> >
> > Regards
> > Prabhu
> >
> > [1]http://mail-archives.apache.org/mod_mbox/subversion-dev/201011.mbox/%
> > 3C1291018400.4021.3.camel@prabhugnanasundar%3E
>