a small code review (epg/cb2.0-compat)

e...@google.com

unread,

May 22, 2008, 7:39:51 PM5/22/08

to gvn-d...@googlegroups.com

I'd like you to do a code review. To review this change, run

gvn review --project https://gvn.googlecode.com/svn epg/cb2.0-compat@933

Alternatively, to review the latest snapshot of this change
branch, run

gvn --project https://gvn.googlecode.com/svn review epg/cb2.0-compat

to review the following change:

*epg/cb2.0-compat@933 | epg | 2008-05-22 15:39:05 -0800 (Thu, 22 May 2008)

Description:

Implement read support for new changebranch format, the bit that's been
listed in the design doc forever. We'll push out a release that includes
this read support but only writes the old format, then a release that
writes the new format, and then a release that can't read the old.

* doc/design.html
- Add TODO about documenting gvn properties.
- Update new changebranch stuff, including JSON state file, to match what's
actually implemented.

* lib/gvn/changebranch.py
(_DIFF_COMMAND): Remove.
(IsRegularProp, FilterRegularProps): Add functions.
(_ADD_CODES): Add global listing action codes which can be considered Adds
('Add' and 'R'eplace).
(State): Add class representing state of a changebranch snapshot.
(PathState): Add class representing state of a path in a changebranch
snapshot.
(_ParseStateKind, ParseState): Add functions to parse JSON state file in the
repository into State objects, using simplejson module.
(ChangeBranch):
(BranchPath, StatePath): Add methods for assembling branch and state paths
rather than calling project.ChangeBranchPath all over the place. Since
old-style changebranches may have anything as the name of the branch,
add new _branch_name attribute, set by _GetChangedPathsHelper.
(_GetChangedPaths): Move nearly all this to new _GetChangedPathsHelper
which this calls and handles the RepoPath crapitude. This and the
changed_paths property it backs vanish in the next release.
(_GetChangedPathsHelper): Rename topname to branch_name for clarity, and
store it in ._branch_name attribute. Return source and source_revision
in addition to the changes list.
(_OldGetRepoState): Add function to turn _GetChangedPathsHelper's results
into a State object.
(_NewGetRepoState): Add function to return State object from state file in
the repository.
(GetRepoState): Add function to return State object from _NewGetRepoState
or _OldGetRepoState if no state file.
(_makedirs, _GetFile, MakeLocalDiffTree, Diff, _MergeAdd, _MergeRemove,
_MergeReplace, _MergeModify, _MergeActions, Merge): Remove all the Diff
stuff, which was awful and buggy (and Merge, which never worked anyway).
(DiffCallbacks): Add base class for callbacks called by new Diff functions.
(SvnDiffCallbacks): Add DiffCallbacks implementation using svn's internal
textual diff.
(_MaybeWriteProperties, TkDiffCallbacks): Add DiffCallbacks implementation
using tkdiff, and helper function.
(_FindLeft, _RaGetDirOrFile, DiffBaseToSnapshot): Add function and helpers
to diff the base of a snapshot to the snapshot itself (gvn review).
(DiffSnapshotToWC): Add function to diff a snapshot to the working copy (gvn
diff --from-change).
(DiffSnapshots): Just TODO comment; not implemented.

* lib/gvn/diff.py
(GetShortDiff): Adapt for new diff stuff and add TODO about how this
function seems bogus anyway.

* lib/gvn/cmdline.py
(OptionParser.parse_args): Post-process --extensions/-x option the same way
svn does.

* lib/gvn/description.py
(RevisionDescriptionImpl.AffectedPaths): Take pool argument (unused here).
(ChangeBranchDescriptionImpl): Adapt to new GetRepoState; this AffectedPaths
method actually needs the pool.
(Description.Output): Take pool to pass along to AffectedPaths.

* lib/gvn/errors.py
(SVN_NOENT): Add list of error codes that mean ENOENT, and leave TODO about
removing this (fix might make it into 1.5.0).
(NoDiffCommand): Remove; we use svn.diff for internal diff now.
(ParseState): Add new Internal error for failure to parse state file.

* lib/gvn/util.py
(ApplyPropDiffs): Add function to apply svn.core.svn_prop_diffs diffs.
(TmpTree): Add class to manage a temporary tree.

* lib/gvn/wc.py
(WorkingCopy._GetAdmAccess, adm_access): Add new property rather than
mucking about with adm_access all over the place; leave TODO about
actually changing all those places to use this.
(WorkingCopy.AdmRetrieve): Add function to get an adm baton rather than
calling svn.wc.adm_retrieve all over the place; leave TODO about actually
changing all those places to use this.
(WorkingCopy.GetPropDiffs): Add wrapper around svn.wc.get_prop_diffs .

* lib/gvn/subcommands/describe.py
* lib/gvn/subcommands/mail.py
Adapt to Description.Output needing a pool.

* lib/gvn/subcommands/diff.py
Adapt --from-change to new Diff stuff.

* lib/gvn/subcommands/review.py
- Oops, need to use an iterpool!
- Adapt to new Diff stuff.
- Leave TODO about letting user's specify custom diff commands.
- Take --extensions option.

* setup.py
(setup): Add gvn/third_party/simplejson .

* testing/test_changebranch.py
Revamp and expand Diff testing; should cover a pretty good bit now.

Affected Paths:
M //trunk/doc/design.html
M //trunk/lib/gvn/changebranch.py
M //trunk/lib/gvn/cmdline.py
M //trunk/lib/gvn/description.py
M //trunk/lib/gvn/diff.py
M //trunk/lib/gvn/errors.py
M //trunk/lib/gvn/subcommands/describe.py
M //trunk/lib/gvn/subcommands/diff.py
M //trunk/lib/gvn/subcommands/mail.py
M //trunk/lib/gvn/subcommands/review.py
R //trunk/lib/gvn/util.py
M //trunk/lib/gvn/wc.py
M //trunk/setup.py
M //trunk/testing/test_changebranch.py

This is a semiautomated message from "gvn mail". See
<http://code.google.com/p/gvn/> to learn more.

