Information that appears on info panel

[boruch]

1) I posted a pull request / patch to have the info panel (C-x i) display the mime type of the selected file (ref: ​https://github.com/MidnightCommander/mc/pull/117 ).

2) The info box includes two lines at the end (free space, free nodes) which I want to suggest should either not appear at all, or appear after an empty line and a header indicating that what follows is information about the file system foo (on which the currently selected file resides).

2.1) I could code that; I didn't see on the project's github page a "comments" section enabled, so I couldn't post this offer there in a way to integrate with my patch / pull request.

3) If the info box is going to provide information about one filesystem when a user selects file information for a single file, why not provide information for ALL filesystems?

3.1) Better in my opinion not to have any filesystem information in the info box, and have a separate box for filesystem information.

[andrew_b]

Replying to boruch:

1) I posted a pull request / patch to have the info panel (C-x i) display the mime type of the selected file (ref: ​https://github.com/MidnightCommander/mc/pull/117 ).

No pull requests. Only tickets here.

​https://www.midnight-commander.org/wiki/Hacking

[boruch]

Replying to andrew_b:

So, where does this stand at this point? I see that you acknowledged the github pull request and linked it to this ticket, and then closed the pull request. Does that mean the patch was rejected?

If what you want is for me to not use github and just post patches here as attachments, that's actually more straightforward than bouncing between two frameworks on two separate websites.

Let me know please. Also, if you agree that I'm being helpful, please also agree that trying to help shouldn't be so difficult.

[zaytsev]

If what you want is for me to not use github and just post patches here as attachments, that's actually more straightforward than bouncing between two frameworks on two separate websites.

Yes, exactly.

Also, if you agree that I'm being helpful, please also agree that trying to help shouldn't be so difficult.

Unfortunately, we can't disable pull requests on GitHub? due to their long standing stupid policy of only allowing to disable issues, but not pull requests. All we can do is to document that we don't work with pull requests, and redirect pull requests to Trac when someone files them. I know that some projects have bots that auto-close them, and file issues in their tracking systems, but I don't have the resources to implement that.

P.S. Your changes don't even compile currently, and not only because of the indentation problems.

[boruch]

Replying to zaytsev:

P.S. Your changes don't even compile currently, and not only because of the indentation problems.

That's strange. I tested the patches before committing and pushing them. I've just now posted here the commits in the form of three patch files, and here is some info about my build environment.

$gcc --version
gcc (Debian 4.9.2-10) 4.9.2
Copyright (C) 2014 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE

$make --version
GNU Make 4.0
Built for x86_64-pc-linux-gnu
Copyright (C) 1988-2013 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

[boruch]

Replying to zaytsev:

The patch to usermenu.c is included here in order to parallel what was posted in github. In reality, you'll see that it's a patch for a second issue (as documented on github), but I had github trouble pushing it to a separate branch.

So, you'll see that each diff files is independent, and can be patched / compiled separately. Let me know if you want me to post a separate ticket for the usermenu.c patch.

[egmont]

See #3713 for a closely related issue (I personally like the approach of that one much better).

[egmont]

I like the current (i.e. prior to your patches) approach of showing file system information for the given file. I like the idea of adding a separator line.

I dislike the approach of showing information of all file systems there. It totally wouldn't fit. It wouldn't emphasize the file system of the current location (or if it would: how?).

On my brand new simple Yakkety installation "df" prints 10 lines in addition to the header, "mount" prints 34 lines. Multiply this by 4 since currently we print 4 lines for the current FS. And good luck filtering out all the virtual ones in a way that works on all OSes, distros... I wouldn't want to go into the maintenance hell of enlisting and updating over time all the known virtual ones. Or execute "df" as an external command and parse its (probably OS-dependent) output?

If you want to see info about all file systems, I'd go for a totally standalone separate window that you can open somehow, but keep the Info panel focusing on the current file and its context (e.g. FS) only.

[egmont]

As for the mime type, I'm sorry but I don't like that either (actually I don't like the concept behind "file -i").

All the currently shown information is concrete exact piece of data. Mime type would be guesswork. The whole point of Mime is not to have to guess but to carry a certain piece of known meta-data along with a file.

It's typically used e.g. in HTTP, or in e-mail attachments etc. The sender of the object (e.g. webserver, e-mail's sender etc.) knows the value, and tells it to the receiver (browser, e-mail's recipient etc.).

If it would be a guessed value, probably the concept of Mime type wouldn't exist at all, there would be no need for it, since the receiver could guess just as well as the sender.

The whole point is e.g. to be able to tell which charset a non-UTF-8 8-bit text file is encoded in, to tell whether a file that looks like html, xml, svg... is actually to be interpreted (shown to the user) as plain text, to avoid security issues due to a file matching the magic bytes of multiple formats, and so on. It's configured via some reliable external means, e.g. the webserver's config (e.g. the .htaccess file), or output by the cgi or php script, etc., and then it's carried over the given network protocol so that the browser or the e-mail client can display the file correctly, as intended by the sender, rather than as guessed by the receiver. Alas, this metadata gets lost e.g. when you save the file.

So I just don't understand how a guessed value via "file -bi" (by the way: does this command work on all OSes, including BSDs etc.?) could be useful. Even if it is, it should not be called the "mime type" of the file, as it is clearly not, it is as most the "guessed mime type" of that.

That being said, I'd love to see #3713 getting addressed, and then anyone could easily configure their mc to show the result of this guesswork as well.

[boruch]

Replying to egmont:

First of all, do your comments mean that you've successfully compiled and tested the patches? I ask because the prior comment zaytsev indicated that he was having some trouble with the github commit that I had pushed, which was the version I had tested reasonably thoroughly.

I agree that adding information for all filesystems would be too much clutter. We may then be in total agreement about a possible change to that. What wasn't clear in your response was what you felt about a header line after the separator line to clearly indicate that next section was for filesystem foo.

Your characterizations of mimetype and 'file -i' ("guesswork", etc.) are totally at odds with the documentation at 'man(1) file' and seems like a modern attempt to redefine a unix standard command that's been around since at least 1973 (again, read the man page; it's all discussed there). See there for all the details, but I'll start you off with the first relevant contradictory excerpt there:

The magic tests are used to check for files with data in particular
fixed formats. The canonical example of this is a binary executable
(compiled program) a.out file, whose format is defined in <elf.h>,
<a.out.h> and possibly <exec.h> in the standard include directory.

That being said, I'd love to see #3713 getting addressed, and then anyone could easily configure their mc to show the result of this guesswork as well.

I guess that's an invitation for me to add a comment to that ticket.

Please confirm whether you've successfully compiled and tested the patch.

[egmont]

Replying to boruch:

First of all, do your comments mean that you've successfully compiled and tested the patches?

No, I did not say that. In fact I did not attempt to test your patch yet. I can have an opinion just by reading this ticket, can't I? :)

Your characterizations of mimetype and 'file -i' ("guesswork", etc.) are totally at odds with the documentation at 'man(1) file' and seems like a modern attempt to redefine a unix standard command that's been around since at least 1973 (again, read the man page; it's all discussed there).

