GNOME Bugzilla – Bug 557615
svn keyword expansion causing patch to fail
Last modified: 2012-12-02 22:16:33 UTC
Please describe the problem: The following report was posted on the meld mailing list: From: Edwin Vane <revane <at> gmail.com> Subject: GNU Patch version? Newsgroups: gmane.comp.gnome.meld.general Date: 2008-07-15 13:26:52 GMT (14 weeks, 2 days and 41 minutes ago) Hi, Using meld today I came across an error dialogue unexpectedly that read: Invoking patch failed, you need GNU patch. 'patch --strip=0 --reverse --directory=/tmp/tmpfsGU0V-meld' I have GNU patch 2.5.9 installed (the most current available to Ubuntu Gutsy). I can't immediately see anything weird about this file but I can look into it later if you guys don't have something obvious to suggest right now. Run from the command-line I get this Traceback: Traceback (most recent call last):
+ Trace 208596
ret = task()
self.show_patch(prefix, patch)
misc.run_dialog( _("Invoking patch failed, you need GNU patch.")+ "\n'%s'"%" ".join(patchcmd), parent=self)
d.set_transient_for(parent.widget.get_toplevel())
-- Edwin V ---------------------------------------------------------------------------- Following this I posted this followup ---------------------------------------------------------------------------- On Wed, Oct 22, 2008 at 7:20 PM, Rick Dynarski <rdynarski@qualcomm.com> wrote: > I had this same problem and was able to track down the failure but am > still looking at how to fix it. The error dialogue happens on any > file which fails when a patch is applied. Patch fails whenever a diff > is within 3 lines(normal diff context) of a SVN expanded keyword. The > happens because the patch file is generated from svn diff(which does > not expand the keywords) but in vcview.show_patch the shutil.copyfile > call copies the file from the tree and this contains all of the > subversion keywords expanded. When patch goes to apply the patch file it will fail because the patch looks like this: > > Index: Makefile > =================================================================== > --- Makefile (revision 973) > +++ Makefile (working copy) > @@ -1,4 +1,4 @@ > -# > +# > # $Id$ > # > # The following creates/sets a few fundamental shell variables for us > > But the actual file looks like this: > > # > # $Id: Makefile 942 2008-10-17 18:55:48Z fubar $ # # The following > creates/sets a few fundamental shell variables for us > > Therefore, patch cannot figure out how to apply the patch because the > diff file does not match the real file because in the diff the $Id$ > keyword has not been expanded and in the copied file it has already > been expanded by subversion. Thanks for the additional information, this should be helpful to reproduce & fix, I'm on holidays now so won't be able to fix it, but it should not be too difficult If you hadn't added those infos to any bug report, please do so, so that other can have a shot at it. -- Vincent Legoll Steps to reproduce: I had this same problem and was able to track down the failure but am > still looking at how to fix it. The error dialogue happens on any > file which fails when a patch is applied. Patch fails whenever a diff > is within 3 lines(normal diff context) of a SVN expanded keyword. The > happens because the patch file is generated from svn diff(which does > not expand the keywords) but in vcview.show_patch the shutil.copyfile > call copies the file from the tree and this contains all of the > subversion keywords expanded. When patch goes to apply the patch file it will fail because the patch looks like this: > > Index: Makefile > =================================================================== > --- Makefile (revision 973) > +++ Makefile (working copy) > @@ -1,4 +1,4 @@ > -# > +# > # $Id$ > # > # The following creates/sets a few fundamental shell variables for us > > But the actual file looks like this: > > # > # $Id: Makefile 942 2008-10-17 18:55:48Z fubar $ # # The following > creates/sets a few fundamental shell variables for us > > Therefore, patch cannot figure out how to apply the patch because the > diff file does not match the real file because in the diff the $Id$ > keyword has not been expanded and in the copied file it has already > been expanded by subversion. Actual results: You get an error dialogue about Invoking patch failed, Please use GNU patch Expected results: Does this happen every time? yes provided that the 2 files under subversion are created according to the rules I laid out above Other information: I created a quick work around for this problem by simply using the subversion stored version(under .svn/text-base/FILE.svn-base) rather than using patch to undo the changes in my tree. Given the class structure, I think the proper fix is to create another method for the vc class, which "knows" how to create a clean copy of the file for meld to work on. This way the existing SW can be reused by cvs,bz...and subversion can have a derived class which uses the stored svn copy rather than using patch.
Agreed - the vc provider should give us the files to compare instead of us trying to recreate them.
Simple test case from bug 661561. 1) add and commit a file to SVN containing a line like this: # $Id: file.txt 8936 2010-09-20 12:24:51Z piper $ 2) set the svn:keywords property on the file to "Id". Do NOT commit this change to SVN! 3) Try to view the file in meld. Might try to attach a SVN <1.6 patch here soon, if I can convince myself that it's not a terrible hack.
Created attachment 203296 [details] [review] Patch This patch adds a new file-getter API to our VC backend, and implements it for SVN. In my very limited testing, this fixes the bug. I'd appreciate some further feedback and testing from people who actually use subversion on a regular basis, since this is a big change to a central part of the SVN-handling code. Note that this *will not work* for SVN 1.7. For that version, using the new 'svn patch' command to obtain the original file may be a possibility.
Hi, It's five years since this was reported and it's still causing me trouble :-) For the record, I'm using meld 1.6.1, GNU patch 2.7 and SVN 1.7.6 (r1370777). I haven't study the sources of meld deeply, but I believe that the correct solution to make this work should be something like: 1) Check svn:eol-style property when copying the compared file to temporary directory. 2) Convert the file to line endings specified in eol-style. 3) Apply the patch as usually, it should work now. I did few test in bash that should mimic the behavior of meld. Feel free to call me ignorant if I got it wrong :-) : # this is what currently happens when meld is comparing svn file: $ mkdir /tmp/meld $ cp Diag.h /tmp/meld $ svn diff Diag.h | patch -p0 -R -d /tmp/meld/ --dry-run patching file Diag.h Hunk #1 FAILED at 19. 1 out of 1 hunk FAILED -- saving rejects to file Diag.h.rej # this is what works correctly for me: $ mkdir /tmp/meld $ cp Diag.h /tmp/meld $ svn propget svn:eol-style Diag.h native $ sed -i 's/\r//g' /tmp/meld/Diag.h $ svn diff Diag.h | patch -p0 -R -d /tmp/meld/ --dry-run patching file Diag.h Could you implement a fix similar to this please? It is very annoying that half of my files can't be compared in meld, which is otherwise the best merge tool I ever worked with ;-)
Did you mean to add this comment to bug 613685 instead? Assuming so, we should continue discussion on that bug. The reason these bugs are still around is that I don't personally use Subversion, and developing against its quirks is... not fun. What would really help in developing a fix is an example repository that demonstrates the problem. If you could post a repro to create a broken repository that I can test against, I'll see whether I can make something work. As for your sample solution, surely we should convert the EOLs in the patch, not in the file itself? Otherwise we'll have the checkout file containing \r\n and the patched file with \n. While this *will* work in Meld currently because we ignore line endings in file comparisons, that's considered to be a bug, and we should at least notify of the difference. Converting EOLs in the patch may be a problem because our code supports multiple-file patches, and it's entirely possible that two files in a patch have different svn:eol-style properties. IIRC, I believe we never actually *use* multiple-file patches though, so maybe we could rip out support for that and proceed as above.
Of course, what I'd *really* like (almost as much as an example repo) is some testing or feedback on the patch I provided that should fix this issue in a much cleaner way. For anyone who was held back by lack of SVN 1.7 support, I've fixed that and have pushed a branch to: https://github.com/kaiw/meld/tree/DirectFileVC Please. Clone it, test it and let me know whether there are any issues. This is a major change to the infrastructure and I'm *not going to push it* without at least a few people confirming that it doesn't break in real use.
Hi Kai, Here are step by step instruction how to create the repo and demonstrate the problem: # create directory for repository mkdir /tmp/repo # create local repository svnadmin create /tmp/repo # check out working copy svn co file:///tmp/repo /tmp/wc # create a file echo -e "some\ntext" > /tmp/wc/file.txt file /tmp/wc/file.txt # add file to svn svn add /tmp/wc/file.txt # set eol-style to LF svn propset svn:eol-style LF /tmp/wc/file.txt # commit svn ci -m"first" /tmp/wc file /tmp/wc/file.txt # update svn up /tmp/wc file /tmp/wc/file.txt # modify file & mess up its line-endings echo -en "some\r\nmore\r\ntext\r\n" > /tmp/wc/file.txt file /tmp/wc/file.txt # try running meld meld /tmp/wc/file.txt I will also try to check the git branch today and let you know how it works.
Excellent! Thanks, that's really helpful. The patch appears to work for me for Subversion 1.7... ...except for a bug I recently introduced that appears to break directly invoking a version comparison on a single file. That has nothing to do with this however. Change the final line to run on the directory instead of the file itself and it should work fine. Either way, some further testing, especially in real world use, is needed.
(In reply to comment #6) > Of course, what I'd *really* like (almost as much as an example repo) is some > testing or feedback on the patch I provided that should fix this issue in a > much cleaner way. For anyone who was held back by lack of SVN 1.7 support, I've > fixed that and have pushed a branch to: > > https://github.com/kaiw/meld/tree/DirectFileVC > > Please. Clone it, test it and let me know whether there are any issues. This is > a major change to the infrastructure and I'm *not going to push it* without at > least a few people confirming that it doesn't break in real use. Downloaded and built the branch... It exhibits the same problem with the line endings. Also freezed at one point when comparing because of an IndexError: Traceback (most recent call last):
+ Trace 231089
for i in self._diff_files():
while step.next() is None:
self._update_merge_cache(sequences)
self._merge_cache[i] = (consume_blank_lines(c[0], texts, 1, 0),
c3, c4 = _find_blank_lines(texts[pane2], chunk[3], chunk[4])
while not txt[lo] and lo < hi:
raise IndexError IndexError
Let me know if I can help you more in any way with the eol-style issue.
So running that exact script you gave above fails on my system with Meld 1.6.1, and passes with that branch, after changing the last line to actually run the git copy of Meld rather than the system copy. Was that the test you were using, or something more complicated? Also, I've pushed a fix to the same branch for the bug I mentioned earlier with comparing single VC files. As for the second problem, any chance you could file a new bug with the files that caused that traceback?
I tested with my full repo and that is where the IndexError happened, I'll try to reproduce it again and file a new bug as you suggest. I also tried the simple test I sent before and it fails as well (with the "Invoking 'patch' failed." message box). I just hope I'm not invoking it wrong - I downloaded the whole branch from github as tar.gz, unpacked, and run "make PYTHON=python2" (I'm on ArchLinux where python 3 is default). Now I run the test with <path-to-the-extracted-archive>/bin/meld instead of just meld, that should be correct, right?
Created attachment 227227 [details] [review] Debug patch You shouldn't even need to run make. "python2 bin/meld <filename or folder>" from the archive directory should work just fine. You said earlier that you were running 1.6.1 by default? (and SVN 1.7?) if so, then just check the version number in the About dialog. The current git version is still shown as 1.6.0. Could you please apply this patch to your downloaded version and let me know what you get out? I'm trying to figure out whether SVN is still not giving the original files back, or whether there's a different bug that I'm missing. (Just run this on the example repo you provided. No point in getting the full dump for anything else until that works.)
Hi Kai, I also ran into this issue, when using meld to diff certain files. It was such a corner case. I was fixing a C file where the revision keyword has been capitalized incorrectly ("$REV" instead of the correct "$Rev"). The capitalization was preventing revision keyword expansion from working, so I fixed the capitalization, but when I tried to use meld to diff the file I got the good old "Invoking 'patch' failed. Maybe you don't have 'GNU patch' installed..." error. I cloned your DirectFileVC branch on your git repo, then applied your Debug patch and can confirm that your fix works. That said, would it be possible to post a diff which can be applied to the standard meld code (ideally the meld-1.6 branch)? I'd prefer to not have to use the DirectFileVC custom branch unless I have too... Thanks for the fix! Mark
Created attachment 230495 [details] [review] Backported patch I'm attaching a patch that applies against 1.6.1. This patch is barely tested, so YMMV. I can't in good conscience actually push this on the stable 1.6 branch, since it's a significant change in the way we get our files, so it will have to stay here as an unsupported patch.
After a couple of extra fixes, I've decided to just push the current version to git. As a result, this bug should be fixed in current HEAD. If it resurfaces, please file a new bug (or re-open this one) with as many details as possible.