GNOME Bugzilla – Bug 597631
diff fails if svn username of last commit has spaces
Last modified: 2010-09-30 21:46:51 UTC
Created attachment 144925 [details] Patch to move from 'svn status -Nv' to 'svn status -Nv --xml' to resolve problem with spaces in svn usernames If the svn username of the last person to commit changes to file has spaces meld will not process the file correctly. In addition, if the svn username of the last person to commit to a subdirectory has spaces the whole subdirectory will fail to be shown. The problem is thus: In vc/svn.py the following regex string is used to match version controlled files from the output of 'svn status -Nv': re_status_vc = re.compile(r'^(.) +\d+ +(\?|(?:\d+)) +[^ ]+ +([^ ].*)$') This expects that the 3rd group will be the filename. The output of "svn status -Nv" looks something like this, depending on the state of your tree: 6 3 dkelley fake_dir_1/test2 D 7 7 dkelley fake_dir_1/test2/bleh.c.merge2sv_bak.eoYs60 M 7 7 dkelley fake_dir_1/test2/bleh.c However, if the username is not dkelley, but 'david kelley', then the result is that group(3) of the above regex match, which is supposed to be the file/dir name, evaluates to 'kelley fake_dir_1/test2', and is corrupted for future use. I have created the following patch against meld-1.3.1 which uses the svn status '--xml' option instead. This works great for my environment, even if users have spaces in their usernames. However, I have some concerns. If the xml output of different versions of svn is drastically different then this method may fail (eg. if an entire entry is on one line instead of broken up, then the opening identifier of the entry will be matched and the line will be discarded without processing it's contents). I don't believe this is the case, but I have also not checked. It may also be advisable to modify what I have done to use built in python xml parsing, but I avoided this for two reasons. One, I'm not really sure how to do it and I was hoping to fix this quickly and move o with my work. Two I'm not sure if it adds another dependency to meld which may not be on all systems. Please let me know if you have any questions about the bug or my solution for it. Thanks, David
Created attachment 145301 [details] [review] Patch to move from 'svn status -Nv' to 'svn status -Nv --xml' to resolve problem with spaces in svn usernames Removed some debug prints Fixed some checks for item type. One string was checked as "unrevisioned" when it should have been "unversioned". Added a check for "missing".
The same problem has been troubling me for a while, making the version control support in meld on current Debian and Ubuntu pretty much unusable. Here's a Debian bug report from last September that duplicates this one: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=545359 Is there anything I can do to speed up adoption of this patch, e.g. to review it?
Hi David, thanks for the patch. You're right, regexes have been flaky and XML status is definitely the way to go here. It seems like a shame to keep the regexes though since we've got a well formed xml document. Could you extract the values with an xml parser instead of hand parsing? ElementTree looks like a good match. e.g. import xml.etree.ElementTree import os cmd = "svn status -Nv --xml /home/stephen/dev/mdiff/vc" tree = xml.etree.ElementTree.fromstring( os.popen(cmd).read() ) for target in tree.findall("target"): target_path = target.attrib["path"] for entry in (t for t in target.getchildren() if t.tag == "entry"): path = os.path.join( target_path, entry.attrib["path"] ) for status in (e for e in entry.getchildren() if e.tag=="wc-status"): print path, status.tag, status.attrib
Created attachment 152065 [details] [review] Patch to move from 'svn status -Nv' to 'svn status -Nv --xml' to resolve problem with spaces in svn usernames Hi Stephen, I agree that using ElementTree would be better. It looks like you did most of the work already in your post, however, in case you haven't finished it off, here is a patch which solves the problem using ElementTree. It works for me, however, I would appreciate it if you could look it over for any obvious mistakes. I don't spend much time programming in python. Thanks, David
Created attachment 164592 [details] [review] Modified version of above patch This is a reworked version of the above patch. It's now simpler and should deal with more statuses. The downside is that by using ElementTree, we're limiting ourselves to Python 2.5; currently Meld only requires 2.4. However, we may well version bump in the near future, at which point this could go in. Testing and comments appreciated.
As I mentioned in bug #483451: the --xml flag wasn't introduced until 1.3 according to the release notes: http://subversion.apache.org/docs/release-notes/1.3.html I agree that XML is a good thing in a forward looking sense, but just remember that you're going to be killing support for older versions of Subversion. It should also be noted that Tigress or Apache or whomever is running Subversion development does not support 1.4.x and before as of 1.6.
(In reply to comment #5) > Created an attachment (id=164592) [details] [review] > Modified version of above patch > > This is a reworked version of the above patch. It's now simpler and should deal > with more statuses. The downside is that by using ElementTree, we're limiting > ourselves to Python 2.5; currently Meld only requires 2.4. However, we may well > version bump in the near future, at which point this could go in. > > Testing and comments appreciated. This patch works for me. I verified that my SVN tree still exhibits the problem using the clean meld 1.3.1 but is resolved with this patch. I'm not sure how important it is to keep compatibility with Python < 2.6 and SVN < 1.3. Both of these are decently old now, but sometimes you are on a system where you do not have access to upgrade. It would be a shame to break new versions of meld for those users. It would also be a shame not to fix meld for many of the people experiencing this bug. If it were my project I'd move on to the XML based formatting. Just my two cents. Thanks for all the hard work. Cheers, David
I've pushed the above patch to HEAD. Thanks for the bug report and testing.