Index: doc/design.html
===================================================================
--- doc/design.html (^/trunk/doc/design.html@910)
+++ doc/design.html (^/changes/epg/cb2.0-compat/trunk/doc/design.html@933)
@@ -56,7 +56,7 @@ function highlight(name) {


Status: Draft  
- (as of 2007-11-27)
+ (as of 2008-05-14)


<address>
@@ -171,6 +171,26 @@ Google engineer not working on the project.
If you're feeling masochistic, see <a href="#appendixA">Appendix A:
Where We've Been</a>.

+TODO(epg): document properties:</p?
+<ul>
+ <li>revprops
+ <ul>
+ <li>gvn:approve* (on snapshots)</li>
+ <li>gvn:submitted (on snapshots)</li>
+ <li>gvn:block-this-commit (on txn)</li>
+ <li>gvn:bypass-hooks (on submits)</li>
+ <li>gvn:change (on submits)</li>
+ <li>gvn:bug (on snapshots and submits)</li>
+ </ul>
+ </li>
+ <li>node props
+ <ul>
+ <li>gvn:project (on project roots)</li>
+ <li>gvn:superusers (on repository root)</li>
+ </ul>
+ </li>
+</ul>
+
<h4>Where We Are</h4>

A changebranch is an entry in a project's changes directory in the
@@ -255,54 +275,26 @@ change without sending the change itself until fin
Additionally, we have the problems described in "Where We Are". We
can solve all these problems with one change to the model: when
snapshotting a changebranch, store our own copy of the path change
-metadata, and only re-branch files as needed. We store this as the
-gvn:state property on the changebranch container
-(e.g. <code>//changes/epg/foo</code>).
+metadata, and only re-branch files as needed. We store this as a file
+called <code>state</code> in the changebranch container (e.g.
+<code>//changes/epg/foo/state</code>). This <code>state</code> file
+is a <a href="http://www.json.org/">JSON</a> serialization of the
+new <code>gvn.changebranch.State</code> class.

-We already have the <code>ChangeState</code> class, we just need to
-add {change,source}_{path,rev} attributes and
-serialize it to XML (we'll store the same format in
-the <code>.changebranch</code> file in the wc (replacing
-<code>.gvnstate</code>)):
-
-<pre class="prettyprint">
-<?xml version="1.0"?>
-<changebranch version="0.1">
- <node path="README">
- <change-name>foo</change-name> 
- 
- <source-path>e.g. trunk/README</source-path>
- <source-rev>%d</source-rev>
- 
- <change-path>e.g. changes/epg/foobar/trunk/README</change-path>
- <change-rev>%d</change-rev>
- <kind>file | dir</kind>
- <copyfrom-url>%s</copyfrom-url> ?
- <copyfrom-rev>%d</copyfrom-rev> ?
- <has-prop-mods/> ?
- <text-status>A | D | R | M | C</text-status>
- <prop-status>A | D | M | C</prop-status>
- <checksum>%s</checksum>
- <mtime>%d</mtime> ?
- <size>%d</size> ?
- </node>
-</changebranch>
-</pre>
-
For the class of changes we heuristically decide are "big" (or
-maybe the user has to use an option), we just set old and new to None
-for all files and don't create the "branch" part of the changebranch;
-we create <code>//changes/epg/foo</code> but
-not <code>//changes/epg/trunk</code> . <code>gvn review</code> can
+maybe the user has to use an option), we just don't create the
+<code>branch</code> part of the changebranch; we create
+<code>//changes/epg/foo/state</code> but not
+<code>//changes/epg/foo/branch</code> . <code>gvn review</code> can
still show the changed files and how they were changed (and even where
things are being copied from, e.g. <code>svn mv gigantic
src/gigantic</code>).

Now, for changes where we do branch
-to <code>//changes/epg/foo/trunk</code> , we do a full changebranch on
-the first snapshot. On subsequent snapshots,
+to <code>//changes/epg/foo/branch</code> , we do a full snapshot only
+at changebranch creation. On subsequent snapshots,
unless --force is specified, we don't
-changebranch files unless NeedsSnapshot.
+snapshot files unless NeedsSnapshot.
E.g.

<pre>
@@ -318,12 +310,14 @@ Sending testing/testcommon.py
Changed epg/foo@17.

=> same as today, copied //trunk@15 and applied diffs
- A /changes/epg/foo/trunk (from:/trunk@15)
- R /changes/epg/foo/trunk/Makefile (from:/trunk/Makefile@16)
- M /changes/epg/foo/trunk/testing/testcommon.py
+ A /changes/epg/foo
+ A /changes/epg/foo/state
+ A /changes/epg/foo/branch (from:/trunk@15)
+ R /changes/epg/foo/branch/Makefile (from:/trunk/Makefile@16)
+ M /changes/epg/foo/branch/testing/testcommon.py

- but review looks at Foo.action from the dict on
- //changes/epg/foo and so prints an M not an R.
+ but review looks at the path's action in the State object and
+ prints an M not an R.

wc% gvn opened
--- foo
@@ -334,18 +328,31 @@ wc% gvn change -c foo --non-interactive -m 'new lo
Changed epg/foo@18.
</pre>

+The state file looks like:
+
+<pre class="prettyprint">
+{"base": "trunk", "base_rev": 15, "paths": {
+ "Makefile": {
+ "action": "M",
+ "base_rev": 16
+ },
+ "testing/testcommon.py": {
+ "action": "M"
+ }
+ }
+}
+</pre>
+
+
<code>gvn</code> notices that
nothing NeedsSnapshot, and just tries to
change the <code>svn:log</code> property of the last snapshot. If
-that fails, it tries a no-op change, like this:
+that fails, it commits a change just to <code>state</code>, updating
+all the <code>snap</code> entries to the last snap revision. Hmm, or
+maybe it always does that, rather than trying to change the log.

-<pre>
- R /changes/epg/foo/trunk (from:/changes/epg/foo/trunk@17)
-</pre>
-
-The Foo objects haven't changed, so we
-didn't have to re-send the
-dict, and <code>gvn review</code> has no
+The State objects haven't changed, so we
+didn't have to re-send them and <code>gvn review</code> has no
trouble showing the correct information and diffs. We didn't have to
send any diffs, we're just making a new revision with a
new <code>svn:log</code> .
@@ -360,28 +367,48 @@ Sending Makefile
Changed epg/foo@19.

=> notices only Makefile needs snapshot, so does this:
- A /changes/epg/foo/trunk (from:/trunk@15)
- R /changes/epg/foo/trunk/Makefile (from:/trunk/Makefile@16)
- R /changes/epg/foo/trunk/testing/testcommon.py (from:/changes/epg/foo/trunk/testing/testcommon.py@17)
+ M /changes/epg/foo/state
+ R /changes/epg/foo/branch/Makefile (from:/trunk/Makefile@16)
</pre>

-<code>gvn</code> only had to send a new delta for Makefile; it did
-not have to send anything for testcommon.py or for the
-Foo dict. If,
-however, the user changebranches a new file, we have to upload the
-dict again.
+<code>gvn</code> only had to send a new delta for Makefile and set
+the snap revisions for everything else to 18:

-Even sending this dict could be expensive
-on really large changes, so we could make even further an
-optimization. If the user just adds one more file to such a branch,
-should we send the whole dict again? If we
-store the dict textually (I guess we should
-use XML for ease of use from multiple languages), we can send just a
-delta even for this metadata. This is much more involved than all the
-above, as we either have to keep a non-recursive wc
-of <code>//changes/epg/foo</code> squirrelled away or write our own
-delta code; but we could go here if we needed to, one day.
+<pre class="prettyprint">
+{"base": "trunk", "base_rev": 15, "paths": {
+ "Makefile": {
+ "action": "M",
+ "base_rev": 16
+ },
+ "testing/testcommon.py": {
+ "action": "M",
+ "snap_rev": 18
+ }
+ }
+}
+</pre>

+If some path had been copied:
+
+<pre class="prettyprint">
+{"base": "trunk", "base_rev": 15, "paths": {
+ "Makefile": {
+ "action": "M",
+ "base_rev": 16
+ },
+ "testing/testcommon.py": {
+ "action": "M",
+ "snap_rev": 18
+ },
+ "copy-example": {
+ "action": "M",
+ "copyfrom_path": "/trunk/foo",
+ "copyfrom_rev": 14
+ }
+ }
+}
+</pre>
+
<h3>Commands</h3>

<h4>checkout (co)</h4>
Index: lib/gvn/changebranch.py
===================================================================
--- lib/gvn/changebranch.py (^/trunk/lib/gvn/changebranch.py@910)
+++ lib/gvn/changebranch.py (^/changes/epg/cb2.0-compat/trunk/lib/gvn/changebranch.py@933)
@@ -24,6 +24,7 @@ Diff -- diff a change branch

"""

+import cStringIO
import os
import posixpath
import random
@@ -31,16 +32,17 @@ import re
import shutil
import subprocess
import sys
-import tempfile

from errno import EEXIST

+import svn.diff
import svn.ra
import svn.wc

from svn.core import SVN_DIRENT_CREATED_REV, SVN_DIRENT_KIND
from svn.core import SWIG_SVN_INVALID_REVNUM as SVN_INVALID_REVNUM
-from svn.core import svn_node_dir, svn_node_none
+from svn.core import svn_node_dir, svn_node_file, svn_node_none, svn_node_unknown
+from svn.core import svn_path_local_style
from svn.core import svn_prop_regular_kind, svn_property_kind

import gvn.commit
@@ -51,10 +53,138 @@ import gvn.svncmd
import gvn.util
import gvn.wc

+try:
+ import simplejson
+except ImportError:
+ from gvn.third_party import simplejson

-_DIFF_COMMAND = 'diff'

+def IsRegularProp(name):
+ (kind, prefix_len) = svn.core.svn_property_kind(name)
+ return kind == svn.core.svn_prop_regular_kind

+def FilterRegularProps(properties):
+ """Return copy of properties with special svn properties removed."""
+ return dict((name, value) for (name, value) in properties.iteritems()
+ if IsRegularProp(name))
+
+
+_ADD_CODES =['A', 'R']
+
+class State(object):
+ """state of a changebranch snapshot
+
+ Attributes:
+ base_path, base_revision -- path/revision snapshot is based on
+ paths -- dict mapping unicode path to PathState
+ """
+ def __init__(self, base_path, base_revision):
+ self.base_path = base_path
+ self.base_revision = base_revision
+ self.paths = {}
+
+
+class PathState(object):
+ """state of a path in a changebranch snapshot
+
+ Note: kind is always svn_node_unknown for old-style changebranches,
+ which are going away soon.
+
+ Attributes:
+ action -- 'A'dd, 'D'elete, 'R'eplace, 'M'odify
+ base_revision -- revision of working copy item this represents
+ snap_revision -- revision of last snapshot
+ copyfrom_path -- path copied from or None
+ copyfrom_revision -- revision copied from or SVN_INVALID_REVNUM
+ kind -- svn_node_kind_t
+ """
+ def __init__(self, action, base_revision, snap_revision,
+ copyfrom_path, copyfrom_revision, kind):
+ self.action = action
+ self.base_revision = base_revision
+ self.snap_revision = snap_revision
+ self.copyfrom_path = copyfrom_path
+ self.copyfrom_revision = copyfrom_revision
+ self.kind = kind
+
+ def __eq__(self, other):
+ return (self.action == other.action
+ and self.base_revision == other.base_revision
+ and self.snap_revision == other.snap_revision
+ and self.copyfrom_path == other.copyfrom_path
+ and self.copyfrom_revision == other.copyfrom_revision
+ and self.kind == other.kind)
+ def __ne__(self, other):
+ return not self == other
+
+
+def _ParseStateKind(state_dict):
+ """Parse path kind from State dict (helper for ParseState)."""
+ if state_dict.get('kind') in [None, 'file']:
+ return svn_node_file
+ return svn_node_dir
+
+def ParseState(state_utf8, snap_revision):
+ """Parse state_utf8 at snap_revision.
+
+ Arguments:
+ state_utf8 -- utf8-encoded str from the repository state file
+ snap_revision -- revision of the snapshot being examined
+
+ Raises:
+ gvn.errors.ParseState
+ """
+ # Decode the UTF-8 text.
+ try:
+ decoded = state_utf8.decode('utf8')
+ except UnicodeDecodeError, e:
+ raise gvn.errors.ParseState(child=gvn.errors.Encoding(e))
+ # Parse the dict from the decoded text.
+ try:
+ parsed = simplejson.loads(decoded)
+ except ValueError, e:
+ raise gvn.errors.ParseState(child=e)
+ # Check the format.
+ try:
+ format = parsed['format']
+ except KeyError:
+ # no format => format 0
+ format = 0
+ if format != 0:
+ # which is all we support
+ raise gvn.errors.ParseState(message='unknown format %d' % (format,))
+ # Get base path and revision.
+ try:
+ base_path = parsed['base']
+ except KeyError:
+ raise gvn.errors.ParseState(message='missing base path')
+ try:
+ base_revision = parsed['base_rev']
+ except KeyError:
+ raise gvn.errors.ParseState(message='missing base revision')
+
+ change_state = State(base_path, base_revision)
+ # Get path state.
+ try:
+ path_state = parsed['paths']
+ except KeyError:
+ raise gvn.errors.ParseState(message='missing paths')
+ for (path, state) in path_state.iteritems():
+ try:
+ action = state['action']
+ except KeyError:
+ raise gvn.errors.ParseState(message="missing action for '%s'" % (path,))
+ # base, snap, copyfrom_path/rev, and kind may be missing.
+ change_state.paths[path] = PathState(action,
+ state.get('base', base_revision),
+ state.get('snap', snap_revision),
+ state.get('copyfrom_path'),
+ state.get('copyfrom_rev',
+ SVN_INVALID_REVNUM),
+ _ParseStateKind(state))
+ return change_state
+
+
class _WCBranch(gvn.wc.Edit):
"""Apply a change from a working copy to a change branch."""

@@ -137,6 +267,8 @@ class PostfixTextDeltaManager(gvn.wc.PostfixTextDe
os.unlink(tmp)

+# TODO(next-release): The new changebranch creation code fixes all these
+# TODOs about self.revision.
class ChangeBranch(object):
"""Representation of a change branch."""

@@ -171,6 +303,11 @@ class ChangeBranch(object):
self._description = None
self._revisions = []

+ # TODO(next-release): Remove; this is just to support old-style
+ # changebranches during the transition; see also _GetChangedPathsHelper
+ # and BranchPath methods.
+ self._branch_name = 'branch'
+
name = property(lambda self: self._name)
username = property(lambda self: self._username)
change_name_at_head = property(lambda self: '/'.join([self.username,
@@ -179,6 +316,22 @@ class ChangeBranch(object):
self.name]),
str(self.revision.number)]))

+ def BranchPath(self, path=None):
+ """Return root-relative path of (or within) the branch.
+
+ If path is None return the branch path itself, otherwise return the
+ root-relative path of path within the branch.
+ """
+ if path is None:
+ path = self._branch_name
+ else:
+ path = self._branch_name + '/' + path
+ return self.project.ChangeBranchPath(self.username, self.name, path)
+
+ def StatePath(self):
+ """Return root-relative path of the state file."""
+ return self.project.ChangeBranchPath(self.username, self.name, 'state')
+
def _GetRevision(self):
if self._revision is None:
dirent = self.project.repository.Stat(
@@ -199,11 +352,8 @@ class ChangeBranch(object):
return self._description
description = property(_GetDescription)

- def _GetChangedPaths(self):
- try:
- revision = self.revision
- except gvn.errors.RepoPath:
- return []
+ def _GetChangedPathsHelper(self):
+ revision = self.revision

cbp = self.project.ChangeBranchPath(self.username, self.name)

@@ -213,17 +363,17 @@ class ChangeBranch(object):
end=1, limit=1)

changes = []
- topname = None
+ branch_name = None
for i in sorted(revision.paths, key=lambda x: x.path):
# When we start through here, we don't know the "top name"
# ('trunk' for '//trunk', 'projects' for '//public/projects').
- if topname is None:
+ if branch_name is None:
(dirname, basename) = posixpath.split(i.path.lstrip('/'))
if dirname == cbp:
- # Found the branch; save topname, path, and copyfrom.
- topname = basename
+ # Found the branch; save branch_name, path, and copyfrom.
+ self._branch_name = branch_name = basename
cbp = self.project.ChangeBranchPath(self.username, self.name,
- topname)
+ branch_name)
source = i.copyfrom_path
source_revision = i.copyfrom_revision
continue
@@ -239,10 +389,64 @@ class ChangeBranch(object):
i.copyfrom_path, i.copyfrom_revision,
relative_path, source,
source_revision))
+ return (source, source_revision, changes)

- return changes
+ def _GetChangedPaths(self):
+ # TODO(next-release): Preserving this crapitude for just one more...
+ try:
+ return self._GetChangedPathsHelper()[2]
+ except gvn.errors.RepoPath:
+ return []
changed_paths = property(_GetChangedPaths, doc="""List of ChangedPaths.""")

+ def _OldGetRepoState(self):
+ (source, source_revision, changes) = self._GetChangedPathsHelper()
+ state = State(source.lstrip('/'), source_revision)
+ for cp in changes:
+ relative_path = cp.relative_path.lstrip('/')
+ if cp.copyfrom_path is None:
+ copyfrom_path = None
+ else:
+ copyfrom_path = cp.copyfrom_path.lstrip('/')
+ state.paths[relative_path] = PathState(cp.action, cp.old_revision,
+ self.revision.number,
+ copyfrom_path,
+ cp.copyfrom_revision,
+ svn_node_unknown)
+ return state
+
+ def _NewGetRepoState(self, pool):
+ """Return State object parsed from repository state file.
+
+ Will replace GetRepoState in next release.
+
+ Raises:
+ SubversionException.apr_err in gvn.errors.SVN_NOENT
+ SubversionException.apr_err==???
+ gvn.errors.ParseState
+ """
+ sio = cStringIO.StringIO()
+ svn.ra.get_file(self.project.repository.ra,
+ self.StatePath(), self.revision.number,
+ sio, pool)
+ return ParseState(sio.getvalue(), self.revision)
+
+ def GetRepoState(self, pool):
+ """Return State object from old or new style changebranch.
+
+ First try _NewGetRepoState, but if the state file does not exist, try
+ _OldGetRepoState.
+
+ Will be replaced by _NewGetRepoState in next release.
+ """
+ try:
+ return self._NewGetRepoState(pool)
+ except svn.core.SubversionException, e:
+ if e.apr_err not in gvn.errors.SVN_NOENT:
+ raise
+ # No state path; try old-style changebranch.
+ return self._OldGetRepoState()
+
def ChangedPathsRelative(self):
"""Return iterator returning relative paths of changed_paths."""
return (x.relative_path for x in self.changed_paths)
@@ -673,336 +877,599 @@ def RandomBranchName(length=4):

return ''.join(branchname)

-######################################################################
+###########################################################################
+# changebranch diffing

-def _makedirs(filename):
- try:
- os.makedirs(filename)
- except OSError, e:
- if e.errno != EEXIST:
- raise
+# first, the callbacks

-def _GetFile(fn, repo, path, revision, prop_hack=False, pool=None):
- """Get path from repo at revision into local file fn.
+class DiffCallbacks(object):
+ """Base class for callbacks called by Diff functions."""
+ cb = None
+ repo_state = None

- Arguments:
- fn -- name of local file to write to
- repo -- Repository to get path from
- path -- path to get from repo
- revision -- revision of path to get
- prop_hack -- whether to append properties and values to the local file
- pool -- memory pool
- """
+ def SetChangeState(self, cb, state):
+ """Called with ChangeBranch and State objects before any other callback.

- path = path.lstrip('/')
- if repo.Stat(path, revision).kind == svn_node_dir:
- _makedirs(fn)
- return
+ Callback implementors should not need to implement this one, as this
+ implementation saves them as the cb and state attributes.
+ """
+ self.cb = cb
+ self.repo_state = state

- fp = open(fn, 'w')
- (revision, properties) = svn.ra.get_file(repo.ra, path, revision, fp, pool)
+ def Directory(self, path,
+ left_properties, right_properties, pool):
+ """Called for added or changed directories.

- if not prop_hack:
- # XXX temporary hack to see props diff, but only do it if we're in
- # diff source cb mode.
- fp.write('\n\nProperties:\n')
- for name in sorted(properties.iterkeys()):
- (kind, prefix_len) = svn_property_kind(name)
- if kind != svn_prop_regular_kind:
- continue
- fp.write('%s: %s\n' % (name, properties[name]))
+ Arguments:
+ path -- unicode path relative to the base path
+ left_properties -- properties for the left side
+ right_properties -- properties for the right side
+ pool -- scratch pool
+ """
+ pass

- fp.close()
+ def File(self, path, left_path, right_path,
+ left_properties, right_properties, pool):
+ """Called for added or changed files.

-def MakeLocalDiffTree(repo, cb, from_change_paths=None, wc=None, callback=None,
- tmpdir=None, pool=None):
- """Build two local trees for diffing; pass file pairs to callback.
+ Implementations may freely modify the two tmp files, but may not
+ delete them.

- To make this crap even messier, from_change_paths indicates that
- we're diffing cb => wc rather than source => cb; from_change_paths
- may be None or a list of paths to diff.
+ Arguments:
+ path -- unicode path relative to the base path
+ left_path -- name of local temporary file for the left side
+ right_path -- name of local temporary file for the right side
+ left_properties -- properties for the left side
+ right_properties -- properties for the right side
+ pool -- scratch pool
+ """
+ pass

- Arguments:
- repo -- Repository to get files from
- cb -- ChangeBranch whose files to get
- from_change_paths -- list of paths to diff from changebranch to local
- wc -- WorkingCopy; required if from_change_paths is not None
- callback -- callable(change, old, new) for each pair
- tmpdir -- directory to build trees in
- pool -- memory pool
- """
+ def Abort(self, pool):
+ """Called if something goes wrong before finishing the diff.

- for change in cb.changed_paths:
- if from_change_paths is not None:
- # diff cb wc mode
- if change.relative_path not in from_change_paths:
- # Caller didn't ask to diff this file.
- continue
+ Arguments:
+ pool -- scratch pool
+ """
+ pass

- old_tmp_path = posixpath.join(tmpdir, 'old', change.relative_path)
- old_tmpdir = posixpath.dirname(old_tmp_path)
- new_tmp_path = posixpath.join(tmpdir, 'new', change.relative_path)
- new_tmpdir = posixpath.dirname(new_tmp_path)
+ def Finish(self, pool):
+ """Called when the diff is finished.

- _makedirs(old_tmpdir)
- _makedirs(new_tmpdir)
+ Arguments:
+ pool -- scratch pool

- if from_change_paths is not None:
- if change.action == 'D':
- new_tmp_path = None
- else:
- # symlink or copy the wc path to be the new path
- if sys.platform == 'win32':
- shutil.copy(wc.AbsolutePath(change.relative_path), new_tmp_path)
- else:
- os.symlink(wc.AbsolutePath(change.relative_path), new_tmp_path)
- else:
- cbp = '/' + cb.project.ChangeBranchPath(cb.username, cb.name)
+ Returns:
+ Whatever the implementor wants (this implementation returns None); Diff
+ functions will return this.
+ """
+ return None

- if (change.action == 'A'
- # If it's a plain old Add...
- and (change.copyfrom_path is None
- # or a the copyfrom is from somewhere outside the
- # changebranch (probably merge of a plain old Add)
- or not change.old_path.startswith(cbp))):
- # Then treat it as an Added file, diffing from empty file.
- old_tmp_path = None
+
+class SvnDiffCallbacks(DiffCallbacks):
+ """DiffCallbacks implementation using svn's internal textual diff."""
+
+ def __init__(self, options, out_file, encoding, pool):
+ """Initialize from list, file-like, str, svn.core.Pool .
+
+ Arguments:
+ options -- list of option strings for svn.diff.file_options_parse
+ out_file -- file-like object to which to write the diff
+ encoding -- user's encoding
+ pool -- scratch memory pool; may be cleared after instantiation
+ """
+ self.options = svn.diff.file_options_create()
+ svn.diff.file_options_parse(self.options, options, pool)
+ self.fp = out_file
+ self.encoding = encoding
+
+ def _DiffProps(self, path, left_properties, right_properties, pool):
+ prop_diffs = svn.core.svn_prop_diffs(right_properties, left_properties,
+ pool)
+ if len(prop_diffs) == 0:
+ return
+ self.fp.write('\n'
+ 'Property changes on: %s\n'
+ '___________________________________________________________________\n'
+ % (path.encode(self.encoding),))
+ for (name, value) in prop_diffs.iteritems():
+ original = left_properties.get(name)
+ # TODO(glasser): Use svn.diff.mem_string stuff here.
+ if original is None:
+ self.fp.write('Added: %s\n'
+ ' + %s\n'
+ % (name.encode(self.encoding), value))
+ elif value is None:
+ self.fp.write('Deleted: %s\n'
+ ' - %s\n'
+ % (name.encode(self.encoding), original))
else:
- # Else, we have a copy, so show the difference between the source
- # and destination paths.
- try:
- _GetFile(old_tmp_path, repo, change.old_path,
- change.old_revision, from_change_paths is not None, pool)
- except gvn.errors.RepoPath:
- # Some parent of this path is in this change a copy, and now
- # we're deleting this path out of the copy: change.old_path
- # does not exist. XXX Should we do anything but ignore it?
- continue
+ self.fp.write('Modified: %s\n'
+ ' - %s\n'
+ ' + %s\n'
+ % (name.encode(self.encoding),
+ original, value))

- if from_change_paths is not None:
- cb_tmp_path = old_tmp_path
- else:
- cb_tmp_path = new_tmp_path
+ def Directory(self, path,
+ left_properties, right_properties,
+ left_header, right_header, pool):
+ # TODO(epg): Support --no-diff-deleted
+ no_diff_deleted = True
+ if (no_diff_deleted
+ and self.repo_state is not None
+ and self.repo_state.paths[path].action == 'D'):
+ return
+ self._DiffProps(path, left_properties, right_properties, pool)

- if change.action != 'D':
- _GetFile(cb_tmp_path, repo, change.path,
- change.revision, from_change_paths is not None, pool)
+ def File(self, path, left_path, right_path,
+ left_properties, right_properties,
+ left_header, right_header, pool):
+ # TODO(epg): Support --no-diff-deleted
+ no_diff_deleted = True
+ if (no_diff_deleted
+ and self.repo_state is not None
+ and self.repo_state.paths[path].action == 'D'):
+ maybe_deleted = ' (deleted)'
else:
- cb_tmp_path = None
+ maybe_deleted = ''
+ self.fp.write('Index: %s%s\n'
+ '===================================================================\n'
+ % (path.encode(self.encoding), maybe_deleted))
+ if (no_diff_deleted
+ and self.repo_state is not None
+ and self.repo_state.paths[path].action == 'D'):
+ return
+ diff = svn.diff.file_diff_2(left_path, right_path,
+ self.options, pool)
+ relative_to_dir = None
+ svn.diff.file_output_unified3(self.fp, diff,
+ left_path, right_path,
+ left_header, right_header,
+ self.encoding, relative_to_dir,
+ self.options.show_c_function, pool)
+ self._DiffProps(path, left_properties, right_properties, pool)

- if from_change_paths is not None:
- old_tmp_path = cb_tmp_path
- else:
- new_tmp_path = cb_tmp_path
- callback(change, old_tmp_path, new_tmp_path)

-def Diff(repo, cb, fp, diff_command, from_change_paths=None, wc=None,
- pool=None):
- """Diff source => cb (or cb => wc if from_changes_paths is not None).
+def _MaybeWriteProperties(properties, filename):
+ """Serialize properties into the file filename, if any properties.

- Arguments:
- repo -- Repository cb is in
- cb -- ChangeBranch to diff
- fp -- file-like object to write diff output to (only the
- write method must exist)
- diff_command -- 'internal', 'tkdiff', or a custom command to use to
- diff file pairs
- from_change_paths -- see MakeLocalDiffTree
- wc -- see MakeLocalDiffTree
- pool -- memory pool
+ Helper for TkDiffCallbacks.
"""
- if len(cb.changed_paths) == 0:
+ if len(properties) == 0:
return
+ fp = open(filename, mode='ab')
+ fp.write('\n\nProperties:\n')
+ for (name, value) in sorted(properties.iteritems(), key=lambda x: x[0]):
+ fp.write('%s: %s\n' % (name, value))
+ fp.close()

- if diff_command == 'internal':
- # default, 'internal', diff implementation
- # TODO(epg): Now that we do a separate diff per file, we can
- # easily do property diffs like svn does; none of the svn_diff
- # stuff is yet wrapped (that's why we run diff) so it will be yet
- # more imitation code...
- def callback(change, old, new):
- if change.action == 'D':
- return
+class TkDiffCallbacks(DiffCallbacks):
+ """DiffCallbacks implementation using tkdiff."""

- path = change.relative_path
- if old is None:
- old = os.devnull
- if new is None:
- new = os.devnull
+ def __init__(self, tkdiff, encoding):
+ """Initialize from str, str.

- fp.write("""Index: %s
-===================================================================
-""" % (path,))
+ Arguments:
+ tkdiff -- tkdiff command; passed to subprocess.call (without shell)
+ encoding -- user's encoding
+ """
+ self.tkdiff = tkdiff
+ self.encoding = encoding
+ self.pairs = []

- if change.action == 'A' and change.copyfrom_path is None:
- oldpath = change.path
- oldrevision = 0
- else:
- oldpath = change.old_path
- oldrevision = change.old_revision
- # TODO(epg): Use difflib until svn_diff is available?
- try:
- proc = subprocess.Popen([_DIFF_COMMAND, '-u',
- '-L', '%s (/%s@%d)' % (path, oldpath,
- oldrevision),
- '-L', '%s (/%s@%d)' % (path, change.path,
- change.revision),
- old, new], stdout=subprocess.PIPE)
- except OSError, e:
- if e.errno not in gvn.platform.ENOENT_codes:
- raise
- raise gvn.errors.NoDiffCommand
- for line in proc.stdout:
- # MSYS and/or Cygwin GNU diff put CRLF line endings on the
- # ends of diff lines, but we need LF only in internal data.
- fp.write(line.rstrip('\r\n') + '\n')
- else:
- file_pairs = []
- def callback(change, old, new):
- file_pairs.append((old, new))
+ def Directory(self, path,
+ left_properties, right_properties,
+ left_header, right_header, pool):
+ # TODO(epg): _MaybeWriteProperties in some parallel tree (so names match
+ # up) for directory prop diffs.
+ return

- tmpdir = None
- try:
- tmpdir = tempfile.mkdtemp(prefix='gvndiff-')
- MakeLocalDiffTree(repo, cb, from_change_paths, wc, callback, tmpdir, pool)
+ def File(self, path, left_path, right_path,
+ left_properties, right_properties,
+ left_header, right_header, pool):
+ self.pairs.append((left_path, right_path))
+ if len(svn.core.svn_prop_diffs(right_properties, left_properties,
+ pool)) == 0:
+ return
+ _MaybeWriteProperties(left_properties, left_path)
+ _MaybeWriteProperties(right_properties, right_path)

- if diff_command and os.path.basename(diff_command) == 'tkdiff':
- # Hacky tkdiff implementation.
- argv = []
- first = True
- for (old, new) in file_pairs:
- if not first:
- argv.append(':')
- else:
- first = False
+ def Finish(self, pool):
+ if len(self.pairs) == 0:
+ # Nothing actually changed, so don't run tkdiff.
+ return 0
+ argv = [self.tkdiff]
+ for (left, right) in self.pairs:
+ argv.append(':')
+ argv.append(left)
+ argv.append(right)
+ return subprocess.call(argv)

- if old is None:
- old = os.devnull
- argv.append(old)
+#################################################################
+# now, the Diff functions

- if new is None:
- new = os.devnull
- argv.append(new)
+# first, DiffBaseToSnapshot

- argv.insert(0, diff_command)
- subprocess.call(argv)
+def _FindLeft(path, base_revision, repo_state, copied_directory_stack):
+ """Return (root-relative path, revision, header) for the left side of a diff.

- elif diff_command and diff_command != 'internal':
- # We appear to have a custom diff_command.
- for (old, new) in file_pairs:
- argv = []
- if old is None:
- old = os.devnull
- argv.append(old)
+ In the simple, common case this is just

- if new is None:
- new = os.devnull
- argv.append(new)
+ (repo_state.base_path + path, base_revision)

- argv.insert(0, diff_command)
- subprocess.call(argv)
- finally:
- if tmpdir is not None:
- gvn.platform.rmtree(tmpdir)
+ But if some parent directory of path was copied
+ (i.e. copied_directory_stack is not empty), we need to find the
+ left side under the path from which that copy was made.

-# TODO(epg): Use bindings. Extend Notify callbacks to handle
-# content_state and prop_state. Handle conflicts (i'm thinking we
-# want to allow conflicted files to be snapshotted without resolving;
-# submit already won't let them through...). It doesn't even check
-# svn exit codes.
+ The returned header is suitable for to svn.diff.file_output_unified3 and
+ similar functions.

-def _MergeAdd(change, _wc, pool):
- if change.copyfrom_path is None:
- # XXX Why isn't the .lstrip('/') required in _GetFile? Anywhere else?
- if _wc.project.repository.Stat(change.path.lstrip('/'),
- change.revision).KindIsDir():
- gvn.svncmd.RunSvn(['mkdir', '-q', change.relative_path])
- else:
- _GetFile(change.relative_path, _wc.project.repository,
- change.path, change.revision, pool)
- gvn.svncmd.RunSvn(['add', '-q', change.relative_path])
+ Arguments:
+ path -- base-relative path
+ base_revision -- what the caller thinks the base revision
+ of path is
+ repo_state -- State object
+ copied_directory_stack -- list of copied directories
+ (see DiffBaseToSnapshot)
+ """
+ try:
+ # Check for a copied directory.
+ (copied_dir, copyfrom_path, copyfrom_revision) = copied_directory_stack[-1]
+ except IndexError:
+ # Not under copied directory; use same path for left side.
+ left_path = '/'.join([repo_state.base_path, path])
+ left_revision = base_revision
else:
- gvn.svncmd.RunSvn(['cp', '-qr', str(change.copyfrom_revision),
- _wc.project.repository.URL(change.copyfrom_path),
- change.relative_path])
+ # Some parent directory was copied; find left side in there.
+ path_in_copied_dir = path[len(copied_dir) + 1:]
+ left_path = '/'.join([copyfrom_path, path_in_copied_dir])
+ left_revision = copyfrom_revision
+ # Now make the header.
+ if repo_state.paths[path].action in _ADD_CODES:
+ left_header = path + '\t(added)'
+ else:
+ left_header = ('%s\t(^/%s@%d)'
+ % (left_path[len(repo_state.base_path) + 1:],
+ left_path, left_revision))
+ return (left_path, left_revision, left_header)

-def _MergeRemove(change, _wc, pool):
- gvn.svncmd.RunSvn(['rm', '-q', change.relative_path])

-def _MergeReplace(change, _wc, pool):
- _MergeRemove(change, _wc, pool)
- _MergeAdd(change, _wc, pool)
+def _RaGetDirOrFile(ra, path, revision, fp, dirent_fields, pool,
+ kind=svn_node_unknown):
+ """Call get_file or get_dir for DiffBaseToSnapshot.

-def _MergeModify(change, _wc, pool):
- gvn.svncmd.RunSvn(['merge', '-q',
- '@'.join([_wc.project.repository.URL(change.old_path),
- str(change.old_revision)]),
- '@'.join([_wc.project.repository.URL(change.path),
- str(change.revision)]),
- change.relative_path])
+ This is for old-style changebranches, where we don't know whether path
+ is a directory or file. Arguments are a combination of svn.ra.get_file
+ and svn.ra.get_dir2 arguments, plus the optional kind, which we know
+ with new-style changebranches.
+ """
+ if kind == svn_node_file:
+ result = svn.ra.get_file(ra, path, revision, fp, pool)
+ return (kind, result)

-# TODO(epg): This isn't correct until (unless) we have a way to
-# understand what R actions in changebranches mean (i.e. user
-# modifies, not replaces, a file, but it's an R on the changebranch
-# because of out-of-date top). I haven't thought about that, but i
-# think we can do it. If not, this is going to require more work.
-# Workaround for now is to changbranch from up-to-date working copy.
+ if kind == svn_node_dir:
+ result = svn.ra.get_dir2(ra, path, revision, dirent_fields, pool)
+ return (kind, result)

-_MergeActions = {
- 'A': _MergeAdd,
- 'D': _MergeRemove,
- 'R': _MergeReplace,
- 'M': _MergeModify,
-}
+ gvn.DiagLog('_RaGetDirOrFile(%s@%d) => try get_file => ', path, revision)
+ try:
+ result = svn.ra.get_file(ra, path, revision, fp, pool)
+ kind = svn_node_file
+ gvn.DiagLog('is file')
+ except svn.core.SubversionException, e:
+ if e.apr_err not in [svn.core.SVN_ERR_FS_NOT_FILE,
+ # ra-neon screws us again; don't even ask
+ # about serf, which just wrote some HTML 301
+ # foo into fp! see svn issue 3206
+ svn.core.SVN_ERR_RA_DAV_RELOCATED]:
+ raise
+ gvn.DiagLog('get_dir2 => ')
+ result = svn.ra.get_dir2(ra, path, revision, dirent_fields, pool)
+ kind = svn_node_dir
+ gvn.DiagLog('is dir')
+ gvn.DiagLog('\n')
+ return (kind, result)

-def Merge(cb, project, _wc, subpath, config, pool):
- """Merge ChangeBranch cb into WorkingCopy _wc; return a new ChangeBranch.

- The Config and Pool objects config and pool are used to create the new
- ChangeBranch object, which is then snapshotted and returned. If
- the old ChangeBranch belongs to the user on whose behalf the caller
- is acting, return a new snapshotted ChangeBranch, but re-using the
- same underlying changebranch.
+def DiffBaseToSnapshot(cb, callbacks, encoding, pool):
+ """Diff the base of a snapshot to the snapshot itself.

+ This is 'gvn review'; it shows the change being proposed.
+
+ Arguments:
+ cb -- ChangeBranch object
+ callbacks -- DiffCallbacks implementation
+ encoding -- user's encoding
+ pool -- memory pool
+
+ Returns:
+ the return value of callbacks.Finish
"""
+ copied_directory_stack = []
+ tree = gvn.util.TmpTree()
+ try:
+ try:
+ change_state = cb.GetRepoState(pool)
+ callbacks.SetChangeState(cb, change_state)
+ iterpool = svn.core.Pool(pool)
+ for (path, state) in sorted(change_state.paths.iteritems(),
+ key=lambda x: x[0]):
+ iterpool.clear()
+ # See if we just came out from under a copied directory.
+ try:
+ last_copied_dir = copied_directory_stack[-1]
+ except IndexError:
+ last_copied_dir = None
+ if (last_copied_dir is not None
+ and not gvn.util.IsChild(path, last_copied_dir[0])):
+ # Yep, so pop it off the stack.
+ copied_directory_stack.pop(-1)

- if len(cb.changed_paths) == 0:
- return cb
+ kind = state.kind
+ local_path = svn_path_local_style(path, iterpool).decode('utf8')
+ local_path_encoded = local_path.encode(encoding)

- _wc.Close()
+ # Get left path/revision/header now, as we need them in a few
+ # different blocks below.
+ (left_path, left_revision, left_header) = _FindLeft(
+ path, state.base_revision, change_state, copied_directory_stack)

- for change in cb.changed_paths:
+ # This is more complicated than it could be in order to support
+ # old-style changebranches (no repo state file), where we don't
+ # know what kind of path we have.
+
+ #################################################################
+ # left side
+
+ if kind != svn_node_dir:
+ # Create tmp file, possibly incorrectly for old-style.
+ fp = tree.MkFile(['left', local_path_encoded])
+ else:
+ fp = None
+
+ if state.action in _ADD_CODES:
+ if state.copyfrom_path is None:
+ # This is an added dir or file, so no left-side properties.
+ properties = {}
+ if kind == svn_node_file and state.action == 'R':
+ # This is a Replaced file that isn't copied; svn diffs
+ # these as Modified files, so let's do the same.
+ # TODO(next-release): Note that for old-style changebranches
+ # we don't yet know kind, so we never get here. That's OK,
+ # this can be a teensy bit broken until we kick the old
+ # style to the curb. Don't forget to adjust testDiff in
+ # test_changebranch.py
+ svn.ra.get_file(cb.project.repository.ra,
+ left_path, left_revision,
+ fp, iterpool)
+ else:
+ # This is a copied dir or file, so get properties of copyfrom
+ # for the left side.
+ (kind, result) = _RaGetDirOrFile(cb.project.repository.ra,
+ state.copyfrom_path.encode('utf8'),
+ state.copyfrom_revision,
+ fp, dirent_fields=0, kind=kind,
+ pool=iterpool)
+ # The properties come last in the result for both dir and file.
+ properties = result[-1]
+ # And push it onto the stack.
+ copied_directory_stack.append((path, state.copyfrom_path,
+ state.copyfrom_revision))
+
+ else:
+ # Modified or Deleted
+ # TODO: Don't fetch if no_diff_deleted.
+ (kind, result) = _RaGetDirOrFile(cb.project.repository.ra,
+ left_path, left_revision,
+ fp, dirent_fields=0, kind=kind,
+ pool=iterpool)
+ if kind == svn_node_file:
+ (unused_rev, properties) = result
+ else:
+ (unused_dirents, unused_rev, properties) = result
+ # Note whether we made a left tmp file; further down, we'll
+ # delete bogus left and right files for directories in
+ # old-style changebranches.
+ if fp is None:
+ made_left = False
+ else:
+ made_left = True
+ fp.close()
+ left_properties = FilterRegularProps(properties)
+
+ #################################################################
+ # left side
+
+ # You might think we definitely know the kind of this path by now
+ # even in the old style, but if this is an Add, we didn't do
+ # anything for the left side, so we still don't know.
+
+ branch_path = cb.BranchPath(path)
+ if kind != svn_node_dir:
+ # Create tmp file, possibly incorrectly for old-style.
+ fp = tree.MkFile(['right', local_path_encoded])
+ else:
+ fp = None
+ if state.action == 'D':
+ properties = {}
+ else:
+ # Added or Modified
+ branch_path_utf8 = branch_path.encode('utf8')
+ (kind, result) = _RaGetDirOrFile(cb.project.repository.ra,
+ branch_path_utf8, cb.revision.number,
+ fp, dirent_fields=0, kind=kind,
+ pool=iterpool)
+ if kind == svn_node_file:
+ (unused_rev, properties) = result
+ else:
+ (unused_dirents, unused_rev, properties) = result
+ # Note whether we made a bogus tmp file; further down, we'll
+ # delete bogus left and right files for directories in
+ # old-style changebranches.
+ if fp is None:
+ made_right = False
+ else:
+ made_right = True
+ fp.close()
+ right_properties = FilterRegularProps(properties)
+
+ # And here it is: if this was a directory, delete any files we
+ # created incorrectly.
+ if kind == svn_node_dir:
+ if made_left:
+ tree.RmFile(['left', local_path_encoded])
+ if made_right:
+ tree.RmFile(['right', local_path_encoded])
+
+ #################################################################
+ # Now diff the left and right sides.
+
+ right_header = ('%s\t(^/%s@%d)'
+ % (path, branch_path,
+ cb.revision.number))
+ if kind == svn_node_file:
+ callbacks.File(path,
+ tree.Path(['left', local_path_encoded]),
+ tree.Path(['right', local_path_encoded]),
+ left_properties, right_properties,
+ left_header, right_header,
+ iterpool)
+ else:
+ callbacks.Directory(path,
+ left_properties, right_properties,
+ left_header, right_header, iterpool)
+ iterpool.destroy()
+ except:
+ callbacks.Abort(pool)
+ raise
+ else:
+ return callbacks.Finish(pool)
+ finally:
+ tree.Close()
+
+
+def DiffSnapshotToWC(cb, wc, callbacks, encoding, pool):
+ """Diff a snapshot to the working copy.
+
+ This is used to show what the user has changed since last snapshot.
+
+ Arguments:
+ cb -- ChangeBranch object
+ callbacks -- DiffCallbacks implementation
+ encoding -- user's encoding
+ pool -- memory pool
+
+ Returns:
+ the return value of callbacks.Finish
+ """
+ # TODO(next-release): This is easier in upcoming .gvnstate format.
+ paths = dict((path, state) for (path, state) in wc.change_state.iteritems()
+ if state.change_name == cb.name)
+ wc.Open(paths)
+ # TODO(next-release): Remove; this is only here to support old-style
+ # changebranches, by finding the branch name (for BranchPath).
+ cb._GetChangedPathsHelper()
+ tree = gvn.util.TmpTree()
+ try:
try:
- change_name = _wc.change_state[change.relative_path].change_name
- except KeyError:
- continue
- raise gvn.errors.ChangeBranch('%s already in %s'
- % (change.relative_path, change_name))
+ # Shouldn't status callback receive a pool, like most callbacks?
+ # Fine, we'll handle it ourselves.
+ status_pool = svn.core.Pool(pool)
+ def status_cb(target, status):
+ status_pool.clear()
+ path = wc.RelativePath(target)
+ if not wc.NeedsSnapshot(path, status, status_pool):
+ return

- changed_paths = []
- iterpool = svn.core.Pool(pool)
- for change in cb.changed_paths:
- iterpool.clear()
- _MergeActions[change.action](change, _wc, iterpool)
- if subpath == '':
- changed_paths.append(change.relative_path)
+ branch_path = cb.BranchPath(path)
+ branch_path_utf8 = branch_path.encode('utf8')
+
+ left_header = ('%s\t(^/%s@%d)' % (path, branch_path,
+ cb.revision.number))
+ right_header = path + '\t(working copy)'
+
+ if status.entry.kind == svn_node_dir:
+ (unused_dirents, unused_rev,
+ properties) = svn.ra.get_dir2(cb.project.repository.ra,
+ branch_path_utf8,
+ cb.revision.number,
+ 0, # dirent_fields
+ status_pool)
+ properties = FilterRegularProps(properties)
+ (local_prop_diffs, local_props) = wc.GetPropDiffs(target, status_pool)
+ gvn.util.ApplyPropDiffs(local_props, local_prop_diffs)
+ callbacks.Directory(path, properties, local_props,
+ left_header, right_header, status_pool)
+ else:
+ local_path = svn_path_local_style(path, status_pool).decode('utf8')
+ local_path_encoded = local_path.encode(encoding)
+
+ fp = tree.MkFile(['left', local_path_encoded])
+ (unused_rev, properties) = svn.ra.get_file(
+ cb.project.repository.ra, branch_path_utf8,
+ cb.revision.number, fp, status_pool)
+ fp.close()
+ properties = FilterRegularProps(properties)
+ right_path = svn.wc.translated_file2(target.encode('utf8'),
+ target.encode('utf8'),
+ wc.AdmRetrieve(target,
+ status_pool),
+ svn.wc.TRANSLATE_FROM_NF,
+ status_pool)
+ (local_prop_diffs, local_props) = wc.GetPropDiffs(target, status_pool)
+ gvn.util.ApplyPropDiffs(local_props, local_prop_diffs)
+ callbacks.File(path,
+ tree.Path(['left', local_path_encoded]), right_path,
+ properties, local_props,
+ left_header, right_header,
+ status_pool)
+ wc.Status(paths.keys(),
+ recursive=False, get_all=False, no_ignore=False,
+ show_children=False, callback=status_cb)
+ status_pool.destroy()
+ wc.Close()
+ except:
+ callbacks.Abort(pool)
+ raise
else:
- changed_paths.append('/'.join([subpath, change.relative_path]))
- iterpool.destroy()
+ return callbacks.Finish(pool)
+ finally:
+ tree.Close()

- if cb.username == _wc.project_config.username:
- # This is the calling user's own changebranch; reuse it.
- change_name = cb.name
- else:
- # This is someone else's changebranch; create one with the same
- # name (or uniquified, but close).
- change_name = UniqueChangeName(project, _wc.project_config.username, cb.name,
- pool)
- description = cb.description

- # TODO(epg): Pass our list of paths so it can open deeply.
- _wc.Open()
- cb = ChangeBranch(config, _wc.project, change_name)
- cb.Branch(_wc, changed_paths, description, pool)
+def DiffSnapshots():
+ pass
+ # Similar to DiffFromChange; the only way we can do this is by
+ # fetching every file and comparing them. In the common case, a
+ # URL-URL diff would work, but when it doesn't, there's nothing we
+ # can do in the report that will give us the behavior we need.

- return cb
+ # The way it would work in the common case is say:
+ # want diff between branch@20 -> branch@30
+ # report change state, telling it *what versions of branch@20 we have*
+ # receive deltas telling us how to turn that into branch@30
+
+ # That's no good:
+
+ # r20
+ # A /changes/epg/foo/branch from trunk@17
+ # M /changes/epg/foo/branch/a
+ # R /changes/epg/foo/branch/b from trunk@19
+ # =>
+ # epg/foo@20
+ # M a
+ # M b
+
+ # r30
+ # R /changes/epg/foo/branch from trunk@28
+ # M /changes/epg/foo/branch/a
+ # M /changes/epg/foo/branch/b
+ # =>
+ # epg/foo@30
+ # M a
+ # M b
+
+ # But there are files other than a and b! And they changed between
+ # 17 and 28. So, for example 'c': we'll receive a delta turning
+ # trunk/c@17 into trunk/c@28 which is no good at all. In large
+ # trees, we could receive quite a lot of crap we don't want.
Index: lib/gvn/cmdline.py
===================================================================
--- lib/gvn/cmdline.py (^/trunk/lib/gvn/cmdline.py@910)
+++ lib/gvn/cmdline.py (^/changes/epg/cb2.0-compat/trunk/lib/gvn/cmdline.py@933)
@@ -295,6 +295,13 @@ class OptionParser(optparse.OptionParser):
(values, argv) = optparse.OptionParser.parse_args(self, *args, **kwargs)
if not hasattr(values, 'gvn_options'):
values.gvn_options = []
+ # Post-process --extensions/-x option.
+ # With multiple -x options, the last wins rather than appending;
+ # this is how svn behaves.
+ if values.extensions is None:
+ values.extensions = []
+ else:
+ values.extensions = values.extensions.split()
return (values, argv)

Index: lib/gvn/description.py
===================================================================
--- lib/gvn/description.py (^/trunk/lib/gvn/description.py@910)
+++ lib/gvn/description.py (^/changes/epg/cb2.0-compat/trunk/lib/gvn/description.py@933)
@@ -13,13 +13,18 @@
# limitations under the License.

+import cStringIO
import datetime
import time

import gvn.changebranch
import gvn.util

+import svn.core

+from svn.core import SWIG_SVN_INVALID_REVNUM as SVN_INVALID_REVNUM
+
+
class RevisionDescriptionImpl(object):
def __init__(self, config, project, revnum):
# May raise errrors.NonExistentRevision; let it go.
@@ -40,25 +45,29 @@ class RevisionDescriptionImpl(object):
def Description(self):
return self._revision['svn:log']

- def AffectedPaths(self):
+ def AffectedPaths(self, pool):
return [(x.path, x.action) for x in self._revision.paths]

+# TODO(epg): Replace all this jazz with a gvn.changebranch.DiffCallbacks
+# class that prints the summary info before showing diff or running tkdiff.
class ChangeBranchDescriptionImpl(object):
def __init__(self, config, project, change_name):
+ self._project = project
(username, name, revision) = gvn.util.ParseChangeName(change_name)

- self._cb = gvn.changebranch.ChangeBranch(config, project, name, username,
- revision)
+ self._cb = gvn.changebranch.ChangeBranch(config, self._project,
+ name, username, revision)
+ (self._revision,) = self._project.repository.GetRevisions(start=self._cb.revision.number)

def Revision(self):
- return self._cb.revision
+ return self._revision

def Username(self):
return self._cb.username

def Date(self):
- return self._cb.revision['svn:date']
+ return self._revision['svn:date']

def ChangeBranchName(self):
return self._cb.change_name
@@ -66,9 +75,11 @@ class ChangeBranchDescriptionImpl(object):
def Description(self):
return self._cb.description

- def AffectedPaths(self):
- for i in self._cb.changed_paths:
- yield (i.source_path, i.action)
+ def AffectedPaths(self, pool):
+ change_state = self._cb.GetRepoState(pool)
+ for (path, state) in sorted(change_state.paths.iteritems(),
+ key=lambda x: x[0]):
+ yield ('/' + '/'.join([change_state.base_path, path]), state.action)

class Description(object):
@@ -129,14 +140,14 @@ class Description(object):
return ''
return log

- def Output(self):
+ def Output(self, pool):
rev = "r%s" % self._impl.Revision()
if self._impl.ChangeBranchName():
rev = '*' + self._impl.ChangeBranchName()

header = ' | '.join([rev, self.Author(), self.Date()])

- affected_paths = self._impl.AffectedPaths()
+ affected_paths = self._impl.AffectedPaths(pool)
affected_path_string = '\n'.join([" %s /%s" % (a, p)
for (p, a) in affected_paths])
return '\n'.join([
Index: lib/gvn/diff.py
===================================================================
--- lib/gvn/diff.py (^/trunk/lib/gvn/diff.py@910)
+++ lib/gvn/diff.py (^/changes/epg/cb2.0-compat/trunk/lib/gvn/diff.py@933)
@@ -64,7 +64,13 @@ def GetShortDiff(project, cb, pool):
raise truncate
fp = DiffReader()
try:
- gvn.changebranch.Diff(project.repository, cb, fp, 'internal', pool=pool)
+ # TODO(epg): I think only gvn mail uses this, but it's still dumb
+ # for this to assume what the caller wants.
+ options = ['-pu']
+ encoding = 'utf8'
+ callbacks = gvn.changebranch.SvnDiffCallbacks(options, fp,
+ encoding, pool)
+ gvn.changebranch.DiffBaseToSnapshot(cb, callbacks, encoding, pool)
except truncate:
shortdiff.append('\n*** TRUNCATED TO %d lines ***\n'
% (project.diff_lines,))
Index: lib/gvn/errors.py
===================================================================
--- lib/gvn/errors.py (^/trunk/lib/gvn/errors.py@910)
+++ lib/gvn/errors.py (^/changes/epg/cb2.0-compat/trunk/lib/gvn/errors.py@933)
@@ -60,6 +60,12 @@ import svn.core
SVN_OUT_OF_DATE = [svn.core.SVN_ERR_FS_TXN_OUT_OF_DATE,
# different code with ra-dav, oddly enough
svn.core.SVN_ERR_FS_CONFLICT]
+# list of SubversionException.apr_err values which mean "no such
+# path"; TODO(epg): we do this all over the place rather than using
+# this, but it looks like the fix for this crap is going to make it
+# into 1.5.0 so track it all down and remove it.
+SVN_NOENT = [svn.core.SVN_ERR_FS_NOT_FOUND,
+ svn.core.SVN_ERR_RA_DAV_PATH_NOT_FOUND]

class Root(Exception):
@@ -198,12 +204,6 @@ class MixedPaths(User):
User.__init__(self, 'All must be absolute or relative')

-class NoDiffCommand(User):
- code = 17
- def __init__(self):
- User.__init__(self,
- 'No diff command found; see http://code.google.com/p/gvn/wiki/Install')
-
class OutOfDateParent(User):
"""Tried to changebranch a path whose parent is out-of-date.

@@ -388,3 +388,16 @@ class NotChangeBranched(Internal):
def __init__(self, path):
Internal.__init__(self, "'%s' not changebranched" % (path,))
self.path = path
+
+class ParseState(Internal):
+ code = 111
+ def __init__(self, child=None, message=None):
+ self.child = child
+ self.message = message
+ def __str__(self):
+ message = ['repository state of changebranch is invalid']
+ if self.message is not None:
+ message.append(self.message)
+ if self.child is not None:
+ message.append(str(self.child))
+ return ': '.join(message)
Index: lib/gvn/subcommands/describe.py
===================================================================
--- lib/gvn/subcommands/describe.py (^/trunk/lib/gvn/subcommands/describe.py@910)
+++ lib/gvn/subcommands/describe.py (^/changes/epg/cb2.0-compat/trunk/lib/gvn/subcommands/describe.py@933)
@@ -15,6 +15,8 @@

import sys

+import svn.core
+
import gvn.cmdline
import gvn.description
import gvn.errors
@@ -25,10 +27,10 @@ usage: describe CHANGE | REVISION
"""

-def TryDescribe(name, ctx):
+def TryDescribe(name, ctx, pool):
try:
desc = gvn.description.Description(ctx.config, ctx.project, name)
- print desc.Output()
+ print desc.Output(pool)
except gvn.errors.User, e:
ctx.Notify('%s\n' % (e,))
return e.code
@@ -39,11 +41,14 @@ usage: describe CHANGE | REVISION
def Handle_GvnDescribe(ctx):
ctx.wc_operands = False
rval = 0
+ iterpool = svn.core.Pool(ctx.pool)
for operand in ctx.operands:
- r = TryDescribe(operand, ctx)
+ iterpool.clear()
+ r = TryDescribe(operand, ctx, iterpool)
if r != 0:
# Use the last error code as the exit code.
rval = r
+ iterpool.destroy()

return rval

Index: lib/gvn/subcommands/diff.py
===================================================================
--- lib/gvn/subcommands/diff.py (^/trunk/lib/gvn/subcommands/diff.py@910)
+++ lib/gvn/subcommands/diff.py (^/changes/epg/cb2.0-compat/trunk/lib/gvn/subcommands/diff.py@933)
@@ -12,6 +12,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.

+import os
import sys

import svn.core
@@ -75,31 +76,17 @@ def Handle_GvnDiff(ctx):
(username, name, revision) = gvn.util.ParseChangeName(change)
cb = gvn.changebranch.ChangeBranch(ctx.config, ctx.project,
name, username, revision)
- relative_paths = list(cb.ChangedPathsRelative())
- ctx.wc.Open(relative_paths)
- # Shouldn't the callback receive a pool, like most callbacks?
- # Fine, we'll handle it ourselves.
- pool = svn.core.Pool(ctx.pool)
- def status_cb(target, status):
- pool.clear()
- target = ctx.wc.RelativePath(target)
- if ctx.wc.NeedsSnapshot(target, status, pool):
- targets.add(target)
- ctx.wc.Status(list(relative_paths),
- recursive=False, get_all=False, no_ignore=False,
- show_children=False, callback=status_cb)
+ if (ctx.config.diff_command is not None
+ and os.path.basename(ctx.config.diff_command) == 'tkdiff'):
+ callbacks = gvn.changebranch.TkDiffCallbacks(ctx.config.diff_command,
+ ctx.encoding)
+ else:
+ callbacks = gvn.changebranch.SvnDiffCallbacks(ctx.options.extensions,
+ sys.stdout, ctx.encoding,
+ ctx.pool)
+ return gvn.changebranch.DiffSnapshotToWC(cb, ctx.wc, callbacks,
+ ctx.encoding, ctx.pool)

- if not targets:
- # All paths on change are up-to-date.
- return 0
-
- gvn.changebranch.Diff(ctx.project.repository,
- cb,
- sys.stdout,
- ctx.config.diff_command,
- from_change_paths=targets, wc=ctx.wc, pool=ctx.pool)
- return 0
-
targets = set(ctx.operands)

######################################################################
Index: lib/gvn/subcommands/mail.py
===================================================================
--- lib/gvn/subcommands/mail.py (^/trunk/lib/gvn/subcommands/mail.py@910)
+++ lib/gvn/subcommands/mail.py (^/changes/epg/cb2.0-compat/trunk/lib/gvn/subcommands/mail.py@933)
@@ -156,7 +156,7 @@ def Handle_GvnMail(ctx):
ccaddrs = [AppendEmailDomain(pconfig, r) for r in ccrecipients]
desc_object = gvn.description.Description(ctx.config, project,
cb.change_name)
- cb_desc = desc_object.Output()
+ cb_desc = desc_object.Output(ctx.pool)
short_desc = desc_object.Log()
if len(short_desc) > 39:
short_desc = short_desc[:36] + '...'
Index: lib/gvn/subcommands/review.py
===================================================================
--- lib/gvn/subcommands/review.py (^/trunk/lib/gvn/subcommands/review.py@910)
+++ lib/gvn/subcommands/review.py (^/changes/epg/cb2.0-compat/trunk/lib/gvn/subcommands/review.py@933)
@@ -12,8 +12,11 @@
# See the License for the specific language governing permissions and
# limitations under the License.

+import os
import sys

+import svn.core
+
import gvn.changebranch
import gvn.cmdline
import gvn.description
@@ -28,29 +31,40 @@ usage: review CHANGE | REVISION

def Handle_GvnReview(ctx):
ctx.wc_operands = False
+ iterpool = svn.core.Pool(ctx.pool)
for reviewable in ctx.operands:
+ iterpool.clear()
if gvn.util.IsValidChangeName(reviewable):
# XXX Ugh, Description gets a ChangeBranch and then we get a
# second one here.
print gvn.description.Description(ctx.config, ctx.project,
- reviewable).Output()
+ reviewable).Output(iterpool)

(username, name, revision) = gvn.util.ParseChangeName(reviewable)
- gvn.changebranch.Diff(ctx.project.repository,
- gvn.changebranch.ChangeBranch(ctx.config,
- ctx.project,
- name,
- username,
- revision),
- sys.stdout,
- ctx.config.diff_command,
- pool=ctx.pool)
+ cb = gvn.changebranch.ChangeBranch(ctx.config,
+ ctx.project,
+ name,
+ username,
+ revision)

+ if ctx.config.diff_command in [None, 'internal']:
+ callbacks = gvn.changebranch.SvnDiffCallbacks(ctx.options.extensions,
+ sys.stdout, ctx.encoding,
+ iterpool)
+ elif os.path.basename(ctx.config.diff_command) == 'tkdiff':
+ callbacks = gvn.changebranch.TkDiffCallbacks(ctx.config.diff_command,
+ ctx.encoding)
+ else:
+ # TODO: Implement callbacks that run user's custom diff command.
+ callbacks = gvn.changebranch.UserDiffCallbacks(ctx.config.diff_command)
+ return gvn.changebranch.DiffBaseToSnapshot(cb, callbacks, ctx.encoding,
+ iterpool)
+
elif gvn.util.MatchRevision(reviewable):
if reviewable[0] == 'r':
reviewable = reviewable[1:]
print gvn.description.Description(ctx.config, ctx.project,
- reviewable).Output()
+ reviewable).Output(iterpool)
gvn.diff.RunSvnDiff(ctx.options, ctx.config, ['-c', reviewable,
ctx.project.repository.URL()])

@@ -59,9 +73,9 @@ def Handle_GvnReview(ctx):
"invalid operand: '%s'" % reviewable,
"neither a change name nor a revision number"
]))
-
+ iterpool.destroy()
return 0