The manual page of "file" says "file tests each argument in an attempt to classify it" and later on "will try to guess the text encoding" (emphasis by me). I think the manual should be more explicit that it makes a guess and that it has theoretically no way of knowing the file type for sure.

It's not the "file" utility, it's the definition and purpose of MIME type, and the corresponding RFC(s) that should matter.

Even without checking "file"'s manual or the MIME RFCs, it should be absolutely clear that there is no way to tell for sure e.g. the character encoding a file just by examining the file contents. It's only the creator of the file who knows the meaning of the accented letters placed there who can tell it for sure. This concept of the contents' creator who knows the semantics does exist in case of a webserver, but does not exist in case of a file found locally on your disk.

Example: Let's take a file containing three bytes: 102, 251, 116. Is this a plain text file encoded in ISO8859-1 containing the word "fût" ("barrel" in French), or encoded in ISO8859-2 containing the word "fűt" ("heats" in Hungarian). Absolutely no way to tell just by looking at the file itself. So, what should be MIME type reported??

Making a guess of the MIME type using any method (including the "file" utility) and claming that that is the MIME type of the file is conceptually wrong. There is just no way to get it right. The MIME type, by definition, cannot be determined just by examining the file and having no external metadata, context about its creation etc. available, it can only be guessed.

I guess that's an invitation for me to add a comment to that ticket.

It's a public ticket system, everyone is allowed to comment on any bug. I did not invite you to do so, but you are free to do it if you wish.

[boruch]

Replying to egmont:

See #3713 for a closely related issue (I personally like the approach of that one much better).

