GNOME Bugzilla – Bug 598296
Make meld behave like diff when diff <file> <directory>
Last modified: 2009-10-17 06:11:03 UTC
When making a diff between a file and a directory, diff assume that you append the file name to the directory. So : $ diff foo bar/ is actually -> $ diff foo bar/foo I think it will be great that meld have the same behavior than diff in this case (even with multiple folder) Currently, it shows a "Cannot compare a mixture of files and directories." Attached patch solves this issue in testing if <directory + file name> (taking the first specified filename) exists for each specified directory. In case there is a match, we are making a diff, otherwise, we get the "Cannot compare a mixture of files and directories." message. I attached the patch or you can find my git repository at http://www.didrocks.fr/temp/meld/ containing the fix in a "behave-like-diff" branch.
Created attachment 145361 [details] [review] patch for fixing the behavior
This sounds like possibly-interesting command-line behaviour, but the patch will cause the same thing to happen in-app (e.g., when selected in the New dialogue or the above-pane file entries), and I'm not so sure whether that's a good idea.
Good point! So, I gave some tests in the gui: * If I try in the New dialog: - the first tab "File Comparaison" only accept file (when I Browse). If I try to put manually a directory, I got: "Could not read from '...', the error was: Is a directory: ... - the second tab is not affected by my code (directory comparison) as when we have only directories, I don't change the behavior. * Now, with the above-pane file entries: I get the same behavior when mixing a file with a directory, ie if I put a directory in the second pane for instance, as I have an existing file in the first, I get "... is a directory" * I tested that in both cases, comparing a "foo" file with a bar/ directory: 1. bar/ directory not containing "foo" file 2. bar/ directory containing "foo" file So, my patch doesn't seem to affect those behavior but only the command line.
Review of attachment 145361 [details] [review]: The patch looks good to me
Thanks for checking the gui behaviour - that looks good. However, the case of three-way comparisons is a little more complicated. Looking at diff3, it doesn't seem to support the <file> <directory> syntax at all, so I don't know how these *should* behave. Testing the patch: - meld a b c/ creates a three-way comparison between a, b and c/a - meld b a c/ shows an error dialog (when c/b doesn't exist) - meld a c/ b creates a three-way comparison between a, c/a and b - meld c/ a b creates a three-way comparison between c/a, a and b This argument ordering looks odd. While I'm not sure, I'd suggest: - if there is only one file, take it as the base - if there are two files, take the one immediately prior to the single directory as the base which would change the first two cases above, and make the last case show an error. Thoughts? Also, there should be some documentation of how this works. We probably *should* have a man page, but since we don't, how about adding something to the command-line description about this mode?
Yes, the 4 cases you described behave like this in my patch and I was aware of that. I like your proposal, I can change my patch easily to behave like this. I can also add something in the command-line description (just have to find where it is). added testcase (which will be supported too, even in the existing patch), will be: meld a b/ c/ creates a three-way comparison between a, b/a and c/a I'll try to catch some time to work on it within the day.
Ok, so, here is a new patch covering what has been previously discussed. My git branch has been updated too. I added a quick usage coverage. Please note that this patch support also: meld b/ a (taking b/basename(a)) as diff support also that. Consequently: b/ a c/ works too (comparing b/basename(a), a, c/basename(a)) What do you thing about it?
Created attachment 145532 [details] [review] Patch v2 fixing diff 3 argument orders reliability
There is some five-space indentation starting at "builtfilename = ...", but otherwise looks good to me. As for the usage note, I think maybe the first note is enough to hint that we support the behaviour, and we can leave the last line off for the sake of brevity. I tried to pull from your git repo, but I get: fatal: http://www.didrocks.fr/temp/meld/info/refs not found: did you run git update-server-info on the server? Could you fix your repo or attach a 'git format-patch' version of the patch (for commit message & authorship purposes), and I'll apply.
Sorry for the five-space indentation, didn't notice that at first glance. It's fixed. The second usage example was to emphasize that the last filename was used. But yes, we can avoid cluttering it. Concerning git, I must admit I'm more familiar with svn and bzr (I used git most of the time to get new git release and package them). So, this was the right time to try it a little more :) Ok, so, I ran "git update-server-info" and I can't clone from another post. So, I created a new clone bare repository from my previous one (from what I saw on the internet) and it seems to work: http://www.didrocks.fr/temp/meld-public I attach here the patches too in git format if this doesn't work for you. Thanks for your responsiveness.
Created attachment 145583 [details] [review] git commit 1
Created attachment 145584 [details] [review] git patch format
Cheers for that. I reworded the command-line help slightly and committed. Thanks for both the bug report and the patch.