2009-08-26 17:24:54 Stefan Sperling napisał(a):
> So the question whether the current implementation of support for
> the SVNPATCH meta data in patches should be removed from trunk has
> been open for some time now.
>
> I want to bring this topic up again before we get too close for
> release to be able to discuss this question in the depth it deserves.
>
> To recap what this feature is about, I am quoting bits from notes/svnpatch
> below. See that file for all the details.
>
> [[[
> The svnpatch format is in fact a simplified version of the Subversion protocol
> -- see subversion/libsvn_ra_svn/protocol -- that meets our needs. The
> advantage here is that changes are serialized into a language that Subversion
> already speaks and only a few minor tweaks were needed to accommodate. As an
> example, revisions have been stripped from the protocol to allow fuzzing.
>
> The implementation with the command line client uses `svn diff --svnpatch' to
> generate the rich diffs and `svn patch' to apply the diffs against a working
> copy. Other frontends can also take advantage of svnpatch in the same way
> through the usual API's svn_client_diff5 and svn_client_patch that use files to
> communicate.
> ]]]
>
> This sounds very nifty, but there is a problem with the implementation
> (yes, only with the implementation, the feature itself is great)
> which is also mentioned in the notes:
>
> [[[
> 'svn patch' is similar in many ways to 'svn merge'. Basically, we have a
> tree-delta in hand that we want to apply to a working-tree. Thus it's not
> surprising to see they have a lot in common when comparing both implementations.
> 'patch' uses a mix of revamped merge_callbacks (see libsvn_client/merge.c) and
> repos-repos editor functions (see libsvn_client/repos_diff.c). Why not merge
> those two together then, for code-share sake? Well, although they share a close
> logic, to join the two implies having one single file (repos_diff.c) to handle
> at least three burdens: repos-repos diff, merge, and patch. Such a design
> can't be achieved without a myriad of tests/conditions and a large amount of
> blurry mess at mixing three different tools in one place. In the end, what was
> supposed to enhance software maintainability turned out to cause a lot of damage
> at tightening different things together.
> ]]]
>
> What this means is that the implementation adds about 3800 lines of code
> to the subversion code base. The code implements three new editors,
> duplicating code from libsvn_client/merge.c, libsvn_client/repos_diff.c,
> and libsvn_ra_svn/client.c, and calls into these editors from existing
> editors, complicating existing editors as well.
>
> There are concerns about the maintainability of this.
> I don't exactly know when or why the decision to copy-paste-modify
> all this code was made, but it's happened now and we have to think about
> whether that decision was right. But I can't believe that this was
> inevitable. I think it is possible that with the right abstractions,
> a lot of the code duplication could be avoided.
>
> While working on the unidiff support, I have been avoiding this "other
> half" of svn patch. The unidiff application support is entirely independent.
> So removing the svnpatch block support would not affect the unidiff
> application support.
>
> Below is a diff which removes this implementation from trunk.
> I primarily made it for the purpose of this discussion, so that the
> implementation can be reviewed in isolation. Should we decide to remove it,
> the diff could be applied (I haven't run make check on it yet, but it
> compiles).
>
> Here is what diffstat has to say about this diff:
> include/svn_client.h | 67 -
> include/svn_wc.h | 39
> libsvn_client/client.h | 11
> libsvn_client/deprecated.c | 50 -
> libsvn_client/diff.c | 107 --
> libsvn_client/merge.c | 3
> libsvn_client/patch.c | 1744 -------------------------------------------
> libsvn_client/repos_diff.c | 479 -----------
> libsvn_wc/deprecated.c | 3
> libsvn_wc/diff.c | 1064 --------------------------
> svn/diff-cmd.c | 10
> svn/main.c | 28
> svn/patch-cmd.c | 4
> tests/cmdline/patch_tests.py | 267 ------
> 14 files changed, 50 insertions(+), 3826 deletions(-)
>
> Thoughts? Comments? Should we remove this?
> If not, ideas about how to reduce the bloat?
> People who'd like to work on getting this trimmed down before release?
> Or is everything OK as it is?
>
> Stefan
>
> Index: subversion/tests/cmdline/patch_tests.py
> ==================================================================--- subversion/tests/cmdline/patch_tests.py (revision 38940)
> +++ subversion/tests/cmdline/patch_tests.py (working copy)
> @@ -46,111 +46,8 @@ SkipUnless = svntest.testcase.SkipUnless
> Item = svntest.wc.StateItem
>
> ########################################################################
> -#Tools
> -
> -
> -def svnpatch_encode(l):
> - return [x + "\n" for x in textwrap.wrap(base64.encodestring(zlib.compress("".join(l))), 76)]
> -
> -########################################################################
> #Tests
>
> -def patch_basic(sbox):
> - "'svn patch' basic functionality with no unidiff"
> -
> - sbox.build()
> - wc_dir = sbox.wc_dir
> -
> - # We might want to use The-Merge-Kludge trick here
> - patch_file_path = tempfile.mkstemp(dir=os.path.abspath(svntest.main.temp_dir))[1]
> -
> - os.chdir(wc_dir)
> -
> - svnpatch = [
> - '( open-root ( 2:d0 ) ) ',
> - '( open-dir ( 1:A 2:d0 2:d1 ) ) ',
> - '( open-dir ( 3:A/B 2:d1 2:d2 ) ) ',
> - '( delete-entry ( 5:A/B/E 2:d2 ) ) ',
> - '( delete-entry ( 10:A/B/lambda 2:d2 ) ) ',
> - '( close-dir ( 2:d2 ) ) ',
> - '( open-dir ( 3:A/C 2:d1 2:d3 ) ) ',
> - '( add-dir ( 10:A/C/newdir 2:d3 2:d4 ( ) ) ) ',
> - '( close-dir ( 2:d4 ) ) ',
> - '( close-dir ( 2:d3 ) ) ',
> - '( open-dir ( 3:A/D 2:d1 2:d5 ) ) ',
> - '( open-dir ( 5:A/D/H 2:d5 2:d6 ) ) ',
> - '( open-file ( 9:A/D/H/psi 2:d6 2:c7 ) ) ',
> - '( change-file-prop ( 2:c7 7:psiprop ( 10:psipropval ) ) ) ',
> - '( close-file ( 2:c7 ( ) ) ) ',
> - '( close-dir ( 2:d6 ) ) ',
> - '( close-dir ( 2:d5 ) ) ',
> - '( open-file ( 4:A/mu 2:d1 2:c8 ) ) ',
> - '( change-file-prop ( 2:c8 13:svn:mime-type ( 24:application/octet-stream ) ) ) ',
> - '( apply-textdelta ( 2:c8 ( ) ) ) ',
> - '( textdelta-chunk ( 2:c8 4:SVN\001 ) ) ',
> - '( textdelta-chunk ( 2:c8 5:\000\000\057\0020 ) ) ',
> - '( textdelta-chunk ( 2:c8 2:\001\257 ) ) ',
> - '( textdelta-chunk ( 2:c8 48:/This is the file \'mu\'.\n',
> - 'Some\001more\002binary\003bytes\000\n',
> - ' ) ) ',
> - '( textdelta-end ( 2:c8 ) ) ',
> - '( close-file ( 2:c8 ( 32:24bf575dae88ead0eaa0f3863090bd90 ) ) ) ',
> - '( close-dir ( 2:d1 ) ) ',
> - '( add-file ( 3:foo 2:d0 2:c9 ( ) ) ) ',
> - '( close-file ( 2:c9 ( ) ) ) ',
> - '( close-dir ( 2:d0 ) ) ',
> - '( close-edit ( ) ) ',
> - ]
> -
> - svnpatch = svnpatch_encode(svnpatch)
> - svntest.main.file_write(patch_file_path,\
> - '========================= SVNPATCH1 BLOCK =========================\n')
> - svntest.main.file_append(patch_file_path, ''.join(svnpatch))
> -
> - expected_output = wc.State('.', {
> - 'A/B/lambda' : Item(status="D "),
> - 'A/B/E' : Item(status="D "),
> - 'A/B/E/alpha' : Item(status="D "),
> - 'A/B/E/beta' : Item(status="D "),
> - 'A/mu' : Item(status="UU"),
> - 'A/C/newdir' : Item(status="A "),
> - 'A/D/H/psi' : Item(status=" U"),
> - 'foo' : Item(status="A "),
> - })
> -
> - expected_disk = svntest.main.greek_state.copy()
> - expected_disk.remove('A/B/lambda', 'A/B/E/alpha',
> - 'A/B/E/beta') # A/B/E is still there (until commit)
> - mu_contents = "This is the file 'mu'.\nSome\001more\002binary\003bytes\000\n"
> - expected_disk.add({
> - 'A/C/newdir' : Item(),
> - 'foo' : Item(''), # empty file, ready for Unidiffs
> - })
> - expected_disk.tweak('A/mu', contents=mu_contents,
> - props={'svn:mime-type':'application/octet-stream'})
> - expected_disk.tweak('A/D/H/psi', props={'psiprop':'psipropval'})
> -
> - expected_status = svntest.actions.get_virginal_state('.', 1)
> - expected_status.tweak('A/B/E/alpha', 'A/B/E/beta', 'A/B/E', 'A/B/lambda',
> - status="D ", wc_rev=1)
> - expected_status.tweak('A/mu', status="MM")
> - expected_status.tweak('A/D/H/psi', status=" M")
> - expected_status.add({
> - 'foo' : Item(status="A ", wc_rev=1),
> - 'A/C/newdir' : Item(status="A ", wc_rev=0),
> - })
> -
> - expected_skip = wc.State('', { })
> -
> - svntest.actions.run_and_verify_patch('.', os.path.abspath(patch_file_path),
> - expected_output,
> - expected_disk,
> - None,
> - expected_skip,
> - None, # expected err
> - 1, # check-props
> - 0) # no dry-run, outputs differ
> -
> def patch_unidiff(sbox):
> "apply a unidiff patch"
>
> @@ -320,168 +217,7 @@ def patch_unidiff(sbox):
> 1, # check-props
> 0) # dry-run
>
> -def patch_copy_and_move(sbox):
> - "test copy and move operations"
>
> - sbox.build()
> - wc_dir = sbox.wc_dir
> -
> - # two subtests
> - wc2_dir = sbox.add_wc_path('wc2')
> - abs_wc2_dir = os.path.abspath(wc2_dir)
> -
> - patch_file_path = tempfile.mkstemp(dir=os.path.abspath(svntest.main.temp_dir))[1]
> -
> - mu_path = os.path.join('A', 'mu')
> - gamma_path = os.path.join('A', 'D', 'gamma')
> -
> - os.chdir(wc_dir)
> -
> - # set up some properties to ensure base props are considered in the
> - # copy and move operations, and commit r2
> - svntest.main.run_svn(None, 'propset', 'pristinem', 'pristm',
> - mu_path)
> - svntest.main.run_svn(None, 'propset', 'pristineg', 'pristg',
> - gamma_path)
> - svntest.main.run_svn(None, 'ci', '-m', 'log msg')
> - svntest.main.run_svn(None, 'up')
> -
> - # Subtest 1
> - # The aim of this test is to ensure that the move operation will not
> - # fail when the delete-entry strikes before the add-file with
> - # copyfrom. Since the file (A/mu) doesn't have local mods it is
> - # unversioned and removed from disk at delete-entry time. The
> - # add-file should use its text-base instead. The order matters,
> - # because of the depth-first algorithm. Additionally, we also test a
> - # basic copy operation with pristine and working properties and text
> - # modifications.
> -
> - unidiff_patch = [
> - "Index: A/mu (deleted)\n",
> - "===================================================================\n",
> - "Index: A/C/gamma\n",
> - "===================================================================\n",
> - "--- A/C/gamma\t(revision 2)\n",
> - "+++ A/C/gamma\t(working copy)\n",
> - "@@ -1 +1,2 @@\n",
> - " This is the file 'gamma'.\n",
> - "+some more bytes to 'gamma'\n",
> - "\n",
> - "Property changes on: A/C/gamma\n",
> - "___________________________________________________________________\n",
> - "Name: svn:mergeinfo\n",
> - "\n",
> - "Index: A/D/gamma\n",
> - "===================================================================\n",
> - "--- A/D/gamma\t(revision 2)\n",
> - "+++ A/D/gamma\t(working copy)\n",
> - "@@ -1 +1,2 @@\n",
> - " This is the file 'gamma'.\n",
> - "+some more bytes to 'gamma'\n",
> - "\n",
> - "Property changes on: mu-ng\n",
> - "___________________________________________________________________\n",
> - "Name: newprop\n",
> - " + newpropval\n",
> - "Name: svn:mergeinfo\n",
> - "\n",
> - ]
> -
> - svnpatch = [
> - '( open-root ( 2:d0 ) ) ',
> - '( open-dir ( 1:A 2:d0 2:d1 ) ) ',
> - '( open-dir ( 3:A/C 2:d1 2:d2 ) ) ',
> - '( add-file ( 9:A/C/gamma 2:d2 2:c3 ( 9:A/D/gamma ) ) ) ',
> - '( change-file-prop ( 2:c3 13:svn:mergeinfo ( 0: ) ) ) ',
> - '( close-file ( 2:c3 ( ) ) ) ',
> - '( close-dir ( 2:d2 ) ) ',
> - '( delete-entry ( 4:A/mu 2:d1 ) ) ',
> - '( close-dir ( 2:d1 ) ) ',
> - '( add-file ( 5:mu-ng 2:d0 2:c4 ( 4:A/mu ) ) ) ',
> - '( change-file-prop ( 2:c4 7:newprop ( 10:newpropval ) ) ) ',
> - '( change-file-prop ( 2:c4 13:svn:mergeinfo ( 0: ) ) ) ',
> - '( close-file ( 2:c4 ( ) ) ) ',
> - '( close-dir ( 2:d0 ) ) ',
> - '( close-edit ( ) ) ',
> - ]
> -
> - svnpatch = svnpatch_encode(svnpatch)
> - svntest.main.file_write(patch_file_path, ''.join(unidiff_patch))
> - svntest.main.file_append(patch_file_path,
> - '========================= SVNPATCH1 BLOCK =========================\n')
> - svntest.main.file_append(patch_file_path, ''.join(svnpatch))
> -
> - expected_output = [
> - 'A %s\n' % os.path.join('A', 'C', 'gamma'),
> - 'D %s\n' % os.path.join('A', 'mu'),
> - 'A mu-ng\n',
> - 'U %s\n' % os.path.join('A', 'C', 'gamma'),
> - 'U %s\n' % os.path.join('A', 'D', 'gamma'),
> - ]
> -
> - gamma_contents = "This is the file 'gamma'.\nsome more bytes to 'gamma'\n"
> - mu_contents="This is the file 'mu'.\n"
> -
> - expected_disk = svntest.main.greek_state.copy()
> - expected_disk.remove('A/mu')
> - expected_disk.tweak('A/D/gamma', contents=gamma_contents,
> - props={'pristineg': 'pristg'})
> - expected_disk.add({
> - 'A/C/gamma' : Item(gamma_contents,
> - props={SVN_PROP_MERGEINFO : '',
> - 'pristineg': 'pristg'}),
> - 'mu-ng' : Item(mu_contents,
> - props={SVN_PROP_MERGEINFO : '',
> - 'pristinem': 'pristm',
> - 'newprop': 'newpropval'}),
> - })
> -
> - expected_status = svntest.actions.get_virginal_state('', 2)
> - expected_status.tweak('A/mu', status='D ')
> - expected_status.tweak('A/D/gamma', status='M ')
> - expected_status.add({
> - 'A/C/gamma' : Item(status="A ", copied='+', wc_rev='-'),
> - 'mu-ng' : Item(status="A ", copied='+', wc_rev='-'),
> - })
> -
> - expected_skip = wc.State('', { })
> -
> - svntest.actions.run_and_verify_patch('.', os.path.abspath(patch_file_path),
> - expected_output,
> - expected_disk,
> - expected_status,
> - expected_skip,
> - None, # expected err
> - 1, # check-props
> - 0) # dry-run
> -
> - # Subtest 2
> - # The idea is to take subtest 1 and to add some local mods to A/mu.
> - # The delete-entry should leave the working file in place and the
> - # add-entry use it as its copyfrom argument suggests. In other words,
> - # this tests the move-file-with-copyfrom-path-modified case.
> -
> - svntest.actions.run_and_verify_svn(None, None, [], 'checkout',
> - sbox.repo_url, abs_wc2_dir)
> - os.chdir(abs_wc2_dir)
> -
> - svntest.main.file_append(mu_path, 'Junk here, junk now.\n')
> - mu_contents += 'Junk here, junk now.\n'
> - expected_disk.tweak('mu-ng', contents=mu_contents)
> -
> - # A/mu is unversioned but remains on disk with/because of its local mods
> - expected_disk.add({'A/mu' : Item(contents=mu_contents,
> - props={'pristinem' : 'pristm' })})
> -
> - svntest.actions.run_and_verify_patch('.', os.path.abspath(patch_file_path),
> - expected_output,
> - expected_disk,
> - expected_status,
> - expected_skip,
> - None, # expected err
> - 1, # check-props
> - 0) # dry-run
> -
> def patch_unidiff_absolute_paths(sbox):
> "apply a unidiff patch containing absolute paths"
>
> @@ -544,6 +280,7 @@ def patch_unidiff_absolute_paths(sbox):
> 1, # check-props
> 0) # dry-run
>
> +
> def patch_unidiff_offset(sbox):
> "apply a unidiff patch with offset searching"
>
> @@ -759,9 +496,7 @@ def patch_unidiff_offset(sbox):
>
> # list all tests here, starting with None:
> test_list = [ None,
> - patch_basic,
> patch_unidiff,
> - patch_copy_and_move,
> patch_unidiff_absolute_paths,
> patch_unidiff_offset,
> ]
Removing references to these tests from test_list would be sufficient.
Removing definitions of these tests would cause needless problems for
maintainers of branches on which these tests have been modified.
--
Arfrever Frehtes Taifersar Arahesis
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2388410
Received on 2009-08-29 00:59:26 CEST