I looked at that ticket and commented there. Thanks for mentioning it because it raised a point appropriate to add here. The discussion for that ticket refers to the request of a third ticket, #2904, which I think is a great idea and am willing to add to the code of my patch for this ticket since it's so closely related.

The idea at #2904 is to include extended attribute information (reference: getfacl, getfattr, getcap) in the info panel (C-x i).

Excellent idea, and the coding would be a minor addition to my patch.

[boruch]

Replying to egmont:

Example: Let's take a file containing three bytes: 102, 251, 116. Is this a plain text file encoded in ISO8859-1 containing the word "fût" ("barrel" in French), or encoded in ISO8859-2 containing the word "fűt" ("heats" in Hungarian). Absolutely no way to tell just by looking at the file itself. So, what should be MIME type reported??

The short answer is yes. The longer answer is that your example will correctly be identified as mimetype "text/plain". Secondarily, it will, possibly, have its character set incorrectly identified, but not to the detriment of it being unreadable or uninterpretable, because any error will still associate it with a character set containing all the characters in the file. That's hardly a major criticism.

More to the point:

+ The information provided will reflect how pretty much any standard operating environment anywhere in the world will interpret the file.

+ The information provided will remain consistent and accurate regardless of whether one changes the filename's extension.

+ The information provided is based upon longstanding (40+ years) international standards.

[egmont]

Replying to boruch:

The short answer is yes.

Not sure to which question is this the answer. The quoted part contained only one question and that was not a yes-or-no one.

The longer answer is that your example will correctly be identified as mimetype "text/plain". Secondarily, it will, possibly, have its character set incorrectly identified, but not to the detriment of it being unreadable or uninterpretable, because any error will still associate it with a character set containing all the characters in the file. That's hardly a major criticism.

Getting the character set incorrectly is a major problem. It should never happen. Showing an û where an ű is required, or the other way around, is always unacceptable. Mandatory read: ​http://www.joelonsoftware.com/articles/Unicode.html. It sucks with plain text files because unfortunately you don't know the character set, but still it's unacceptable to claim an incorrect value.