gvn.cmdline.AddCommand('review', Handle_GvnReview, helptext__gvn_review,
- gvn.cmdline.AuthOptions(['project']))
+ gvn.cmdline.AuthOptions(['extensions', 'project']))
Index: lib/gvn/util.py
===================================================================
--- lib/gvn/util.py (added)
+++ lib/gvn/util.py (^/changes/epg/cb2.0-compat/trunk/lib/gvn/util.py@933)
@@ -27,6 +27,7 @@ import posixpath
import re
import shutil
import sys
+import tempfile
import time

from errno import ENOENT
@@ -521,3 +522,102 @@ def ParseOwners(owners_contents):

users.add(line)
return (users, groups, noparent)
+
+
+def ApplyPropDiffs(properties, property_diffs):
+ """Apply property_diffs to properties.
+
+ Arguments:
+ properties -- dict to be transformed
+ property_diffs -- dict as returned by svn.core.svn_prop_diffs
+ """
+ for (name, value) in property_diffs.iteritems():
+ if value is None:
+ del properties[name]
+ else:
+ properties[name] = value
+
+
+class TmpTree(object):
+ def __init__(self, *args, **kwargs):
+ """Initialize temporary tree.
+
+ Pass any arguments to tempfile.mkdtemp .
+ """
+ tmpdir = tempfile.mkdtemp(*args, **kwargs)
+ self._tmp_dirs = [tmpdir]
+ self._tmp_files = []
+
+ def Path(self, path_components):
+ """Return absolute temporary path for joined path_components .
+
+ Arguments:
+ path_components -- sequence of local style path components
+ (encoded str, not unicode)
+ """
+ if path_components[-1] == '':
+ # Caller is asking for one of the two base dir, e.g.
+ # self.Path(['left', os.path.dirname('COPYING')])
+ # dirname returns '' but caller wants '.../left' not '.../left/'.
+ path_components.pop(-1)
+ path_components.insert(0, self._tmp_dirs[0])
+ return os.path.join(*path_components)
+
+ def Mkdir(self, path_components):
+ """Make temporary directory.
+
+ Arguments:
+ path_components -- sequence of local style path components
+ (encoded str, not unicode)
+ """
+ path = self.Path(path_components)
+ # Build list of directories we need to create.
+ todo = []
+ while path not in self._tmp_dirs:
+ todo.append(path)
+ path = os.path.dirname(path)
+ # Create those directories, remembering that we created them.
+ for path in reversed(todo):
+ os.mkdir(path)
+ self._tmp_dirs.append(path)
+
+ def MkFile(self, path_components, mode='wb'):
+ """Return file object opened for binary writing.
+
+ Arguments:
+ path_components -- sequence of local style path components
+ (encoded str, not unicode)
+ """
+ path = self.Path(path_components)
+ self.Mkdir([os.path.dirname(path)])
+ fp = open(path, mode)
+ self._tmp_files.append(path)
+ return fp
+
+ def OpenFile(self, path_components):
+ """Return file object opened for binary reading.
+
+ Arguments:
+ path_components -- sequence of local style path components
+ (encoded str, not unicode)
+ """
+ path = self.Path(path_components)
+ return open(path, 'rb')
+
+ def RmFile(self, path_components):
+ """Remove a tmp file.
+
+ Arguments:
+ path_components -- sequence of local style path components
+ (encoded str, not unicode)
+ """
+ path = self.Path(path_components)
+ os.unlink(path)
+ self._tmp_files.remove(path)
+
+ def Close(self):
+ """Remove temporary files and directories."""
+ for path in self._tmp_files:
+ os.unlink(path)
+ for path in reversed(self._tmp_dirs):
+ os.rmdir(path)
Index: lib/gvn/wc.py
===================================================================
--- lib/gvn/wc.py (^/trunk/lib/gvn/wc.py@910)
+++ lib/gvn/wc.py (^/changes/epg/cb2.0-compat/trunk/lib/gvn/wc.py@933)
@@ -281,6 +281,35 @@ class WorkingCopy(object):
os.remove(state_file)
os.rename(fn, state_file)

