-
-
Notifications
You must be signed in to change notification settings - Fork 52
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed libmagic bug #490
Fixed libmagic bug #490
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree to merge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This BROKE opening of one of my Changelog.Debian.gz files (which I edit a lot in packing changelogs into .debs) which contain a single ChangeLog.Debian text file
Test file included, this one had some minor issues as a Debian changelog but engrampa never had trouble with this file or any similar file without this PR applied
I am using a local build, and have no way to further test this under other conditions. I cannot verify anything on a stock
distro as I do not have a landline and do not have the bandwidth to download installers.
As it stands if this is merged I will have to disable libmagic support or revert this locally, as I
cannot use Engrampa otherwise with this applied. If nobody else can duplicate this, I won't merge it but won't block merging either, and will patch it out of my local builds.
|
@lukefromdc Could you show me the error message |
@lukefromdc Can you help me run commands from the terminal command line and view the output
|
Actually I can reproduce @lukefromdc's problem. And looking into it is that libmagic returns IMO, there's 2 things to do here:
I am not sure of all the consequences, but these changes fixes it for me: diff --git a/src/file-utils.c b/src/file-utils.c
index 4983a7b..6d4f328 100644
--- a/src/file-utils.c
+++ b/src/file-utils.c
@@ -555,7 +555,7 @@ gboolean
is_mime_type (const char *mime_type,
const char *pattern)
{
- return (strcasecmp (mime_type, pattern) == 0);
+ return g_content_type_equals (mime_type, pattern);
}
const char*
diff --git a/src/fr-init.c b/src/fr-init.c
index c018bfa..c7a876d 100644
--- a/src/fr-init.c
+++ b/src/fr-init.c
@@ -292,7 +292,7 @@ fr_registered_command_get_capabilities (FrRegisteredCommand *reg_com,
FrMimeTypeCap *cap;
cap = g_ptr_array_index (reg_com->caps, i);
- if (strcmp (mime_type, cap->mime_type) == 0)
+ if (is_mime_type (mime_type, cap->mime_type))
return cap->current_capabilities;
}
Additionally, I'd think we'd need this (but it's not required to open the file) diff --git a/src/fr-init.c b/src/fr-init.c
index c018bfa..c7a876d 100644
--- a/src/fr-init.c
+++ b/src/fr-init.c
@@ -312,7 +312,7 @@ fr_registered_command_get_potential_capabilities (FrRegisteredCommand *reg_com,
FrMimeTypeCap *cap;
cap = g_ptr_array_index (reg_com->caps, i);
- if ((cap->mime_type != NULL) && (strcmp (mime_type, cap->mime_type) == 0))
+ if ((cap->mime_type != NULL) && (is_mime_type (mime_type, cap->mime_type)))
return cap->potential_capabilities;
}
@@ -508,7 +508,7 @@ get_mime_type_index (const char *mime_type)
int i;
for (i = 0; mime_type_desc[i].mime_type != NULL; i++)
- if (strcmp (mime_type_desc[i].mime_type, mime_type) == 0)
+ if (is_mime_type (mime_type_desc[i].mime_type, mime_type))
return i;
return -1;
} |
Thanks @cwendling will take a look at this and update the PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This now works fine on my end after the latest changes, no more problem with gzipped Debian changelog files.
@lukefromdc thanks for testing, but I think merging was a tad too fast here :) No need to revert though, but I think we might need to look at this a tad more carefully. @sinha-toyeesh did you check all the implications of my suggested changes? As I mentioned, I'm not sure if this has side effects, like how it handles MIME aliases. I'm not sure it poses any problem, but as there is special handling for them, it might be worth checking a bit what's supposed to happen, and whether my suggestions interferes or not. Also, we should really integrate @zhuyaliang's suggested change, because right now when using libmagic the logic is like |
If this was a screwup, let's create a second PR with the cleanups in it and get this done right. How the binaries put out by distros work for end users is more important than how neat the changelogs look. We could really use more testers here, as all too often PR's don't get reviewed or there are not enough testers to find the corner cases. Years ago, GNOME's development versions were distributed with the alpha versions of Ubuntu, I don't know if rolling releases like Debian Sid also distributed them. Ideally that would be resumed as it would make bugs getting all the way to releases in final versions of distros much less likely. |
@cwendling I made the recommended changes as suggested by @zhuyaliang, as I too realised the fallacy of checking for the same thing twice in a single function, which seemed a tad bit wierd to me. |
I wholeheartedly agree with @lukefromdc regarding both of his points, especially so on the topic of additional testers ;) |
A blooper has been made there: * if ENABLE_MIME is set, the intention was to try, in order: magic, content, filename; but it was made filename, content, magic (which was the same as before the changes); * if ENABLE_MIME is not set, the intention was to try, in order: filename, content, magic; but it has been made magic, content, magic (notice the duplicate, and the missing "filename"). This probably doesn't change much in the wild as magic is gonna work most of the time, but it's especially problematic that the non-libmagic case doesn't have the filename test. Anyway, fix this so the code is consistent, and we retain the behavior for the non-libmagic case, and have the new expected one for the libmagic case.
Unfortunately no, you didn't. Look at the changes again, you actually made a blunder and only changed the non-libmagic code path making it never try filename, rather than making the libmagic one prefer magics. See #491 for what I believe is the intended behavior.
The libmagic case maybe (although it probably ultimately depends on the version of the magic database, for which @lukefromdc and I probably have a more recent version); but the question I raised is the other implications of the Anyhow, the changes I suggested seemed fine to me, but then again I didn't spend enough time evaluating the consequences. If we have enough testing, maybe it's enough? we'll see :) |
The followup PR didn't create any problems on my end with a variety of files including the previous offenders |
A blooper has been made there: * if ENABLE_MIME is set, the intention was to try, in order: magic, content, filename; but it was made filename, content, magic (which was the same as before the changes); * if ENABLE_MIME is not set, the intention was to try, in order: filename, content, magic; but it has been made magic, content, magic (notice the duplicate, and the missing "filename"). This probably doesn't change much in the wild as magic is gonna work most of the time, but it's especially problematic that the non-libmagic case doesn't have the filename test. Anyway, fix this so the code is consistent, and we retain the behavior for the non-libmagic case, and have the new expected one for the libmagic case.
Referring to issue #206 , Engrampa uses Libmagic only if extension is not known, otherwise it determines mime type from the file extension, even though the
--enable-magic
flag is used, along withlibmagic
support.This pull request adds an if condition to use
libmagic
if--enable-magic
if used.