Improved diff and properties #99
Conversation
apocryphal
commented
Sep 15, 2017
apocryphal
commented
- Improves unit testing on Windows by ensuring canonical file uri.
- Improves diff behavior, enabling direct diffing of a single file.
- Fixes a bug in diff, where == (equality operator) was used as a separator, which would lead to incomplete diff results.
- Fixes properties method failed when no props defined #89
…ed (svn_uri_is_canonical(url, scratch_pool))
…lit from == to 67 ='s to match the output. Original would break on equality checks in many languages. 2. Improved handling of diff on directories so that the root directory does not cause an exception.
| if target_elem is not None: | ||
| property_names = [p.attrib["name"] | ||
| for p in target_elem.findall('property')] | ||
| else: |
There was a problem hiding this comment.
Any time there are no properties associated, the "target" element it's searching for won't be found.
Examples:
http://svn.apache.org/repos/asf/activemq/activecluster/trunk/src/main/java/org/apache/activecluster@1808406
<?xml version="1.0" encoding="UTF-8"?>
<properties>
</properties>
http://svn.apache.org/repos/asf/activemq/activecluster/trunk/src/main/java/org/apache/activecluster/Cluster.java@1808406
<?xml version="1.0" encoding="UTF-8"?>
<properties>
<target path="http://svn.apache.org/repos/asf/activemq/activecluster/trunk/src/main/java/org/apache/activecluster/Cluster.java">
<property name="svn:eol-style"/>
</target>
</properties>
There was a problem hiding this comment.
Oh, I got it. Thanks for clarifying. You're absolutely sure that you can't have more than one target? Seems like you could.
svn/common.py
Outdated
| @@ -450,12 +453,18 @@ def diff(self, old, new, rel_path=None): | |||
| do_combine=True) | |||
| file_to_diff = {} | |||
| for non_empty_diff in filter(None, diff_result.decode('utf8').split('Index: ')): | |||
| split_diff = non_empty_diff.split('==') | |||
| split_diff = \ | |||
There was a problem hiding this comment.
The current implementation would split the overall diff text any time it finds a ==, including in places like if (a == b). Without an equivalent change, the diff text will be partially lost.
I can think of prettier ways to do it, say something like '='*67 instead of having a big long list.
I provided the unit test test_diff_separator for this one, which checks for code above and below a double-equals in a known file. It passes with the new implementation, but fails with the old one.
| file_to_diff[split_diff[0].strip().strip('/')] = split_diff[-1].strip('=').strip() | ||
| diff_summaries = self.diff_summary(old, new, rel_path) | ||
| for diff_summary in diff_summaries: | ||
| diff_summary['diff'] = \ | ||
| file_to_diff[diff_summary['path'].split(full_url_or_path)[-1].strip('/')] | ||
| diff_summary['diff'] = '' |
There was a problem hiding this comment.
This section could use some commentary. Also, can you also refactor the split() to use startswith() or searching instead? I realize you weren't responsible for this, but the way it is is far from intuitive. Though I can surmise what we're doing here, it's not obvious when the if's will or will not trigger.
| @@ -32,7 +32,10 @@ def test_create_repository(self): | |||
| a.create(temp_path) | |||
|
|
|||
| # Do a read. | |||
| rc = svn.remote.RemoteClient('file://' + temp_path) | |||
| if temp_path[0] == '/': | |||
There was a problem hiding this comment.
Can you place a comment indicating that this is the duck-typing approach to detecting Windows versus non-Windows?
| @@ -54,7 +54,10 @@ def __setup_test_environment(self): | |||
| self.__temp_co_path = self.__get_temp_path_to_use() | |||
| print("CO_PATH: {}".format(self.__temp_co_path)) | |||
|
|
|||
| r = svn.remote.RemoteClient('file://' + self.__temp_repo_path) | |||
| if self.__temp_repo_path[0] == '/': | |||
|
This will have just been broken by a reimplementation of diff that I did. |
|
Closed since stale. |