This was just the simplest example I could quickly come up with. Others might be perhaps mixing cyrillic vs. greek charsets, mistaking a picture for a text file, mistaking a word for an mp3 (don't know if these in particular can happen, but similar ones definitely can, and were (probably still are) the cause of exploits out there).

+ The information provided will reflect how pretty much any standard operating environment anywhere in the world will interpret the file.

Except for charsets, that's a total mess anyways.

+ The information provided will remain consistent and accurate regardless of whether one changes the filename's extension.

That's true, although I don't know how it's relevant here.

+ The information provided is based upon longstanding (40+ years) international standards.

What standards? It's a constantly evolving set of magic rules trying to catch up with the ever increasing set of file formats.

---

If we omit the charset bits, the rest of the MIME type would probably be "correctly" guessed in most of the cases. (I quoted the word because there's no clear definition of what's "correct".) With the charset included it'd be way worse.

---

Even if you could guess the value "correctly" (in quotes, just as above) in 99% of the cases, what really worries me is the fundamental misunderstanding of the concept and the purpose of MIME.

It is not something that's meant to be derivable from the file. It is not derivable from the file. If it was, there would be no need for it, and hence this concept wouldn't exist at all. Everyone would just derive whatever they need from the file.

The purpose of MIME type is to be an external bit of metainfo, which, if correctly available, correctly carried along with the file (as it is e.g. in http, but not in case of local file), lets whoever will interpret the file interpret it correctly (e.g. display using the right charset instead of a badly chosen one where thet accents are messed up). That is, by concept, it is something that the creator of the file needs to create as well, knowing the intent (such as u with circumflex vs. u with double accent, see above), and pass along with the file.

By definition, the MIME type is not something you can derive from the file, and as such, IMO any attempt should at the very least be clearly marked as "guess" on the UI (but I'd rather not attempt to do it at all). I would definitely not do it in between many other fields that are all exact pieces of data read from the filesystem (inode, perms, timestamps...).

[andrew_b]

src_filemanager_info.c.diff​

fname_len = strlen (fname); 
command = malloc ((fname_len * 2) + command_substr_len + 1); 
if (command == NULL) 
   goto malloc_error_case_17; 
command_pos = stpcpy (command, "file -b -- '"); 
command_pos = stpcpy (command_pos, fname); 
command_pos = stpcpy (command_pos, "';file -bi -- '"); 
command_pos = stpcpy (command_pos, fname); 
command_pos = stpcpy (command_pos, "'"); 

Why not use GString instead of this spaghetti?

---

src_filemanager_usermenu.c.diff

#define END_OF_STRING '\0'

What reason for that? Do you know some end-of-line other than '\0'?

You use nested functions in this patch. Please don't. This is GCC extension and invalid standard C code.

[zaytsev]

Travis automatically builds all pull requests, yours was this failed build:

​https://travis-ci.org/MidnightCommander/mc/builds/176764806

[boruch]

Replying to andrew_b:

src_filemanager_info.c.diff​
Why not use GString instead of this spaghetti?

Only because my background isn't with GNOME and Glib.

---

src_filemanager_usermenu.c.diff

#define END_OF_STRING '\0'

What reason for that? Do you know some end-of-line other than '\0'?

That's just me and a readability coding convention from prior employment / projects.

---

You use nested functions in this patch. Please don't. This is GCC extension and invalid standard C code.

Agreed. I may have noted in the code that it was meant to be temporary, and suggested that the function be moved to its own source code file.

---

Also, in a parallel comment on this ticket, I'm discussing the TRAVIS build system with zaytzev. These nested functions were not objected to by TRAVIS, so maybe in addition to what I'm suggesting to zaytsev, the compiler variables should address this.

---

Also, please remember that this part of the patch, ie. the changes to usermenu.c aren't really part of this ticket, because its unrelated to the info box. I included it to maintain parallelism with the github pull request which went so wrong. One of the things that went wrong there was that when I thought I was staging a second pull request (related to usermenu parsing), it was just added to this prior pull request.

[boruch]

Replying to zaytsev:

Travis automatically builds all pull requests, yours was this failed build:

​https://travis-ci.org/MidnightCommander/mc/builds/176764806

Hi again. I took a look at your link, and it points out a bug in the mc MAKEFILE compilation directives, or at least an inconsistency between what the mc MAKEFILE is directing my local machine and what TRAVIS is performing.

The travis output log indicates that it registered a failure (line 2604) because it was treating all warnings being as errors. That isn't how the mc MAKEFILE is directing my local build. In fact, my local build flags other warnings elsewhere in the mc build.

Makefile:587: warning: overriding recipe for target 'update-po'
Makefile:524: warning: ignoring old recipe for target 'update-po'
Makefile:587: warning: overriding recipe for target 'update-po'
Makefile:524: warning: ignoring old recipe for target 'update-po'
Makefile:587: warning: overriding recipe for target 'update-po'
Makefile:524: warning: ignoring old recipe for target 'update-po'

Also, in a parallel comment from andrew_b on this ticket, it seems that mc wants to reject nested functions and maybe other GNU C extensions. If that's correct, then that would be another modification to be made to the MAKEFILE compile directives, and to TRAVIS.

That would turn this ticket into FOUR different tickets: 1) info box patch already submitted; 2) info box patch proposed (adding extended attributes); 3) user menu; 4) build system.

[egmont]

If this "feature" gets accepted, please make sure to test these scenarios and do something reasonable:

• user has no permissions to read the file,
• file is within an archive (e.g. .tar.gz) -- do you uncompress to run "file" (damn slow) or display "n/a" or similar?
• file resides on a remote virtual file system (e.g. ftpfs) -- same question,
• file is a symlink, named pipe, block device etc.

Also note that the output of "file" is always in English, which looks bad if embedded inside the neatly localized UI of mc. For the mime type ("file -bi") that's okay because that's a technical identifier meant to remain the same. For the English description ("file -b") it's not so much. Don't know what to do though.

[boruch]

Separates out filesystem info from file info; correct2 two TRAVIS warnings

[boruch]

Replying to zaytsev:

Travis automatically builds all pull requests, yours was this failed build:

​https://travis-ci.org/MidnightCommander/mc/builds/176764806

I just uploaded a second version of the patch to info.c, which was the file to which TRAVIS objected, and I addressed those objections. However, I expect the following trouble from TRAVIS, which may kind of illustrates a problem in the way its being used.

The original TRAVIS objection was that I was using function fgets, but that I was "ignoring return value of ‘fgets’, declared with attribute warn_unused_result ". But I don't need the result, ie the return value, for anything! Fine. In order to appease TRAVIS, I needlessly declared a dummy variable and assigned it to the fgets return value. That probably won't be sufficient for TRAVIS, because it might now fail, claiming that I'm assigning but not using that dummy variable.

