After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 598296 - Make meld behave like diff when diff <file> <directory>
Make meld behave like diff when diff <file> <directory>
Status: RESOLVED FIXED
Product: meld
Classification: Other
Component: general
1.3.x
Other All
: Normal normal
: ---
Assigned To: Stephen Kennedy
Stephen Kennedy
Depends on:
Blocks:
 
 
Reported: 2009-10-13 15:29 UTC by Didier Roche
Modified: 2009-10-17 06:11 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch for fixing the behavior (1.54 KB, patch)
2009-10-13 15:30 UTC, Didier Roche
reviewed Details | Review
Patch v2 fixing diff 3 argument orders reliability (2.80 KB, patch)
2009-10-15 15:51 UTC, Didier Roche
none Details | Review
git commit 1 (1.90 KB, patch)
2009-10-16 07:11 UTC, Didier Roche
none Details | Review
git patch format (1.98 KB, patch)
2009-10-16 07:13 UTC, Didier Roche
none Details | Review

Description Didier Roche 2009-10-13 15:29:45 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.
Comment 1 Didier Roche 2009-10-13 15:30:33 UTC
Created attachment 145361 [details] [review]
patch for fixing the behavior
Comment 2 Kai Willadsen 2009-10-13 20:34:13 UTC
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.
Comment 3 Didier Roche 2009-10-14 06:07:23 UTC
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.
Comment 4 Vincent Legoll 2009-10-14 09:40:27 UTC
Review of attachment 145361 [details] [review]:

The patch looks good to me
Comment 5 Kai Willadsen 2009-10-14 21:04:10 UTC
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?
Comment 6 Didier Roche 2009-10-15 07:50:11 UTC
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.
Comment 7 Didier Roche 2009-10-15 15:50:11 UTC
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?
Comment 8 Didier Roche 2009-10-15 15:51:06 UTC
Created attachment 145532 [details] [review]
Patch v2 fixing diff 3 argument orders reliability
Comment 9 Kai Willadsen 2009-10-15 20:01:32 UTC
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.
Comment 10 Didier Roche 2009-10-16 07:08:56 UTC
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.
Comment 11 Didier Roche 2009-10-16 07:11:52 UTC
Created attachment 145583 [details] [review]
git commit 1
Comment 12 Didier Roche 2009-10-16 07:13:53 UTC
Created attachment 145584 [details] [review]
git patch format
Comment 13 Kai Willadsen 2009-10-17 06:11:03 UTC
Cheers for that. I reworded the command-line help slightly and committed. Thanks for both the bug report and the patch.