+ # TODO(epg): see what else should use adm_access instead of _adm_access
+ def _GetAdmAccess(self):
+ if self._adm_access is None:
+ raise gvn.errors.WCClosed
+ return self._adm_access
+ adm_access = property(_GetAdmAccess,
+doc="""svn_wc_adm_access_t if WorkingCopy is Open
+
+ Raises:
+ gvn.errors.WCClosed
+""")
+
+ def AdmRetrieve(self, path, pool):
+ # TODO(epg): This is duplicated all over the place; make them all
+ # call this instead.
+
+ # This is stupid.
+ if os.path.isdir(path):
+ (directory, target) = (path, '')
+ else:
+ (directory, target) = os.path.split(path)
+ if directory == self.path:
+ return self.adm_access
+ return svn.wc.adm_retrieve(self.adm_access, directory, pool)
+
+ def GetPropDiffs(self, path, pool):
+ return svn.wc.get_prop_diffs(path.encode('utf8'),
+ self.AdmRetrieve(path, pool), pool)
+
def Open(self, paths=None, write_lock=False, recursive=True):
"""Open the working copy to deepest common of paths.

Index: setup.py
===================================================================
--- setup.py (^/trunk/setup.py@910)
+++ setup.py (^/changes/epg/cb2.0-compat/trunk/setup.py@933)
@@ -426,7 +426,10 @@ setup(name='gvn', version=GetVersion(),

package_dir={'': 'lib'},
packages=['gvn', 'gvn/subcommands', 'gvn/hooks', 'gvn/hooks/pre_commit',
- 'gvn/hooks/pre_revprop_change', 'gvn/hooks/post_commit' ],
+ 'gvn/hooks/pre_revprop_change', 'gvn/hooks/post_commit',
+ 'gvn/third_party',
+ 'gvn/third_party/simplejson',
+ ],
scripts=['bin/gvn', 'bin/gvn-hook-runner'],

cmdclass={
Index: testing/test_changebranch.py
===================================================================
--- testing/test_changebranch.py (^/trunk/testing/test_changebranch.py@910)
+++ testing/test_changebranch.py (^/changes/epg/cb2.0-compat/trunk/testing/test_changebranch.py@933)
@@ -701,51 +701,49 @@ new line
self.assertEquals(4, len(changebranch.RandomBranchName()))
self.assertEquals(8, len(changebranch.RandomBranchName(8)))

- def testDiff(self):
- # TODO(epg): Need more testing...
+ # TODO(epg): Diff functions need much more testing...
+
+ def testDiffSnapshotToWC(self):
paths2 = MakeChanges2()
change2 = self.BranchAndAssert('change2', paths2, 'change2 desc')

- # Test no diff command raises NoDiffCommand rather than exploding.
- class mock_file(object):
- def write(fp, line):
- pass
- save_diff_command = gvn.changebranch._DIFF_COMMAND
- try:
- gvn.changebranch._DIFF_COMMAND = 'nonexistent-diff-command'
- self.assertRaises(gvn.errors.NoDiffCommand,
- changebranch.Diff, self.wc.project.repository, change2,
- mock_file(), 'internal',
- pool=self.pool)
- finally:
- gvn.changebranch._DIFF_COMMAND = save_diff_command
-
# Test --from-change.
rho = 'greek-tree/A/D/G/rho'
fp = open(rho, 'w')
testcommon.sleepf(fp.write, 'replacement\n')
fp.close()
- expect = ['@@ -1,2 +1 @@',
- "-This is the file 'rho'.",
- '-new line',
- '+replacement']
+ expect = (
+ 'Index: greek-tree/A/D/G/rho\n'
+ '===================================================================\n'
+ '--- greek-tree/A/D/G/rho\t(^/changes/unittest/change2/projects/greek-tree/A/D/G/rho@%d)\n'
+ '+++ greek-tree/A/D/G/rho\t(working copy)\n'
+ '@@ -1,2 +1 @@\n'
+ "-This is the file 'rho'.\n"
+ '-new line\n'
+ '+replacement\n'
+ % (change2.revision.number,))
got = []
class mock_file(object):
def write(fp, line):
- got.append(line.rstrip())
- gvn.changebranch.Diff(self.wc.project.repository, change2, mock_file(),
- 'internal', [rho], self.wc, self.pool)
- self.assertEqual(got[3:], expect)
+ got.append(line)
+ encoding = 'utf8'
+ callbacks = gvn.changebranch.SvnDiffCallbacks(options=[],
+ out_file=mock_file(),
+ encoding=encoding,
+ pool=self.pool)
+ gvn.changebranch.DiffSnapshotToWC(change2, self.wc, callbacks, encoding,
+ self.pool)
+ self.assertEqual(''.join(got), expect)

# Test --from-change when not at the wc top.
os.chdir('greek-tree/A')
got = []
- gvn.changebranch.Diff(self.wc.project.repository, change2, mock_file(),
- 'internal', [rho], self.wc, self.pool)
- self.assertEqual(got[3:], expect)
+ gvn.changebranch.DiffSnapshotToWC(change2, self.wc, callbacks, encoding,
+ self.pool)
+ self.assertEqual(''.join(got), expect)
os.chdir('../..')

- def testDiffCpThenRm(self):
+ def testDiffChangesAfterCopy(self):
# Test a case that used to break gvn: copy a tree and then add
# and/or remove paths under the copied-to tree. In the past, this
# has variously exploded or shown nothing, depending on the phase
@@ -766,45 +764,137 @@ new line
change = ChangeBranch(self.config, self.wc.project, 'cp-then-rm')
change.Branch(self.wc, paths, None, self.pool)

+ expect = (
+ 'Index: acopy/gamma (deleted)\n'
+ '===================================================================\n'
+ 'Index: acopy/gamma-hey\n'
+ '===================================================================\n'
+ '--- acopy/gamma-hey\t(added)\n'
+ '+++ acopy/gamma-hey\t(^/changes/unittest/cp-then-rm/projects/acopy/gamma-hey@%d)\n'
+ '@@ -1 +1,2 @@\n'
+ " This is the file 'gamma'.\n"
+ '+add a line\n'
+ % (change.revision.number,))
+ got = []
class mock_file(object):
def write(fp, line):
- if line.startswith('--- '):
- self.assertEquals(line,
-"""--- acopy/gamma-hey (//public/projects/greek-tree/A/D/gamma@2)
-+++ acopy/gamma-hey (//changes/unittest/cp-then-rm/projects/acopy/gamma-hey@3)
-""")
- changebranch.Diff(self.wc.project.repository, change, mock_file(), None,
- pool=self.pool)
+ got.append(line)
+ encoding = 'utf8'
+ callbacks = gvn.changebranch.SvnDiffCallbacks(options=[],
+ out_file=mock_file(),
+ encoding=encoding,
+ pool=self.pool)
+ gvn.changebranch.DiffBaseToSnapshot(change, callbacks, encoding, self.pool)
+ self.assertEqual(''.join(got), expect)

- def testMerge(self):
+ def testDiff(self):
self.wc.Close()
- paths = MakeChanges(self.wc, self.pool)
- change1 = self.BranchAndAssert('Foo', paths, 'Foo desc')
+ # Build CreateAndAssert paths structure.
+ paths = [
+ ('D', 'greek-tree/A/B'),
+ ('R', 'greek-tree/A/mu'),
+ ('A', 'greek-tree/B.new'),
+ ('A', 'greek-tree/B.new/D.new'),
+ ('M', 'greek-tree/B.new/D.new/gamma'),
+ ('A', 'greek-tree/B.new/iota'),
+ ('M', 'greek-tree/B.new/E/alpha'),
+ ('M', 'greek-tree/B.new/F'),
+ ('R', 'greek-tree/iota'),
+ ]
+ # Rename A/B.
+ testcommon.RunSvn('move', 'greek-tree/A/B', 'greek-tree/B.new')
+ # Replace A/mu with a copy of gamma
+ testcommon.RunSvn('rm', 'greek-tree/A/mu')
+ testcommon.RunSvn('copy', 'greek-tree/A/D/gamma', 'greek-tree/A/mu')
+ # Copy A/D.
+ testcommon.RunSvn('copy', 'greek-tree/A/D', 'greek-tree/B.new/D.new')
+ # Edit A.new/D.new/gamma.
+ fp = open('greek-tree/B.new/D.new/gamma', 'a')
+ fp.write('hi\n')
+ fp.close()
+ # Copy iota.
+ testcommon.RunSvn('copy', 'greek-tree/iota', 'greek-tree/B.new')
+ # Edit B.new/E/alpha.
+ fp = open('greek-tree/B.new/E/alpha', 'a')
+ fp.write('hi\n')
+ fp.close()
+ # Edit B.new/F.
+ testcommon.RunSvn('ps', 'testprop', 'testval', 'greek-tree/B.new/F')
+ # Replace iota with new file
+ testcommon.RunSvn('rm', 'greek-tree/iota')
+ fp = open('greek-tree/iota', 'w')
+ fp.write('This is the replaced iota.\n')
+ fp.close()
+ testcommon.RunSvn('add', 'greek-tree/iota')

- self.wcdir = testcommon.NewWorkingCopy()
- os.chdir(self.wcdir)
- self.wc = wc.WorkingCopy(self.wcdir, cancel_func=None, notify_func=None,
- config=self.config, pool=self.pool)
- self.wc.Open()
- testcommon.GetAuthBaton(self.config, self.pool, self.wc.project.repository)
+ # Create changebranch.
+ cb = self.BranchAndAssert('Diff', paths, 'Diff desc')

- change2 = changebranch.Merge(change1, self.wc.project, self.wc, '',
- self.config, self.pool)
- self.assertEquals(change2.name, change1.name)
- self.assertEquals(_ChangeList(change2.changed_paths),
- _ChangeList(change1.changed_paths))
+ expect = (
+ 'Index: greek-tree/A/mu\n'
+ '===================================================================\n'
+ '\nProperty changes on: greek-tree/A/mu\n'
+ '___________________________________________________________________\n'
+ 'Added: svn:mergeinfo\n'
+ ' + \n'
+ '\nProperty changes on: greek-tree/B.new\n'
+ '___________________________________________________________________\n'
+ 'Added: svn:mergeinfo\n'
+ ' + \n'
+ '\nProperty changes on: greek-tree/B.new/D.new\n'
+ '___________________________________________________________________\n'
+ 'Added: svn:mergeinfo\n'
+ ' + \n'
+ 'Index: greek-tree/B.new/D.new/gamma\n'
+ '===================================================================\n'
+ '--- greek-tree/A/D/gamma\t(^/public/projects/greek-tree/A/D/gamma@%d)\n'
+ '+++ greek-tree/B.new/D.new/gamma\t(^/changes/unittest/Diff/projects/greek-tree/B.new/D.new/gamma@%d)\n'
+ '@@ -1 +1,2 @@\n'
+ " This is the file 'gamma'.\n"
+ '+hi\n'
+ 'Index: greek-tree/B.new/E/alpha\n'
+ '===================================================================\n'
+ '--- greek-tree/A/B/E/alpha\t(^/public/projects/greek-tree/A/B/E/alpha@%d)\n'
+ '+++ greek-tree/B.new/E/alpha\t(^/changes/unittest/Diff/projects/greek-tree/B.new/E/alpha@%d)\n'
+ '@@ -1 +1,2 @@\n'
+ " This is the file 'alpha'.\n"
+ '+hi\n'
+ '\nProperty changes on: greek-tree/B.new/F\n'
+ '___________________________________________________________________\n'
+ 'Added: testprop\n'
+ ' + testval\n'
+ 'Index: greek-tree/B.new/iota\n'
+ '===================================================================\n'
+ '\nProperty changes on: greek-tree/B.new/iota\n'
+ '___________________________________________________________________\n'
+ 'Added: svn:mergeinfo\n'
+ ' + \n'
+ # TODO(next-release): When we drop old-style changebranches, this
+ # will be a diff as if it were Modified; that's good, just what svn
+ # does; don't be alarmed, just update this.
+ 'Index: greek-tree/iota\n'
+ '===================================================================\n'
+ '--- greek-tree/iota\t(added)\n'
+ '+++ greek-tree/iota\t(^/changes/unittest/Diff/projects/greek-tree/iota@5)\n'
+ '@@ -0,0 +1 @@\n'
+ '+This is the replaced iota.\n'
+ % (cb.revision.number - 1, cb.revision.number,
+ cb.revision.number - 1, cb.revision.number))
+ got = []
+ class mock_file(object):
+ def write(fp, line):
+ got.append(line)
+ encoding = 'utf8'
+ callbacks = gvn.changebranch.SvnDiffCallbacks(options=[],
+ out_file=mock_file(),
+ encoding=encoding,
+ pool=self.pool)
+ gvn.changebranch.DiffBaseToSnapshot(cb, callbacks, encoding, self.pool)
+ self.assertEqual(''.join(got), expect)

- # Merge as different user.
- self.wcdir = testcommon.NewWorkingCopy()
- os.chdir(self.wcdir)
- self.wc = wc.WorkingCopy(self.wcdir, cancel_func=None, notify_func=None,
- config=self.config, pool=self.pool)
- self.wc.Open()
- self.wc.project.repository._username = 'basil'
- testcommon.GetAuthBaton(self.config, self.pool, self.wc.project.repository)
- change2 = changebranch.Merge(change1, self.wc.project, self.wc, '',
- self.config, self.pool)
- self.assertEquals(change2.username, 'basil')
- self.assertEquals(change2.name, change1.name)
- self.assertEquals(_ChangeList(change2.changed_paths),
- _ChangeList(change1.changed_paths))
+ # TODO(next-release):
+ # - Edit just one of those files, snapshot, make sure everything
+ # still looks right.
+ # - Edit one of the directories, snapshot, make sure everything
+ # still looks right.
+ # - Submit the change, make sure it looks right.

David Glasser

unread,

May 26, 2008, 6:20:40 PM5/26/08

to e...@google.com, gvn-d...@googlegroups.com

Generally pretty good (though just from reading, not running). I see
more and more why the new changebranches will be better. A few inline
comments:

On Thu, May 22, 2008 at 4:39 PM, <e...@google.com> wrote:
>
> I'd like you to do a code review. To review this change, run
>
> gvn review --project https://gvn.googlecode.com/svn epg/cb2.0-compat@933
>
> Alternatively, to review the latest snapshot of this change
> branch, run
>
> gvn --project https://gvn.googlecode.com/svn review epg/cb2.0-compat
>
> to review the following change:
>
> *epg/cb2.0-compat@933 | epg | 2008-05-22 15:39:05 -0800 (Thu, 22 May 2008)
>
> Description:
>

[...]

Ick. Why not:

if copied_directory_stack:
(...) = copied_directory_stack[01]
path_in_copied_dir = ...
...
else:
left_path = ...
left_rev = ...

So given that this code is all for old-style branches anyway, I think
we should just do the extra check_path so we don't have to stress
about stupid DAV bug.

Similarly here. No need to use exceptions for non-exceptional cases
(in fact, the common case!).

> + if (last_copied_dir is not None
> + and not gvn.util.IsChild(path, last_copied_dir[0])):
> + # Yep, so pop it off the stack.
> + copied_directory_stack.pop(-1)

-1 is the default here anyway.

> + if ctx.config.diff_command in [None, 'internal']:
> + callbacks = gvn.changebranch.SvnDiffCallbacks(ctx.options.extensions,
> + sys.stdout, ctx.encoding,
> + iterpool)
> + elif os.path.basename(ctx.config.diff_command) == 'tkdiff':
> + callbacks = gvn.changebranch.TkDiffCallbacks(ctx.config.diff_command,
> + ctx.encoding)
> + else:
> + # TODO: Implement callbacks that run user's custom diff command.
> + callbacks = gvn.changebranch.UserDiffCallbacks(ctx.config.diff_command)

Put an empty version of this in changebranch.py? (Though speaking of
which, perhaps the diff callback implementations want to get their own
file and keep changebranch.py a little svelter.)

Document what the point of this class is. (Looks pretty useful
though!)

--
David Glasser | gla...@davidglasser.net | http://www.davidglasser.net/

e...@google.com

unread,

May 27, 2008, 8:54:13 PM5/27/08

to David Glasser, gvn-d...@googlegroups.com

"David Glasser" <gla...@davidglasser.net> writes:

> > +def _FindLeft(path, base_revision, repo_state, copied_directory_stack):

> > + try:
> > + # Check for a copied directory.
> > + (copied_dir, copyfrom_path, copyfrom_revision) = copied_directory_stack[-1]
> > + except IndexError:
> > + # Not under copied directory; use same path for left side.
> > + left_path = '/'.join([repo_state.base_path, path])
> > + left_revision = base_revision
> > else:

> > + # Some parent directory was copied; find left side in there.
> > + path_in_copied_dir = path[len(copied_dir) + 1:]
> > + left_path = '/'.join([copyfrom_path, path_in_copied_dir])
> > + left_revision = copyfrom_revision
>
> Ick. Why not:

Like this?

if len(copied_directory_stack) > 0:

# Some parent directory was copied; find left side in there.

(copied_dir, copyfrom_path, copyfrom_revision) = copied_directory_stack[-1]

path_in_copied_dir = path[len(copied_dir) + 1:]

left_path = '/'.join([copyfrom_path, path_in_copied_dir])

left_revision = copyfrom_revision
else:

# Not under copied directory; use same path for left side.

left_path = '/'.join([repo_state.base_path, path])

left_revision = base_revision

> > +def _RaGetDirOrFile(ra, path, revision, fp, dirent_fields, pool,
> > + kind=svn_node_unknown):

> > + if kind == svn_node_file:
> > + result = svn.ra.get_file(ra, path, revision, fp, pool)
> > + return (kind, result)
> >

> > + if kind == svn_node_dir:
> > + result = svn.ra.get_dir2(ra, path, revision, dirent_fields, pool)
> > + return (kind, result)
> >

> > + gvn.DiagLog('_RaGetDirOrFile(%s@%d) => try get_file => ', path, revision)
> > + try:
> > + result = svn.ra.get_file(ra, path, revision, fp, pool)
> > + kind = svn_node_file
> > + gvn.DiagLog('is file')
>
> So given that this code is all for old-style branches anyway, I think
> we should just do the extra check_path so we don't have to stress
> about stupid DAV bug.

There's no stress; the workaround is very simple. Are you
advocating dropping it? If its _RaGetDirOrFile's very existence
that bothers you, I'd still need it if I were doing check_path,
unless I wanted to put this compat logic into DiffBaseToSnapshot
directly, which I'd rather not do. This will be gone soon enough.

How's this?

if len(copied_directory_stack) > 0:
last_copied_dir = copied_directory_stack[-1][0]
if not gvn.util.IsChild(path, last_copied_dir):

# Yep, so pop it off the stack.

copied_directory_stack.pop(-1)

> > + if ctx.config.diff_command in [None, 'internal']:
> > + callbacks = gvn.changebranch.SvnDiffCallbacks(ctx.options.extensions,
> > + sys.stdout, ctx.encoding,
> > + iterpool)
> > + elif os.path.basename(ctx.config.diff_command) == 'tkdiff':
> > + callbacks = gvn.changebranch.TkDiffCallbacks(ctx.config.diff_command,
> > + ctx.encoding)
> > + else:
> > + # TODO: Implement callbacks that run user's custom diff command.
> > + callbacks = gvn.changebranch.UserDiffCallbacks(ctx.config.diff_command)
>
> Put an empty version of this in changebranch.py?

To what end? Are you just worried about the error message? I
can provide a better error message here in less time.

> (Though speaking of
> which, perhaps the diff callback implementations want to get their own
> file and keep changebranch.py a little svelter.)

Maybe, but what do you call it? changebranch_diff_callbacks? Blech.

> > +class TmpTree(object):
>
> Document what the point of this class is. (Looks pretty useful
> though!)

You don't think "TmpTree manages a temporary tree" is redundant?
If not, or if you have a better idea, I can add it. I thought
the name was obvious, but that's often wrong :).

Snapshotting this:

0 gvn7% gvn di -c cb2.0-compat --from-change -x -up >> ~/mh/drafts/5
Index: lib/gvn/changebranch.py
===================================================================
--- lib/gvn/changebranch.py (^/changes/epg/cb2.0-compat/trunk/lib/gvn/changebranch.py@934)
+++ lib/gvn/changebranch.py (working copy)
@@ -1112,18 +1112,16 @@ def _FindLeft(path, base_revision, repo_state, cop

copied_directory_stack -- list of copied directories

(see DiffBaseToSnapshot)
"""
- try:
- # Check for a copied directory.
- (copied_dir, copyfrom_path, copyfrom_revision) = copied_directory_stack[-1]
- except IndexError:
- # Not under copied directory; use same path for left side.
- left_path = '/'.join([repo_state.base_path, path])
- left_revision = base_revision
- else:
+ if len(copied_directory_stack) > 0:

# Some parent directory was copied; find left side in there.

+ (copied_dir, copyfrom_path, copyfrom_revision) = copied_directory_stack[-1]

path_in_copied_dir = path[len(copied_dir) + 1:]

left_path = '/'.join([copyfrom_path, path_in_copied_dir])

left_revision = copyfrom_revision
+ else:

+ # Not under copied directory; use same path for left side.
+ left_path = '/'.join([repo_state.base_path, path])
+ left_revision = base_revision

# Now make the header.

if repo_state.paths[path].action in _ADD_CODES:

left_header = path + '\t(added)'

@@ -1196,14 +1194,11 @@ def DiffBaseToSnapshot(cb, callbacks, encoding, po

key=lambda x: x[0]):

iterpool.clear()

# See if we just came out from under a copied directory.

- try:
- last_copied_dir = copied_directory_stack[-1]
- except IndexError:
- last_copied_dir = None
- if (last_copied_dir is not None
- and not gvn.util.IsChild(path, last_copied_dir[0])):
- # Yep, so pop it off the stack.
- copied_directory_stack.pop(-1)
+ if len(copied_directory_stack) > 0:
+ last_copied_dir = copied_directory_stack[-1][0]
+ if not gvn.util.IsChild(path, last_copied_dir):

+ # Yep, so pop it off the stack.

+ copied_directory_stack.pop()

kind = state.kind

David Glasser

unread,

May 27, 2008, 9:04:07 PM5/27/08

to e...@google.com, gvn-d...@googlegroups.com

Yeah (though "if foo" is a fine pythonism for "if len(foo) > 0").

>> > +def _RaGetDirOrFile(ra, path, revision, fp, dirent_fields, pool,
>> > + kind=svn_node_unknown):
>> > + if kind == svn_node_file:
>> > + result = svn.ra.get_file(ra, path, revision, fp, pool)
>> > + return (kind, result)
>> >
>> > + if kind == svn_node_dir:
>> > + result = svn.ra.get_dir2(ra, path, revision, dirent_fields, pool)
>> > + return (kind, result)
>> >
>> > + gvn.DiagLog('_RaGetDirOrFile(%s@%d) => try get_file => ', path, revision)
>> > + try:
>> > + result = svn.ra.get_file(ra, path, revision, fp, pool)
>> > + kind = svn_node_file
>> > + gvn.DiagLog('is file')
>>
>> So given that this code is all for old-style branches anyway, I think
>> we should just do the extra check_path so we don't have to stress
>> about stupid DAV bug.
>
> There's no stress; the workaround is very simple. Are you
> advocating dropping it? If its _RaGetDirOrFile's very existence
> that bothers you, I'd still need it if I were doing check_path,
> unless I wanted to put this compat logic into DiffBaseToSnapshot
> directly, which I'd rather not do. This will be gone soon enough.

If we are only running the workaround for a short period (until old
changebranches are gone), might as well use the version that obviously
works instead of the one that dances close to RA-specific bugs.

Looks fine, though again you can leave off the len(...) > 0, and the
-1 in "pop".

>> > + if ctx.config.diff_command in [None, 'internal']:
>> > + callbacks = gvn.changebranch.SvnDiffCallbacks(ctx.options.extensions,
>> > + sys.stdout, ctx.encoding,
>> > + iterpool)
>> > + elif os.path.basename(ctx.config.diff_command) == 'tkdiff':
>> > + callbacks = gvn.changebranch.TkDiffCallbacks(ctx.config.diff_command,
>> > + ctx.encoding)
>> > + else:
>> > + # TODO: Implement callbacks that run user's custom diff command.
>> > + callbacks = gvn.changebranch.UserDiffCallbacks(ctx.config.diff_command)
>>
>> Put an empty version of this in changebranch.py?
>
> To what end? Are you just worried about the error message? I
> can provide a better error message here in less time.

Well, I kinda feel like TODOs should be like "fill in the blank", but
this TODO means "go to a completely different file and start writing
things from scratch".

>> (Though speaking of
>> which, perhaps the diff callback implementations want to get their own
>> file and keep changebranch.py a little svelter.)
>
> Maybe, but what do you call it? changebranch_diff_callbacks? Blech.

shrug.

>> > +class TmpTree(object):
>>
>> Document what the point of this class is. (Looks pretty useful
>> though!)
>
> You don't think "TmpTree manages a temporary tree" is redundant?
> If not, or if you have a better idea, I can add it. I thought
> the name was obvious, but that's often wrong :).

A tree of temporary files on disk? I dunno. It was more obvious once
I saw it used.

--dave

e...@google.com

unread,

May 27, 2008, 9:16:24 PM5/27/08

to David Glasser, gvn-d...@googlegroups.com

"David Glasser" <gla...@davidglasser.net> writes:

Explicit is better than implicit :). Seriously, I've been burned
by 'if foo' where 'foo' evaluating to False could mean any of a
dozen of wrong things, and got burned by it being the wrong one.
I think you can even find an example or two in our repository.

> >> > +def _RaGetDirOrFile(ra, path, revision, fp, dirent_fields, pool,
> >> > + kind=svn_node_unknown):
> >> > + if kind == svn_node_file:
> >> > + result = svn.ra.get_file(ra, path, revision, fp, pool)
> >> > + return (kind, result)
> >> >
> >> > + if kind == svn_node_dir:
> >> > + result = svn.ra.get_dir2(ra, path, revision, dirent_fields, pool)
> >> > + return (kind, result)
> >> >
> >> > + gvn.DiagLog('_RaGetDirOrFile(%s@%d) => try get_file => ', path, revision)
> >> > + try:
> >> > + result = svn.ra.get_file(ra, path, revision, fp, pool)
> >> > + kind = svn_node_file
> >> > + gvn.DiagLog('is file')
> >>
> >> So given that this code is all for old-style branches anyway, I think
> >> we should just do the extra check_path so we don't have to stress
> >> about stupid DAV bug.
> >
> > There's no stress; the workaround is very simple. Are you
> > advocating dropping it? If its _RaGetDirOrFile's very existence
> > that bothers you, I'd still need it if I were doing check_path,
> > unless I wanted to put this compat logic into DiffBaseToSnapshot
> > directly, which I'd rather not do. This will be gone soon enough.
>
> If we are only running the workaround for a short period (until old
> changebranches are gone), might as well use the version that obviously
> works instead of the one that dances close to RA-specific bugs.

I'm not sure I understand the objection; the close dancing is
intentional. Are you afraid it's fragile? It's quite specific,
and we know exactly how this behaves. That behavior isn't going
to change until 1.6 at earliest, and this code will be long gone
by then.

These additional turnarounds really hurt the very users this code
mainly targets: gvn users on code.google.com.

> > How's this?
> >
> > if len(copied_directory_stack) > 0:
> > last_copied_dir = copied_directory_stack[-1][0]
> > if not gvn.util.IsChild(path, last_copied_dir):
> > # Yep, so pop it off the stack.
> > copied_directory_stack.pop(-1)
>
> Looks fine, though again you can leave off the len(...) > 0, and the
> -1 in "pop".

Huh, I did kill the -1; came back in further editing I guess.

> Well, I kinda feel like TODOs should be like "fill in the blank", but
> this TODO means "go to a completely different file and start writing
> things from scratch".

TODO means "here's a task to do"; don't read too much into it :).
Anyway, I just went ahead and implemented it; see epg/cb2.0-compat@936.

> >> > +class TmpTree(object):
> >>
> >> Document what the point of this class is. (Looks pretty useful
> >> though!)
> >
> > You don't think "TmpTree manages a temporary tree" is redundant?
> > If not, or if you have a better idea, I can add it. I thought
> > the name was obvious, but that's often wrong :).
>
> A tree of temporary files on disk? I dunno. It was more obvious once
> I saw it used.

I guess I'm not opposed to adding "temporary files on disk", but
where would you think the temporary files are? Since it honors
TMPDIR, they're not even necessarily "on disk" :).

e...@google.com

unread,

May 27, 2008, 9:34:59 PM5/27/08

to David Glasser, gvn-d...@googlegroups.com

epg writes:

> > > if len(copied_directory_stack) > 0:
> > > last_copied_dir = copied_directory_stack[-1][0]
> > > if not gvn.util.IsChild(path, last_copied_dir):
> > > # Yep, so pop it off the stack.
> > > copied_directory_stack.pop(-1)
> >
> > Looks fine, though again you can leave off the len(...) > 0, and the
> > -1 in "pop".
>
> Huh, I did kill the -1; came back in further editing I guess.

Yep, what I snapshotted had no -1 here.

> > Well, I kinda feel like TODOs should be like "fill in the blank", but
> > this TODO means "go to a completely different file and start writing
> > things from scratch".
>
> TODO means "here's a task to do"; don't read too much into it :).
> Anyway, I just went ahead and implemented it; see epg/cb2.0-compat@936.

Then I realized I had no Index: lines. Of course, svn prints
those even when you use a custom diff command. So, it's probably
easier to collapse custom command into SvnDiffCallbacks, as the
diff below does. What do you think? Also, I changed it to use
the shell so you can set diff_command to 'diff -u' or set an
environment variable first or something. Probably I should
rename the class now; SvnOrCustomDiffCallbacks? Bit unwieldy...

What do you think?

Index: lib/gvn/subcommands/diff.py
===================================================================
--- lib/gvn/subcommands/diff.py (^/changes/epg/cb2.0-compat/trunk/lib/gvn/subcommands/diff.py@937)
+++ lib/gvn/subcommands/diff.py (working copy)
@@ -85,8 +85,9 @@ def Handle_GvnDiff(ctx):
ctx.encoding)
else:
sys.stdout.flush()
- callbacks = gvn.changebranch.CustomDiffCallbacks(
- ctx.config.diff_command, ctx.encoding)
+ callbacks = gvn.changebranch.SvnDiffCallbacks(
+ ctx.options.extensions, sys.stdout, ctx.encoding, ctx.pool,
+ command=ctx.config.diff_command)
return gvn.changebranch.DiffSnapshotToWC(cb, ctx.wc, callbacks,
ctx.encoding, ctx.pool)

