You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2006/03/12 23:27:23 UTC
[PATCH] diff base->repos failed to trace a moved file
A base->repos diff of a moved file gave the wrong output.
(Subversion failed to notice the move and gave an all-lines-deleted diff.)
A reproduction recipe is:
echo "Old" > file
svn add file
svn ci -m ""
svn mv file file-new
echo "New" > file-new
svn ci -m ""
svn diff -r BASE:1 file-new
(where 1 is the revision created by the initial commit)
This results in the following output:
Index: file-new
===================================================================
--- file-new (working copy)
+++ file-new (revision 1)
@@ -1 +0,0 @@
-New
The first problem fixed by this patch is that it didn't notice that it had to
trace "file-new" through history from the implied peg at WC back to revision
number 1, and so it thought the file didn't exist at r1. Fixing svn_cl__diff()
to request a "pegged" diff for a non-local end-rev as well as for a non-local
start-rev, the result is:
Index: file-new
===================================================================
--- file-new (.../file) (working copy)
+++ file-new (.../file-new) (revision 1)
@@ -1 +1 @@
-New
+Old
which is better: the diff is correct except that the parenthetical file names
are the wrong way around. This bug is fixed in
libsvn_client/diff.c:diff_repos_wc(), which was failing to observe its
"reverse" flag, and the result is then correct:
Index: file-new
===================================================================
--- file-new (.../file-new) (working copy)
+++ file-new (.../file) (revision 1)
@@ -1 +1 @@
-New
+Old
Any comments before I commit this?
- Julian
Re: [PATCH] diff base->repos failed to trace a moved file
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Mon, Mar 13, 2006 at 12:03:42PM +0000, Julian Foad wrote:
> For now, are you OK with me committing that patch and test which includes
> the comment:
>
> >+ # It's not clear whether we expect a file-change diff or
> >+ # a file-delete plus file-add. The former is currently produced if we
> >+ # explicitly request a diff of the file itself, and the latter if we
> >+ # request a tree diff which just happens to contain the file.
>
> or would you prefer me to change the comment (to avoid saying that it's not
> clear, perhaps)?
>
Please commit; I think the comment's fair: it's still not entirely clear
how things should work.
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] diff base->repos failed to trace a moved file
Posted by Julian Foad <ju...@btopenworld.com>.
Malcolm Rowe wrote:
> On Mon, Mar 13, 2006 at 11:22:36AM +0000, Julian Foad wrote:
>
>>From a high level point of view: why should comparing a directory show
>>changes to a file in a different way from comparing the file?
>
> Only because we don't support moves as a distinct logical operation;
> the directory case is essentially a superset of the file case where we
> additionally see the delete of the original file.
>
> (alternative answer: it shouldn't, and won't, I hope, when we get
> 'real moves').
Good!
>>From a low level point of view, "supports copyfrom" is the crucial phrase.
>>I wonder what we mean by this.
>
> Yes, me too. If you look in libsvn_wc/diff.c, you'll see the comment
> "### TODO: Do we need to support copyfrom?". I'm not entirely sure,
> but I suspect it might be the difference between reporting a file_changed
> and file_added event for the copied files.
OK, that's something to investigate and cogitate on.
For now, are you OK with me committing that patch and test which includes the
comment:
> + # It's not clear whether we expect a file-change diff or
> + # a file-delete plus file-add. The former is currently produced if we
> + # explicitly request a diff of the file itself, and the latter if we
> + # request a tree diff which just happens to contain the file.
or would you prefer me to change the comment (to avoid saying that it's not
clear, perhaps)?
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] diff base->repos failed to trace a moved file
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Mon, Mar 13, 2006 at 11:22:36AM +0000, Julian Foad wrote:
> From a high level point of view: why should comparing a directory show
> changes to a file in a different way from comparing the file?
>
Only because we don't support moves as a distinct logical operation;
the directory case is essentially a superset of the file case where we
additionally see the delete of the original file.
(alternative answer: it shouldn't, and won't, I hope, when we get
'real moves').
> From a low level point of view, "supports copyfrom" is the crucial phrase.
> I wonder what we mean by this.
>
Yes, me too. If you look in libsvn_wc/diff.c, you'll see the comment
"### TODO: Do we need to support copyfrom?". I'm not entirely sure,
but I suspect it might be the difference between reporting a file_changed
and file_added event for the copied files.
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] diff base->repos failed to trace a moved file
Posted by Julian Foad <ju...@btopenworld.com>.
Malcolm Rowe wrote:
> On Sun, Mar 12, 2006 at 11:27:23PM +0000, Julian Foad wrote:
>
>>Any comments before I commit this?
>>
>
>
> +1, makes sense.
>
>
>>+#----------------------------------------------------------------------
>>+# A base->repos diff of a moved file used to output an all-lines-deleted diff
>>+def diff_base_repos_moved(sbox):
>>+ "base->repos diff of moved file"
>>+
>>+ sbox.build()
>>+
>>+ current_dir = os.getcwd()
>>+ os.chdir(sbox.wc_dir)
>>+ try:
>>+ oldfile = 'iota'
>>+ newfile = 'kappa'
>>+
>>+ # Move, modify and commit a file
>>+ svntest.main.run_svn(None, 'mv', oldfile, newfile)
>>+ open(newfile, 'w').write("new content\n")
>>+ svntest.actions.run_and_verify_svn(None, None, [], 'ci', '-m', '')
>>+
>>+ # Check that a base->repos diff shows deleted and added lines.
>>+ # It's not clear whether we expect a file-change diff or
>>+ # a file-delete plus file-add. The former is currently produced if we
>>+ # explicitly request a diff of the file itself, and the latter if we
>>+ # request a tree diff which just happens to contain the file.
>
>
> That's correct. When you select the file itself, you're asking for a
> diff between the file as it exists in BASE (called 'kappa') and the same
> file traced back to r1 (then called 'iota'). So that's a file-change.
Yes.
> However, when you select the directory, you're asking for the changes
> between BASE and r1 of the directory. If diff supports copyfrom (and I
> can't remember whether it does in this specific case), we should see a
> file-change for 'kappa' as above, plus a file-add for 'iota'. If not,
> we should see a file-delete for 'kappa' and a file-add for 'iota'.
From a high level point of view: why should comparing a directory show changes
to a file in a different way from comparing the file?
From a low level point of view, "supports copyfrom" is the crucial phrase. I
wonder what we mean by this.
> We should also be able to verify that 'svn diff -r1:BASE' gives the
> opposite diff.
That would be a good test, yes.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] diff base->repos failed to trace a moved file
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Sun, Mar 12, 2006 at 11:27:23PM +0000, Julian Foad wrote:
> Any comments before I commit this?
>
+1, makes sense.
> +#----------------------------------------------------------------------
> +# A base->repos diff of a moved file used to output an all-lines-deleted diff
> +def diff_base_repos_moved(sbox):
> + "base->repos diff of moved file"
> +
> + sbox.build()
> +
> + current_dir = os.getcwd()
> + os.chdir(sbox.wc_dir)
> + try:
> + oldfile = 'iota'
> + newfile = 'kappa'
> +
> + # Move, modify and commit a file
> + svntest.main.run_svn(None, 'mv', oldfile, newfile)
> + open(newfile, 'w').write("new content\n")
> + svntest.actions.run_and_verify_svn(None, None, [], 'ci', '-m', '')
> +
> + # Check that a base->repos diff shows deleted and added lines.
> + # It's not clear whether we expect a file-change diff or
> + # a file-delete plus file-add. The former is currently produced if we
> + # explicitly request a diff of the file itself, and the latter if we
> + # request a tree diff which just happens to contain the file.
That's correct. When you select the file itself, you're asking for a
diff between the file as it exists in BASE (called 'kappa') and the same
file traced back to r1 (then called 'iota'). So that's a file-change.
However, when you select the directory, you're asking for the changes
between BASE and r1 of the directory. If diff supports copyfrom (and I
can't remember whether it does in this specific case), we should see a
file-change for 'kappa' as above, plus a file-add for 'iota'. If not,
we should see a file-delete for 'kappa' and a file-add for 'iota'.
We should also be able to verify that 'svn diff -r1:BASE' gives the
opposite diff.
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org