Re: [PATCH] Add directory diff options for shallow file comparison mode

On Fri, Jan 4, 2013 at 10:12 PM, Kai Willadsen <kai willadsen gmail com> wrote:
Did you mean to drop the CC on this?

Whoops. Sorry, my mistake :)
On 1 January 2013 13:36, Cristian Dinu <goc9000 gmail com> wrote:
> On Mon, Dec 31, 2012 at 10:32 PM, Kai Willadsen <kai willadsen gmail com>
> wrote:
>> Is this a problem for anything other than FAT drives in practice? And
>> in any case, does it really make sense to declare that files with
>> differing modes are different? (I know that some tools do, but that
>> doesn't mean we have to.)
> As far as I can see, the default NTFS support in a Linux installation
> doesn't actually allow for permissions on a NTFS volume at all: all files
> will appear to have a hardcoded set of permissions as configured in /fstab.
> Exactly what this set is seems to depend on which utility is doing the
> mounting: my external NTFS drive shows permissions like "rw-------' for all
> files, but an internal NTFS partition automounted on startup using the
> standard utilities in the distribution has permissions like 'rwxrwxrwx'
> (every file is executable!). In fact, even the owner is different. This is
> why Meld might run into problems even when comparing between filesystems of
> the same type.

Yeah, okay. That's pretty awful, and we can't do anything about it.

> I think it does make sense for Meld to consider file permissions by default,
> as most source versioning systems do take them into account (which makes
> sense - think of execute bits on scripts, or .htaccess files which have
> strict permissions requirements) and I think this will be the default
> expectation of a user that is employing Meld in a development context.
> However, there should also be an option to turn this off.

In case you haven't noticed, I'm *very* against just including options
to fix things. I'm not at all sure that we should actually consider
permissions in a shallow comparison. An alternative would be to ignore
permissions, but include a mode column in the comparison; that column
could even indicate differences separately if required. (I'm currently
reviewing a patch to add configurable column displays to folder

I'm OK with those alternatives as well.
>> > Second, different
>> > filesystems may and often do have different timestamp resolutions, so
>> > I've
>> > added an option whereby the user can normalize the timestamps to a
>> > common
>> > resolution, allowing for a correct comparison.
>> ...and as above? Does anything other than FAT have a
>> worse-than-1-second resolution?
>> If these two options are here solely for the benefit of external
>> FAT(32)-formatted drives (and systems that don't support sub-second
>> resolution) then can we deal with these without additional
>> preferences?
>> If we do need to support a timestamp resolution preference, then I
>> think we should provide fixed options (e.g., exact, 100msec, 1sec,
>> 2sec, 1 hour, 1 day) rather than a continuous slider.
> Well, even NTFS has a lower timestamp resolution than ext4 (100ns versus
> 1ns), so a match at full resolution will still fail. On the other hand, if
> you think the user would be confused by extra preferences, I don't see why
> we couldn't make this mechanism completely transparent. Meld could
> automatically check the filesystem types on which the LHS/RHS files reside,
> and switch to the lower time resolution as required.

Sadly, I don't think we can. I looked into this, and sniffing
filesystems appears to be quite involved, particularly across
different OSes. It's basically shelling out to blkid or mount (or
something else on Windows), possibly requiring root permissions to do
so. And even then, we can't do anything about network mounted
filesystems (which Meld doesn't currently support properly, but I hope
to fix that one day).

Having said that, if you can find a cross-platform way of doing this,
I'm *definitely* interested.

Hm, it looks like we're stuck with the option for the time being then. We can make it so that it has 3 choices, qualified with the corresponding filesystem type: "2s (FAT) / 100ns (NTFS) / 1ns (ext)". That shouldn't be too confusing, I hope.
>> The patch is a good start! However, you're touching *two* things that
>> I'm cautious about --- UI and preferences --- so some patience might
>> be required to get this done.
>> I'd prefer to review the patch on bugzilla, and there is already at
>> least one bug requesting shallow comparisons that you could attach it
>> to. Having said that, here are some initial comments from a quick
>> look:
>>  * The changes in meldapp and the signal emission and such aren't
>> needed at all. Just add an on_preference_changed() method to DirDiff
>> and do the options handling there.
>>  * Getting PreferencesDialog to accumulate the preferences for you
>> feels unnecessary; again, just have DirDiff do it. Also, the
>> __getattr__ stuff is overkill... just manually grab the prefs from
>> their keys.
>>  * The superficial comparison branch in _files_same only ever
>> shortcuts for identical files. I'd expect instead that if _files_same
>> gets to the superficial comparison, we will either have DodgySame or
>> DodgyDifferent as a result right there. (Unless I'm missing something,
>> and there's a good reason to have it otherwise).
>> cheers,
>> Kai
> Thanks for the feedback! I will adjust my code accordingly and submit it
> there.

Thanks for continuing to look into this.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]