Index: lib/gvn/changebranch.py
===================================================================
--- lib/gvn/changebranch.py (^/changes/epg/cb2.0-compat/trunk/lib/gvn/changebranch.py@937)
+++ lib/gvn/changebranch.py (working copy)
@@ -949,7 +949,7 @@ class DiffCallbacks(object):
class SvnDiffCallbacks(DiffCallbacks):

"""DiffCallbacks implementation using svn's internal textual diff."""

- def __init__(self, options, out_file, encoding, pool):
+ def __init__(self, options, out_file, encoding, pool, command=None):

"""Initialize from list, file-like, str, svn.core.Pool .

Arguments:
@@ -957,11 +957,14 @@ class SvnDiffCallbacks(DiffCallbacks):

out_file -- file-like object to which to write the diff

encoding -- user's encoding

pool -- scratch memory pool; may be cleared after instantiation

+ command -- optional custom diff; passed to subprocess.call (with shell)
"""
self.options = svn.diff.file_options_create()
svn.diff.file_options_parse(self.options, options, pool)
self.fp = out_file
self.encoding = encoding
+ self.command = command
+ self.result = 0

def _DiffProps(self, path, left_properties, right_properties, pool):

prop_diffs = svn.core.svn_prop_diffs(right_properties, left_properties,
@@ -1019,17 +1022,33 @@ class SvnDiffCallbacks(DiffCallbacks):

and self.repo_state is not None

and self.repo_state.paths[path].action == 'D'):