Anyway, this version of the patch makes a few changes to the info box:
+ Add a blank line to separate file info from filesystem info
+ Outdent the "filesystem" line to correspond to the "file" line
+ Move the "mimetype" line up to the last line of file info

[zaytsev-work]

There is no bug in the build system. You have to build with --enable-maintainer-mode to get -Werror, which is what Travis does. Also, if you don't need the result of a non-void function you have to cast it away. It would really help if you were less assertive in your communications, especially when your statements are misguided.

[boruch]

Replying to zaytsev-work:

There is no bug in the build system. You have to build with --enable-maintainer-mode to get -Werror, which is what Travis does. Also, if you don't need the result of a non-void function you have to cast it away. It would really help if you were less assertive in your communications, especially when your statements are misguided.

1] --enable-maintainer-mode isn't what's responsible: I deleted all *.o / *.lo in my build tree, re-ran autogen and configure with --enable-maintainer-mode, deleted the mc binary, and re-ran make / make install. It built successfully. So I went back and did it a second time, with the same result. So the issue with TRAVIS remains open in that regard.

2] When do you want to NOT have -Werror, or whatever other stringencies --enable-maintainer-mode will apply? It seems to me you NEVER really want that, because if all legitimate builds by TRAVIS have those stringencies, then any build by anyone else will pass with them, and should any fail, that's a sign that the build was tampered with / modified.

'"Jim Meyering, the inventor of the AM_MAINTAINER_MODE macro was swayed by François’s
arguments, and got rid of AM_MAINTAINER_MODE in all of his packages."'
source: ​https://www.gnu.org/software/automake/manual/html_node/maintainer_002dmode.html
See there for details.

My suggestion was/is to set the CFLAGS variables for whatever stringency you want, be it -Werror, -Wpedantic, or anything else. Why use use maintainer mode at all?

3] Andrew mentioned that the project wants patches posted here, instead of github pull requests, so potential contributors won't be running TRAVIS, and as long as TRAVIS is reporting errors that users' MAKE isn't, it will make contributing more burdensome.

4] It just occurred to me to ask - What compiler are you expecting me to be using? Might it make a difference? As I posted towards the top of this page, I'm using GNU.

5] Do you get a failed build when you build the patched info.c locally? It may be that you can confirm the issue with TRAVIS by trying that.

[zaytsev]

1) So the issue with TRAVIS remains open in that regard.

The issue remains open with your development environment.

2) Why use use maintainer mode at all?

Because turning on -Werror and other maintainer-only options by default makes life difficult for distro maintainers and users who wouldn't want their builds to fail on a compiler that emits a diagnostic we didn't address at the time of the release.

3) It will make contributing more burdensome.

I'm sorry, but a certain baseline set of skills is simply required to efficiently contribute to a project such as mc. It is absolutely trivial to fork the repository on GitHub? and enable Travis for your fork, so that you can make sure that your stuff builds before you submit your patches.

4) What compiler are you expecting me to be using?

You can see in the Travis build log which toolchain it is using, including compiler versions, etc.

5) Do you get a failed build when you build the patched info.c locally?

Sorry, I don't have time to look into this.

[boruch]

Replying to zaytsev:

5) Do you get a failed build when you build the patched info.c locally?

Sorry, I don't have time to look into this.

That was obvious to me from reading your not-well-thought-out responses to my questions 1-4. Well, if you ever do find the time, review what you wrote, rewrite, drop me a line, and we can continue. If that turns out to be never, that's also fine. Bye.

[andrew_b]

src_filemanager_mountlist.c.diff​ applied as [98f0c3faba4d9a5b3da930e3c000aae81566edf5].

[zaytsev]

That was obvious to me from reading your not-well-thought-out responses to my questions 1-4. Well, if you ever do find the time, review what you wrote, rewrite, drop me a line, and we can continue. If that turns out to be never, that's also fine. Bye.

Luckily, this is a public Trac instance, and so it's up to the readers to decide whose responses are "not-well-thought-out" and whose are not. I'm always trying to use my limited time to give some useful pointers, but unfortunately I'm unable to afford hand-holding individuals who totally ignore them and are unwilling to learn from the documentation. Bye!