Bug 72597

Created attachment 90605 [details] [review]
patch

This is good, but don't the keynote "directories" have a stable suffix?

Eg. OSX apps use ".app". This would make the detection much easier, directories with "*.keynote" would be keynote packages, without the need to dive into the tree (which could be quite expensive on remote filesystems).

(In reply to comment #2)
> This is good, but don't the keynote "directories" have a stable suffix?
> 
> Eg. OSX apps use ".app". This would make the detection much easier,
> directories with "*.keynote" would be keynote packages, without the need to
> dive into the tree (which could be quite expensive on remote filesystems).

Yes, they all have suffix ".key". If you think the matching for internal files is not really necessary, good. I will send an updated patch shortly.

(In reply to comment #3)
> (In reply to comment #2)
> > This is good, but don't the keynote "directories" have a stable suffix?
> > 
> > Eg. OSX apps use ".app". This would make the detection much easier,
> > directories with "*.keynote" would be keynote packages, without the need to
> > dive into the tree (which could be quite expensive on remote filesystems).
> 
> Yes, they all have suffix ".key". If you think the matching for internal
> files is not really necessary, good. I will send an updated patch shortly.

I think that matching directories with ".key" should be enough. I'm not sure how well it works in practice, but now's the time to get bugs fixed :)

Created attachment 90615 [details] [review]
updated patch

Okay, here is an updated patch. But I have no idea how to add the new "files" to the testsuite to make it work.

(In reply to comment #5)
> Created attachment 90615 [details] [review] [review]
> updated patch
> 
> Okay, here is an updated patch. But I have no idea how to add the new
> "files" to the testsuite to make it work.

Add a dummy file inside the directory.

Created attachment 90617 [details] [review]
updated patch

(In reply to comment #6)
> (In reply to comment #5)
> > Created attachment 90615 [details] [review] [review] [review]
> > updated patch
> > 
> > Okay, here is an updated patch. But I have no idea how to add the new
> > "files" to the testsuite to make it work.
> 
> Add a dummy file inside the directory.

That is actually a good idea, but it is not what I meant (I had nearly complete content in the .key dirs). I cannot get the test to succeed. When I add the .key dirs to tests/tree-list, nothing is matched. And the tests/list test apparently always tries to open the tested path as a file, so it fails also.

(In reply to comment #8)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > Created attachment 90615 [details] [review] [review] [review] [review]
> > > updated patch
> > > 
> > > Okay, here is an updated patch. But I have no idea how to add the new
> > > "files" to the testsuite to make it work.
> > 
> > Add a dummy file inside the directory.
> 
> That is actually a good idea, but it is not what I meant (I had nearly
> complete content in the .key dirs). I cannot get the test to succeed. When I
> add the .key dirs to tests/tree-list, nothing is matched. And the tests/list
> test apparently always tries to open the tested path as a file, so it fails
> also.

Yeah, I guess this is going to be a problem for most implementations of this specification. We'll have to discuss this...

This won't work. The spec says that matching is only done on files, not on directories. The specification would need to be changed to allow a separate directory glob.

Something like this:
+ <!ATTLIST glob type (file|directory) #IMPLIED>

with file being the default implied value. This would also require loads of fixes in the stack. I'm not personally going to implement this, but patches would be accepted.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/xdg/shared-mime-info/issues/26.