return
- diff = svn.diff.file_diff_2(left_path, right_path,
- self.options, pool)
- relative_to_dir = None
- svn.diff.file_output_unified3(self.fp, diff,
- left_path, right_path,
- left_header, right_header,
- self.encoding, relative_to_dir,
- self.options.show_c_function, pool)
+
+ if self.command is None:

+ diff = svn.diff.file_diff_2(left_path, right_path,
+ self.options, pool)
+ relative_to_dir = None
+ svn.diff.file_output_unified3(self.fp, diff,
+ left_path, right_path,
+ left_header, right_header,
+ self.encoding, relative_to_dir,
+ self.options.show_c_function, pool)

+ # TODO(epg): Would be nice if svn.diff could tell us if anything
+ # changed so we could set self.result here.
+ else:
+ cmd = '%s %s %s' % (self.command, left_path, right_path)
+ gvn.DiagLog('SvnDiffCallbacks.File => subprocess.call(%s, shell=True)\n',
+ cmd)
+ self.fp.flush()
+ result = subprocess.call(cmd, shell=True)
+ if result != 0:
+ self.result = result

+
self._DiffProps(path, left_properties, right_properties, pool)

+ def Finish(self, pool):
+ return self.result

+
def _MaybeWriteProperties(properties, filename):

"""Serialize properties into the file filename, if any properties.

@@ -1085,34 +1104,6 @@ class TkDiffCallbacks(DiffCallbacks):
argv.append(right)
return subprocess.call(argv)

-class CustomDiffCallbacks(DiffCallbacks):
- """DiffCallbacks implementation for custom diff command."""
-
- def __init__(self, command, encoding):
- """Initialize from str, str.
-
- Arguments:
- command -- custom command; passed to subprocess.call (without shell)
- encoding -- user's encoding
- """
- self.command = command
- self.encoding = encoding
- self.result = 0
-
- def File(self, path, left_path, right_path,
- left_properties, right_properties,
- left_header, right_header, pool):
- argv = [self.command]
- argv.append(left_path)
- argv.append(right_path)
- gvn.DiagLog('CustomDiffCallbacks.File => subprocess.call(%s)\n', argv)
- result = subprocess.call(argv)
- if result != 0:
- self.result = result
-
- def Finish(self, pool):
- return self.result
-
#################################################################
# now, the Diff functions

Index: lib/gvn/subcommands/review.py
===================================================================
--- lib/gvn/subcommands/review.py (^/changes/epg/cb2.0-compat/trunk/lib/gvn/subcommands/review.py@937)
+++ lib/gvn/subcommands/review.py (working copy)
@@ -56,8 +56,9 @@ def Handle_GvnReview(ctx):
ctx.encoding)
else:
sys.stdout.flush()
- callbacks = gvn.changebranch.CustomDiffCallbacks(
- ctx.config.diff_command, ctx.encoding)
+ callbacks = gvn.changebranch.SvnDiffCallbacks(
+ ctx.options.extensions, sys.stdout, ctx.encoding, iterpool,
+ command=ctx.config.diff_command)
return gvn.changebranch.DiffBaseToSnapshot(cb, callbacks, ctx.encoding,
iterpool)

Reply all

Reply to author

Forward