Re: [PATCH] make diff against copy-source by default
Hi Julian, On Thu, 2011-01-06 at 16:25 +0530, Prabhu Gnana Sundar wrote: Isn't it a layering violation for libsvn_wc to know about libsvn_ra? Maybe this needs to use callbacks or something, so that all the RA knowledge remains in libsvn_client. What you say is correct. :) But since we need to access a file which is not in our text-base. I had to get the ra_session to make use of the repository to fetch the file. Anyway now I am working on this to make use of any function in libsvn_client that would get me an ra_session. But that seems to me like a costly work. I was trying to make use of some functions from libsvn_client to keep the RA knowldege in the libsvn_client. In that course, I ended up in a circular dependency in the 'build.conf'. I had to include the svn_client.h in the 'subversion/libsvn_wc/diff.c' file. So I updated the build.conf file and ran the 'autogen.sh' script, there by ending up in *error*. Here is the snip: snip Traceback (most recent call last): File ./gen-make.py, line 313, in module main(conf, gentype, skip_depends=skip, other_options=rest.list) File ./gen-make.py, line 65, in main generator.write() File build/generator/gen_make.py, line 375, in write files = gen_base._sorted_files(self.graph, area) File build/generator/gen_base.py, line 1148, in _sorted_files raise CircularDependencies() gen_base.CircularDependencies /snip I could not find any other way than teaching libsvn_ra to libsvn_wc inorder to fetch a file from the repo (like done in the patch). Am very glad if you have other suggestions :) Thanks and regards Prabhu
Re: [PATCH] make diff against copy-source by default
On Fri, 2011-01-07 at 15:45 +0530, Prabhu Gnana Sundar wrote: Hi Julian, On Thu, 2011-01-06 at 16:25 +0530, Prabhu Gnana Sundar wrote: Isn't it a layering violation for libsvn_wc to know about libsvn_ra? Maybe this needs to use callbacks or something, so that all the RA knowledge remains in libsvn_client. What you say is correct. :) But since we need to access a file which is not in our text-base. I had to get the ra_session to make use of the repository to fetch the file. Anyway now I am working on this to make use of any function in libsvn_client that would get me an ra_session. But that seems to me like a costly work. I was trying to make use of some functions from libsvn_client to keep the RA knowldege in the libsvn_client. In that course, I ended up in a circular dependency in the 'build.conf'. I had to include the svn_client.h in the 'subversion/libsvn_wc/diff.c' file. So I updated the build.conf file and ran the 'autogen.sh' script, there by ending up in *error*. I could not find any other way than teaching libsvn_ra to libsvn_wc inorder to fetch a file from the repo (like done in the patch). Am very glad if you have other suggestions :) I can't look into this in detail, but I have two thoughts: 1. Ideally, I would expect the structure of the diff code to be something like: libsvn_client: call libsvn_wc to get the WC file; call libsvn_ra to get the repos file; call libsvn_diff to compare the two files; report the results. So libsvn_wc would not need to read files from the repository. 2. If for some reason the code is structured such that libsvn_wc does need to fetch a repository file, it should do so via a callback function provided by libsvn_client, not by calling libsvn_client or libsvn_ra functions directly. There is already some code somewhere in libsvn_wc that uses a callback into libsvn_client to fetch a file's text from the repository. I can't remember where. - Julian
Re: [PATCH] make diff against copy-source by default
Hi Julian, On Fri, 2011-01-07 at 12:17 +, Julian Foad wrote: I can't look into this in detail, but I have two thoughts: 1. Ideally, I would expect the structure of the diff code to be something like: libsvn_client: call libsvn_wc to get the WC file; call libsvn_ra to get the repos file; call libsvn_diff to compare the two files; report the results. So libsvn_wc would not need to read files from the repository. 2. If for some reason the code is structured such that libsvn_wc does need to fetch a repository file, it should do so via a callback function provided by libsvn_client, not by calling libsvn_client or libsvn_ra functions directly. There is already some code somewhere in libsvn_wc that uses a callback into libsvn_client to fetch a file's text from the repository. I can't remember where. That's really cool... You are absolutely right in your points. The first suggestion does not hold in certain cases where the 'source file' would not be available in the text-base. I will look closely into your second suggestion :) Thanks and regards Prabhu
Re: [PATCH] make diff against copy-source by default
Hi Julian, On Wed, 2011-01-05 at 11:41 +, Julian Foad wrote: On Thu, 2010-12-30, Prabhu Gnana Sundar wrote: [...] I like this change, in principle. Thank you :) For example, maybe some tables something like this would be a good way to summarize the changes: Type of diff shown BEFORE this patch, WITHOUT --show-copies-as-adds: +---+---+--+---+ | | copied only | copied and modified | ... | +---+---+--+---+ | | | | | | WC-WC diff | | | | | | | | | | WC-repo diff | | | | | | | | | | repo-repo diff | | | | | | | | | +---+---+--+---+ Or whatever rows and columns best convey the information. I made a tabular summary of the change that this patch would make. Since it displayed weirdly in the mail body I have attached it as a file(diff-explanation-table.txt) with this mail. I have also attached the experiment carried out, in another file(diff-explanation.txt), with this mail. Please let me know if I am not clear at any point. Are there any differences with different (old) versions of Subversion server? (I seem to recall that copy-from data was not always supplied, but I am not sure of the details.) Actually, the send_copyfrom_args was passed even without this patch, but was not handled. Only for 'svnserve' this was newly introduced by this patch. So I don't think there's something to worry. Isn't it a layering violation for libsvn_wc to know about libsvn_ra? Maybe this needs to use callbacks or something, so that all the RA knowledge remains in libsvn_client. What you say is correct. :) But since we need to access a file which is not in our text-base. I had to get the ra_session to make use of the repository to fetch the file. Anyway now I am working on this to make use of any function in libsvn_client that would get me an ra_session. But that seems to me like a costly work. Thanks and regards Prabhu $ svnadmin create diffrepo $ svn co file:///tmp/diffrepo diffwc Checked out revision 0. $ cd diffwc/ $ vi file1 $ svn add file1 A file1 $ svn ci -m adding new file Adding file1 Transmitting file data . Committed revision 1. $ svn cp file1 copiedfile1 A copiedfile1 $ svn ci -m copied file1 to copiedfile1 Adding copiedfile1 Committed revision 2. Copy only: $ svn up Updating '.' ... At revision 2. (with patch) $ svn diff -r2:1 Index: copiedfile1 === --- copiedfile1 (revision 2) +++ copiedfile1 (revision 1) @@ -1,5 +0,0 @@ -1 -2 -3 -4 -5 (without patch) $ svn diff -r2:1 Index: copiedfile1 === --- copiedfile1 (revision 2) +++ copiedfile1 (revision 1) @@ -1,5 +0,0 @@ -1 -2 -3 -4 -5 (with patch) $ svn diff -r2:1 --sca Index: copiedfile1 === --- copiedfile1 (revision 2) +++ copiedfile1 (revision 1) @@ -1,5 +0,0 @@ -1 -2 -3 -4 -5 (without patch) $ svn diff -r2:1 --sca Index: copiedfile1 === --- copiedfile1 (revision 2) +++ copiedfile1 (revision 1) @@ -1,5 +0,0 @@ -1 -2 -3 -4 -5 (with patch) $ svn diff -rBASE:1 Index: copiedfile1 === --- copiedfile1 (working copy) +++ copiedfile1 (revision 1) @@ -1,5 +0,0 @@ -1 -2 -3 -4 -5 (without patch) $ svn diff -rBASE:1 Index: copiedfile1 === --- copiedfile1 (working copy) +++ copiedfile1 (revision 1) @@ -1,5 +0,0 @@ -1 -2 -3 -4 -5 (with patch) $ svn diff -rBASE:1 --sca Index: copiedfile1 === --- copiedfile1 (working copy) +++ copiedfile1 (revision 1) @@ -1,5 +0,0 @@ -1 -2 -3 -4 -5 (without patch) $ svn diff -rBASE:1 --sca Index: copiedfile1 === --- copiedfile1 (working copy) +++ copiedfile1 (revision 1) @@ -1,5 +0,0 @@ -1 -2 -3 -4 -5 $ svn cp file1 copiedfile2 A copiedfile2 (with patch) $ svn diff (without patch) $ svn diff (with patch) $ svn diff --sca Index: copiedfile2 === --- copiedfile2 (revision 0) +++ copiedfile2 (working copy) @@ -0,0 +1,5 @@ +1 +2 +3 +4 +5 (without patch) $ svn diff
Re: [PATCH] make diff against copy-source by default
On Thu, 2011-01-06, Prabhu Gnana Sundar wrote: I made a tabular summary of the change that this patch would make. Since it displayed weirdly in the mail body I have attached it as a file(diff-explanation-table.txt) with this mail. I have also attached the experiment carried out, in another file(diff-explanation.txt), with this mail. Please let me know if I am not clear at any point. 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. Why are some of your tests testing deletes, not adds? $ svn diff -r2:1 Index: copiedfile1 === --- copiedfile1 (revision 2) +++ copiedfile1 (revision 1) @@ -1,5 +0,0 @@ -1 -2 -3 -4 -5 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? - Julian Are there any differences with different (old) versions of Subversion server? (I seem to recall that copy-from data was not always supplied, but I am not sure of the details.) Actually, the send_copyfrom_args was passed even without this patch, but was not handled. Only for 'svnserve' this was newly introduced by this patch. So I don't think there's something to worry. Isn't it a layering violation for libsvn_wc to know about libsvn_ra? Maybe this needs to use callbacks or something, so that all the RA knowledge remains in libsvn_client. What you say is correct. :) But since we need to access a file which is not in our text-base. I had to get the ra_session to make use of the repository to fetch the file. Anyway now I am working on this to make use of any function in libsvn_client that would get me an ra_session. But that seems to me like a costly work. Thanks and regards Prabhu
Re: [PATCH] make diff against copy-source by default
Hi Julian, On Thu, 2011-01-06 at 14:22 +, Julian Foad wrote: Please let me know if I am not clear at any point. 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. Why are some of your tests testing deletes, not adds? $ svn diff -r2:1 Index: copiedfile1 === --- copiedfile1 (revision 2) +++ copiedfile1 (revision 1) @@ -1,5 +0,0 @@ -1 -2 -3 -4 -5 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. :) 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 Index: file1 === --- file1 (.../file1) (revision 1) +++ file1 (.../copiedfile2) (revision 3) @@ -3,3 +3,6 @@ 3 4 5 +6 +7 +8 $ svn diff -r1:3 ./ Index: copiedfile2 === --- copiedfile2 (revision 2) +++ copiedfile2 (revision 3) @@ -3,3 +3,6 @@ 3 4 5 +6 +7 +8 Thanks and regards Prabhu -- | WC - WC | REPOS - WC | REPOS - REPOS | |-++---+--+---+---+ | || | | | | | || only diff| N/A | all adds |all adds | | | BEFORE |---+--+---+---+ | || | | | | | || with '--sca' | all adds| all adds |all adds | |COPIED ++---+--+---+---+ | || | | | | | || only diff| N/A | all adds |all adds | | | AFTER |---+--+---+---+ | || | | | | | || with '--sca' | all adds| all adds |all adds | +-++---+--+---+---+ +-++---+--+---+---+ | || | | | | | || only diff|against source|all adds |all adds | | | BEFORE |---+--+---+---+ | || | | | | | || with '--sca' | all adds | all adds|all adds | |COPIED ++---+--+---+---+ ||| | | | | |MODIFIED || only diff|against source|against source | against source | | | AFTER |---+--+---+---+ | || | | | | | || with '--sca' | all adds| all adds| all adds | +-++---+--+---+---+
Re: [PATCH] make diff against copy-source by default
On Thu, 2010-12-30, Prabhu Gnana Sundar wrote: [...] After a few discussions about the inconsistent behaviour of 'svn diff' in different scenarios, Kamesh suggested that let 'svn diff' be done against the copy-source by default, making use of the copyfrom info. Now this patch would do 'diff' against the copy-source by default, whereas the prevailing default behaviour(of showing a *modified copy* as all adds) can be achieved by using the '--show-copies-as-adds' switch(that already exists in the code-base!). I like this change, in principle. We will have to decide whether it is backwards-compatible enough or whether we need to provide an additional compatibility mode. In order to help us decide, it would be very useful if you could provide a summary of the behavioural changes. For example, maybe some tables something like this would be a good way to summarize the changes: Type of diff shown BEFORE this patch, WITHOUT --show-copies-as-adds: +---+---+--+---+ | | copied only | copied and modified | ... | +---+---+--+---+ | | | | | | WC-WC diff | | | | | | | | | | WC-repo diff | | | | | | | | | | repo-repo diff | | | | | | | | | +---+---+--+---+ Type of diff shown BEFORE this patch, WITH --show-copies-as-adds: +---+---+--+---+ Type of diff shown AFTER this patch, WITHOUT --show-copies-as-adds: +---+---+--+---+ Type of diff shown AFTER this patch, WITH --show-copies-as-adds: +---+---+--+---+ Or whatever rows and columns best convey the information. Are there any differences with different (old) versions of Subversion server? (I seem to recall that copy-from data was not always supplied, but I am not sure of the details.) I was quickly in to it and after spending some time in the process I came to know that several existing 'test cases' *failed* due to the change in the behaviour of the 'svn diff'. Later found that the 'wc editor' needs to be taught to handle this behaviour (handling the copyfrom info and retrieving it from the repository as against working copy text-base). This took quite some amount of time... See [1] below of the mailer.py usecase which tries to get the same output as my patch do. May be that's the behaviour of the 'diff' that is prefered ? :) I have attached the patch and the log message with this mail. Please review and respond. I only had a very quick look at the patch. The first thing that catches my eye is this: * subversion/libsvn_wc/diff.c (): included the svn_ra.h header file. (): persisted the ra_session, copyfrom file, copyfrom path and pristine properties in the edit_baton. (make_edit_baton) : introduced the ra_session. (get_file_from_ra): newly added to fetch file from repository. (add_file): support copyfrom by making use of the copyfrom info. (apply_textdelta) : makes use of the copyfrom_temp file making use of the copyfrom info. (close_file) : makes use of the copyfrom info and tweaked the file_changed to display the copyfrom revision number. (svn_wc_get_diff_editor6) : introduced the ra_session. pass NULL for ra_session to make_edit_baton. Isn't it a layering violation for libsvn_wc to know about libsvn_ra? Maybe this needs to use callbacks or something, so that all the RA knowledge remains in libsvn_client. - Julian
[PATCH] make diff against copy-source by default
Hi all,
This patch is a follow up to the patch that I gave last month
(November).
Here is the link to the mail archive...
http://mail-archives.apache.org/mod_mbox/subversion-dev/201011.mbox/%
3c1291110994.4021.66.ca...@prabhugnanasundar%3e
After a few discussions about the inconsistent behaviour of 'svn diff'
in different scenarios, Kamesh suggested that let 'svn diff' be done
against the copy-source by default, making use of the copyfrom info.
Now this patch would do 'diff' against the copy-source by default,
whereas the prevailing default behaviour(of showing a *modified copy* as
all adds) can be achieved by using the '--show-copies-as-adds'
switch(that already exists in the code-base!).
I was quickly in to it and after spending some time in the process I
came to know that several existing 'test cases' *failed* due to the
change in the behaviour of the 'svn diff'. Later found that the 'wc
editor' needs to be taught to handle this behaviour (handling the
copyfrom info and retrieving it from the repository as against working
copy text-base).
This took quite some amount of time...
See [1] below of the mailer.py usecase which tries to get the same
output as my patch do.
May be that's the behaviour of the 'diff' that is prefered ? :)
I have attached the patch and the log message with this mail. Please
review and respond.
[1]
I found the following usecase which is already doing the same as what my
patch intends to do.
The mailer.py script has the same behaviour of 'diff'ing just as this
patch does.
http://mail-archives.apache.org/mod_mbox/subversion-commits/201012.mbox/%3c20101228204459.1e42d2388...@eris.apache.org%3e
snip from the above commit email
Copied:
subversion/branches/1.6.x-svn_fs_commit_txn/subversion/include/private/svn_repos_private.h
(from r1051763,
subversion/trunk/subversion/include/private/svn_repos_private.h)
URL:
http://svn.apache.org/viewvc/subversion/branches/1.6.x-svn_fs_commit_txn/subversion/include/private/svn_repos_private.h?p2=subversion/branches/1.6.x-svn_fs_commit_txn/subversion/include/private/svn_repos_private.hp1=subversion/trunk/subversion/include/private/svn_repos_private.hr1=1051763r2=1053428rev=1053428view=diff
==
--- subversion/trunk/subversion/include/private/svn_repos_private.h
(original)
+++
subversion/branches/1.6.x-svn_fs_commit_txn/subversion/include/private/svn_repos_private.h
Tue Dec 28 20:44:58 2010
@@ -38,53 +38,6 @@ extern C {
/**
- * Permanently delete @a path at revision @a revision in @a fs.
- *
- * Do not change the content of any other node in the repository, even
other
- * nodes that were copied from this one. The only other change in the
- * repository is to copied from pointers that were pointing to the
- * now-deleted node. These are removed or made to point to a previous
- * version of the now-deleted node.
- * (### TODO: details.)
- *
- * @a path is relative to the repository root and must start with /.
- *
- * If administratively forbidden, return @c SVN_ERR_RA_NOT_AUTHORIZED.
If not
- * implemented by the RA layer or by the server, return
- * @c SVN_ERR_RA_NOT_IMPLEMENTED.
- *
- * @note This functionality is not implemented in pre-1.7 servers and
may not
- * be implemented in all 1.7 and later servers.
- *
- * @note TODO: Maybe create svn_repos_fs_begin_obliteration_txn() and
- * svn_repos_fs_commit_obliteration_txn() to enable an obliteration txn
to be
- * constructed at a higher level.
- *
- * @since New in 1.7.
- */
-svn_error_t *
-svn_repos__obliterate_path_rev(svn_repos_t *repos,
- const char *username,
- svn_revnum_t revision,
- const char *path,
- apr_pool_t *pool);
-
-
-/** Validate that property @a name is valid for use in a Subversion
- * repository; return @c SVN_ERR_REPOS_BAD_ARGS if it isn't. For some
- * svn: properties, also validate the @a value, and return
- * @c SVN_ERR_BAD_PROPERTY_VALUE if it is not valid.
- *
- * Use @a pool for temporary allocations.
- *
- * @since New in 1.7.
- */
-svn_error_t *
-svn_repos__validate_prop(const char *name,
- const svn_string_t *value,
- apr_pool_t *pool);
-
-/**
* Given the error @a err from svn_repos_fs_commit_txn(), return an
* string containing either or both of the svn_fs_commit_txn() error
* and the SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED wrapped error from
/snip
Thanks and regards
Prabhu
Index: build.conf
===
--- build.conf (revision 1053833)
+++ build.conf (working copy)
@@ -336,7 +336,7 @@
description = Subversion Working Copy Library
type = lib
path = subversion/libsvn_wc
-libs = libsvn_delta libsvn_diff libsvn_subr aprutil apriconv apr
+libs = libsvn_ra libsvn_delta libsvn_diff libsvn_subr aprutil apriconv apr
install = lib
msvc-export = svn_wc.h private\svn_wc_private.h
Index: subversion/libsvn_ra/deprecated.c
===
--- subversion/libsvn_ra/deprecated.c
